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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
