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

Heinrich Schuchardt
2015-02-25T19:10:06Z

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