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

David Rientjes
2015-02-25T21:07:22Z

On Wed, 25 Feb 2015, Heinrich Schuchardt wrote:

> > 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.
>

Your series is adding an unused argument in patch 1 and passes in UINT_MAX
for no clear reason. Patch 2 then starts to use the argument because the
new helper function needs to cast to u64 and you want to prevent overflow,
so the UINT_MAX becomes apparent but should rather be part of the
implementation of the function that does the casting.

Futhermore, the most significant part of the review still hasn't been
addressed: the fact that doing (u64)totalram_pages * (u64)PAGE_SIZE can
overflow your own numerator, so the calculation will need to be changed
itself.

> 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?
>

I'm not the person who will be merging these, so feel free to follow the
guidance of whoever will be doing that. I think if there was inspection
of the series that it's really two things proposed together: a
divide-by-zero bugfix and changing threads-max to respect limits.

I understand there is depdenencies on the same code between those two
things so proposing them as part of the same patchset is good, but I would
have proposed the current divide-by-zero bugfix first which can be done
solely in fork_init() and then change threads-max afterwards.