linux-kernel - Re: [v7 7/8] mm: only IPI CPUs to drain local pages if they exist

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOtvUMfNFt4KYZP0DceWP1M+=r1_tXzOa8qMKXm2wE4XzsYkyQ@mail.gmail.com>
Date:	Tue, 31 Jan 2012 08:32:21 +0200
From:	Gilad Ben-Yossef <gilad@...yossef.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org, Mel Gorman <mel@....ul.ie>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Christoph Lameter <cl@...ux.com>,
	Chris Metcalf <cmetcalf@...era.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Russell King <linux@....linux.org.uk>, linux-mm@...ck.org,
	Pekka Enberg <penberg@...nel.org>,
	Matt Mackall <mpm@...enic.com>,
	Sasha Levin <levinsasha928@...il.com>,
	Rik van Riel <riel@...hat.com>,
	Andi Kleen <andi@...stfloor.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, Avi Kivity <avi@...hat.com>,
	Michal Nazarewicz <mina86@...a86.com>,
	Milton Miller <miltonm@....com>
Subject: Re: [v7 7/8] mm: only IPI CPUs to drain local pages if they exist

On Mon, Jan 30, 2012 at 11:49 PM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Sun, 29 Jan 2012 14:18:32 +0200
> Gilad Ben-Yossef <gilad@...yossef.com> wrote:
>
>> On Sat, Jan 28, 2012 at 2:12 AM, Andrew Morton
>> <akpm@...ux-foundation.org> wrote:
>> > On Thu, 26 Jan 2012 12:02:00 +0200
>> > Gilad Ben-Yossef <gilad@...yossef.com> wrote:
>> >
>> >> Calculate a cpumask of CPUs with per-cpu pages in any zone
>> >> and only send an IPI requesting CPUs to drain these pages
>> >> to the buddy allocator if they actually have pages when
>> >> asked to flush.
>> >>
>>
>> ...
>>
>> > Can we end up sending an IPI to a now-unplugged CPU? __That won't work
>> > very well if that CPU is now sitting on its sysadmin's desk.
>>
>> Nope. on_each_cpu_mask() disables preemption and calls smp_call_function_many()
>> which then checks the mask against the cpu_online_mask
>
> OK.
>
> General rule of thumb: if a reviewer asked something then it is likely
> that others will wonder the same thing when reading the code later on.
> So consider reviewer questions as a sign that the code needs additional
> comments!

Right, point taken.

>
>> > There's also the case of CPU online. __We could end up failing to IPI a
>> > CPU which now has some percpu pages. __That's not at all serious - 90%
>> > is good enough in page reclaim. __But this thinking merits a mention in
>> > the comment. __Or we simply make this code hotplug-safe.
>>
>> hmm.. I'm probably daft but I don't see how to make the code hotplug safe for
>> CPU online case. I mean, let's say we disable preemption throughout the
>> entire ordeal and then the CPU goes online and gets itself some percpu pages
>> *after* we've calculated the masks, sent the IPIs and waiting for the
>> whole thing
>> to finish but before we've returned...
>
> This is inherent to the whole drain-pages design - it's only a
> best-effort thing and there's nothing to prevent other CPUs from
> undoing your work 2 nanoseconds later.
>
> The exception to this is the case of suspend, which drains the queues
> when all tasks (and, hopefully, IRQs) have been frozen.  This is the
> only way to make draining 100% "reliable".
>
>> I might be missing something here, but I think that unless you have some other
>> means to stop newly hotplugged CPUs to grab per cpus pages there is nothing
>> you can do in this code to stop it. Maybe make the race window
>> shorter, that's all.
>>
>> Would adding a comment such as the following OK?
>>
>> "This code is protected against sending  an IPI to an offline CPU but does not
>> guarantee sending an IPI to newly hotplugged CPUs"
>
> Looks OK.  I'd mention *how* this protection comes about:
> on_each_cpu_mask() blocks hotplug and won't talk to offlined CPUs.

Good. I'll send an updated patch set.

Thanks :-)
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker
gilad@...yossef.com
Israel Cell: +972-52-8260388
US Cell: +1-973-8260388
http://benyossef.com

"Unfortunately, cache misses are an equal opportunity pain provider."
-- Mike Galbraith, LKML
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/