On Sun, 29 Jan 2012 14:18:32 +0200 Gilad Ben-Yossef <gilad@xxxxxxxxxxxxx> wrote: > On Sat, Jan 28, 2012 at 2:12 AM, Andrew Morton > <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 26 Jan 2012 12:02:00 +0200 > > Gilad Ben-Yossef <gilad@xxxxxxxxxxxxx> 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! > > 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>