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

Ingo Molnar
2015-02-25T10:17:59Z

* 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