[PATCH] Add checking num-threads of makedumpfile

["Zhou, Wenjian/周文剑"]

On 08/25/2015 02:11 PM, Minfei Huang wrote:
> On 08/25/15 at 01:49pm, "Zhou, Wenjian/周文剑" wrote:
>> Hello Minfei,
>>
>> Thanks for your quick reply.
>>
>> On 08/25/2015 01:42 PM, Minfei Huang wrote:
>>> On 08/25/15 at 12:51pm, Zhou Wenjian wrote:
>>>> A new feature that doing compressing and writing by multi-threads
>>>> has been added in makedumpfile. The thread num is specified by
>>>> "--num-threads NUM". According to its implementation, there will
>>>> be performance degradation if the threads are more than cpus.
>>>> So we should check it.
>>>>
>>>> Signed-off-by: Zhou wenjian <zhouwj-fnst at cn.fujitsu.com>
>>>> ---
>>>>   kdumpctl |   16 ++++++++++++++++
>>>>   1 files changed, 16 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/kdumpctl b/kdumpctl
>>>> index b504734..a4b4681 100755
>>>> --- a/kdumpctl
>>>> +++ b/kdumpctl
>>>> @@ -259,6 +259,22 @@ check_config()
>>>>   		esac
>>>>   	done < $KDUMP_CONFIG_FILE
>>>>
>>>> +	grep -v "^#" $KDUMP_CONFIG_FILE | grep -q "num-threads"
>>>
>>> This filter cannot handle the all of the corner cases, like the
>>> following.
>>>
>>>   # grep -v "^#" /etc/kdump.conf  | grep "num-threads"
>>>   ext4 /dev/mapper/num-threads
>>>
>>> So it is better to use the exact regular expression to filter out the
>>> string. Maybe you can use $core_collector to filter out the string
>>> --num_threads.
>>>
>>
>> I see.
>>
>>>> +	if [ $? -eq 0 ];then
>>>> +		local nr_cpus=1
>>>> +		local num_threads=0
>>>> +		local core_collector=`grep -v "^#" $KDUMP_CONFIG_FILE | grep "^core_collector"`
>>>> +
>>>> +		num_threads=`echo ${core_collector#*--num-threads} | awk '{print $1}'`
>>>> +		nr_cpus=`echo ${KDUMP_COMMANDLINE_APPEND#*nr_cpus=} | awk '{print $1}'`
>>>> +
>>>> +		test $num_threads -ge $nr_cpus &> /dev/null
>>>> +		if [ $? -eq 0 ];then
>>>
>>> To keep the style, it is better to use "if [ $a -ge $b ]".
>>>
>>
>> Using "test" is to handle the situation that the $num_threads or $nr_cpus is not an integer.
>>
>
> Is it allowed for $num_threads or $nr_cpus, if the value is not the
> integer?
>

Of course not integer is invalid. But the value of num_threads won't be checked
until makedumpfile is executed.
If user sets "core_collector makedumpfile --num-threads a" in kdump.conf, it will
lead to a fault here.

>> BTW, should we add some descriptions to this feature of makedumpfile in some where?
>
> IMO, it is appropriate to document it in the makedumpfile manpage, like
> the feature --split. For kdump, we only document the default usages for
> makedumpfile in kexec-kdump-howto.txt.
>

-- 
Thanks
Zhou