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

[Ingo Molnar <mingo@kernel.org>]

* 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