Re: [PATCH 1/3 v5] kernel/fork.c: new function for max_threads

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 25.02.2015 11:17, Ingo Molnar wrote:
> 
> * David Rientjes <rientjes@google.com> wrote:
> 
>> The problem is with the structure of your patchset.  You 
>> want three patches.  There's one bugfix patch, a 
>> preparation patch, and a feature patch.  The bugfix patch 
>> should come first so that it can be applied, possibly, to 
>> stable kernels and doesn't depend on unnecessary 
>> preparation patches for features.
>>
>> 1/3: change the implementation of fork_init(), with 
>> commentary, to avoid the divide by zero on certain 
>> arches, enforce the limits, and deal with variable types 
>> to prevent overflow.  This is the most urgent patch and 
>> fixes a bug.
>>
>> 2/3: simply extract the fixed fork_init() implementation 
>> into a new set_max_threads() in preparation to use it for 
>> threads-max, (hint: UINT_MAX and ignored arguments should 
>> not appear in this patch),
>>
>> 3/3: use the new set_max_threads() implementation for 
>> threads-max with an update to the documentation.
> 
> I disagree strongly: it's better to first do low-risk 
> cleanups, then do any fix and other changes.
> 
> This approach has four advantages:
> 
>   - it makes the bug fix more apparent, in the context of 
>     an already cleaned up code.
> 
>   - it strengthens the basic principle that 'substantial 
>     work should be done on clean code'.
> 
>   - on the off chance that the bugfix introduces problems 
>     _upstream_, it's much easier to revert in a late -rc 
>     kernel, than to first revert the cleanups. This 
>     happens in practice occasionally, so it's not a
>     theoretical concern.
> 
>   - the _backport_ to the -stable kernel will be more 
>     robust as well, because under your proposed structure, 
>     what gets tested upstream is the fix+cleanup, while the
>     backport will only be the fix, which does not get 
>     tested by upstream in isolation. If there's any 
>     (unintended) side effect of the cleanup that happens to
>     be an improvement, then we'll break -stable!
> 
> It is true that this makes backports a tiny bit more 
> involved (2 cherry-picks instead of just one), but -stable 
> kernels can backport preparatory patches just fine, and 
> it's actually an advantage to have -stable kernel code 
> match the upstream version as much as possible.
> 
> Thanks,
> 
> 	Ingo
> --

Hello David,

to my understanding the biggest no-go for you was that patch 1/3
introduces a function argument that is not needed until patch 3/3.

Could you accept the sequence proposed by Ingo if I leave away the
argument max_threads_suggested in patch 1 and 2 and only introduce it in
patch 3?

So patch 1/4 will refactor the code that may result in division by zero
to new function static void set_max_threads(void). Furthermore it will
change the interface of fork_init to void __init fork_init(void).

Patch 2/4 keeps the interface static void set_max_threads(void) but
corrects the coding using div64_u64.

Patch 3/4 changes the interface to
static void set_max_threads(unsigned int max_threads_suggested),
calls this function in fork_init with value UINT_MAX and calls it
when threads-max is set with the user value.

Patch 4/4 adds the necessary information about theads-max in
Documentation/sysctl/kernel.txt.
I would not like to put this in patch 3/4 as Jonathan Corbet uses a
separate git tree.

Best regards

Heinrich