Re: Fwd: [PATCH] mm: add config option to select the initial overcommit mode

Sebastian Frias
2016-05-10T13:02:34Z

Hi Andy,

On 05/10/2016 02:39 PM, Andy Whitcroft wrote:
> On Tue, May 10, 2016 at 02:00:47PM +0200, Sebastian Frias wrote:
>> Hi,
>>
>> Using checkpatch.pl on the forwarded patch results in:
>>
>> WARNING: please write a paragraph that describes the config symbol fully
>> #57: FILE: mm/Kconfig:451:
>> + config OVERCOMMIT_GUESS
>>
>> WARNING: please write a paragraph that describes the config symbol fully
>> #64: FILE: mm/Kconfig:458:
>> + config OVERCOMMIT_ALWAYS
>>
>> but there is a 'help' section for those 'config' sections.
>> NOTE: I followed the same indentation than the code laying just above the place where I inserted mine.
>>
>> I think it is a false positive, what do you think?
>>
>> Best regards,
>>
>> Sebastian
>
> Well, I am expecting the issue to be that the per option help is not
> indented within the option like I am expecting ...

Changing the indentation does not solve the issue.
Marc (in CC) just told me he had had the same issue and it was related to having less than 4 lines of 'help'.
If I add a dummy line to the 'help' section the warning goes indeed away.

Also notice that 'config OVERCOMMIT_NEVER' is not giving warnings even if its 'help' section has also 3 lines.

>
>> Date: Tue, 10 May 2016 13:56:30 +0200
>> From: Sebastian Frias <sf84@laposte.net>
>> To: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>, Michal
>> Hocko <mhocko@suse.com>, Linus Torvalds <torvalds@linux-foundation.org>
>> CC: LKML <linux-kernel@vger.kernel.org>, mason <slash.tmp@free.fr>
>> Subject: [PATCH] mm: add config option to select the initial overcommit mode
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101
>> Thunderbird/31.2.0
>>
>> Currently the initial value of the overcommit mode is OVERCOMMIT_GUESS.
>> However, on embedded systems it is usually better to disable overcommit
>> to avoid waking up the OOM-killer and its well known undesirable
>> side-effects.
>>
>> This config option allows to setup the initial overcommit mode to any of
>> the 3 available values, OVERCOMMIT_GUESS (which remains as default),
>> OVERCOMMIT_ALWAYS and OVERCOMMIT_NEVER.
>> The overcommit mode can still be changed thru sysctl after the system
>> boots up.
>>
>> This config option depends on CONFIG_EXPERT.
>> This patch does not introduces functional changes.
>>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> ---
>>
>> NOTE: I understand that the overcommit mode can be changed dynamically thru
>> sysctl, but on embedded systems, where we know in advance that overcommit
>> will be disabled, there's no reason to postpone such setting.
>>
>> I would also be interested in knowing if you guys think this option should
>> disable sysctl access for overcommit mode, essentially hardcoding the
>> overcommit mode when this option is used.
>>
>> NOTE2: I tried to track down the history of overcommit but back then there
>> were no single patches apparently and the patch that appears to have
>> introduced the first overcommit mode (OVERCOMMIT_ALWAYS) is commit
>> 9334eab8a36f ("Import 2.1.27"). OVERCOMMIT_NEVER was introduced with commit
>> 502bff0685b2 ("[PATCH] strict overcommit").
>> My understanding is that prior to commit 9334eab8a36f ("Import 2.1.27")
>> there was no overcommit, is that correct?
>>
>> NOTE3: checkpatch.pl is warning about missing description for the config
>> symbols ("please write a paragraph that describes the config symbol fully")
>> but my understanding is that that is a false positive (or the warning message
>> not clear enough for me to understand it) considering that I have added
>> 'help' sections for each 'config' section.
>> ---
>> mm/Kconfig | 32 ++++++++++++++++++++++++++++++++
>> mm/util.c | 8 +++++++-
>> 2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/Kconfig b/mm/Kconfig
>> index abb7dcf..6dad57d 100644
>> --- a/mm/Kconfig
>> +++ b/mm/Kconfig
>> @@ -439,6 +439,38 @@ choice
>> benefit.
>> endchoice
>>
>> +choice
>> + prompt "Overcommit Mode"
>> + default OVERCOMMIT_GUESS
>> + depends on EXPERT
>> + help
>> + Selects the initial value for Overcommit mode.
>> +
>> + NOTE: The overcommit mode can be changed dynamically through sysctl.
>> +
>> + config OVERCOMMIT_GUESS
>> + bool "Guess"
>
> I am expecting the help below to be indented at the same level as the
> bool above. As you have done with the help for the choice itself. I am
> pretty sure checkpatch is assuming the "contents" of the config item are
> all intented more than it is.
>
>> + help
>> + Selecting this option forces the initial value of overcommit mode to
>> + "Guess" overcommits. This is the default value.
>> + See Documentation/vm/overcommit-accounting for more information.
> [...]
>
> -apw
>