linux-kernel - Re: [PATCH v11 01/13] vmstat: add quiet_vmstat

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 21 Mar 2016 11:44:02 -0400
From:	Chris Metcalf <cmetcalf@...lanox.com>
To:	Michal Hocko <mhocko@...nel.org>
CC:	Gilad Ben Yossef <giladb@...hip.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Rik van Riel" <riel@...hat.com>, Tejun Heo <tj@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Christoph Lameter <cl@...ux.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Andy Lutomirski <luto@...capital.net>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v11 01/13] vmstat: add quiet_vmstat_sync function

On 03/21/2016 10:34 AM, Michal Hocko wrote:
> On Fri 11-03-16 17:10:11, Chris Metcalf wrote:
>> In commit f01f17d3705b ("mm, vmstat: make quiet_vmstat lighter")
>> the quiet_vmstat() function became asynchronous, in the sense that
>> the vmstat work was still scheduled to run on the core when the
>> function returned.  For task isolation, we need a synchronous
>> version of the function that guarantees that the vmstat worker
>> will not run on the core on return from the function.  Add a
>> quiet_vmstat_sync() function with that semantic.
> The full series hasn't reached my inbox and from the above it is not
> really clear where is the new function supposed to be used.
>
> It is usually preferable to include the consumer of the new API in the
> same patch. Is this case any special that it couldn't be done that way?

I've seen this done both ways (i.e. smushing things all together, vs
breaking them apart) and my sense is that particularly for larger patch
series, keeping individual atomic patches that bisect cleanly is the
better choice.

In this particular case, there are a three patches in mm that all
individually are needed for task isolation; merging all three into
the patch that enables task isolation would have made that patch even
larger and harder to review.  (Similarly, one could argue that even
that patch on its own does nothing, and is purely an enablement patch
for the various architecture-specific follow-on patches that actually
provide the hooks to make it work, so by your argument perhaps the
whole thing should be merged into a single giant patch...)

In any case, I think you are absolutely right that providing more
context to reviewers is the right thing, and I'll make sure in future
patch series to include all the reviewers from "enabling patches" into
the patch that uses the particular enablement.  For this series, the
patch that uses the quiet_vmstat_sync() call is here:

https://lkml.kernel.org/r/1457734223-26209-5-git-send-email-cmetcalf@mellanox.com

The cover letter email is here if you're curious for more context:

https://lkml.kernel.org/r/1457734223-26209-1-git-send-email-cmetcalf@mellanox.com

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com