Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932218AbbBYVHW (ORCPT ); Wed, 25 Feb 2015 16:07:22 -0500 Received: from mail-ie0-f181.google.com ([209.85.223.181]:39673 "EHLO mail-ie0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932155AbbBYVHQ (ORCPT ); Wed, 25 Feb 2015 16:07:16 -0500 Date: Wed, 25 Feb 2015 13:07:12 -0800 (PST) From: David Rientjes X-X-Sender: rientjes@chino.kir.corp.google.com To: Heinrich Schuchardt cc: Ingo Molnar , Andrew Morton , Aaron Tomlin , Andy Lutomirski , Davidlohr Bueso , "David S. Miller" , Fabian Frederick , Guenter Roeck , "H. Peter Anvin" , Jens Axboe , Joe Perches , Johannes Weiner , Kees Cook , Michael Marineau , Oleg Nesterov , "Paul E. McKenney" , Peter Zijlstra , Prarit Bhargava , Rik van Riel , Rusty Russell , Steven Rostedt , Thomas Gleixner , Vladimir Davydov , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3 v5] kernel/fork.c: new function for max_threads In-Reply-To: <54EE1DA8.7030504@gmx.de> Message-ID: References: <1424722477-23758-1-git-send-email-xypron.glpk@gmx.de> <1424806701-30099-1-git-send-email-xypron.glpk@gmx.de> <1424806701-30099-2-git-send-email-xypron.glpk@gmx.de> <54ECEBD0.9060800@gmx.de> <20150225101751.GB7134@gmail.com> <54EE1DA8.7030504@gmx.de> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3155 Lines: 74 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/