Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y
From: Dan Streetman <hidden>
Date: 2016-12-02 14:57:10
Also in:
lkml
On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko [off-list ref] wrote:
On Fri 02-12-16 15:38:48, Michal Hocko wrote:quoted
On Fri 02-12-16 09:24:35, Dan Streetman wrote:quoted
On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko [off-list ref] wrote:quoted
On Wed 30-11-16 13:15:16, Yu Zhao wrote:quoted
__unregister_cpu_notifier() only removes registered notifier from its linked list when CPU hotplug is configured. If we free registered CPU notifier when HOTPLUG_CPU=n, we corrupt the linked list. To fix the problem, we can either use a static CPU notifier that walks through each pool or just simply disable CPU notifier when CPU hotplug is not configured (which is perfectly safe because the code in question is called after all possible CPUs are online and will remain online until power off). v2: #ifdef for cpu_notifier_register_done during cleanup.this ifedfery is just ugly as hell. I am also wondering whether it is really needed. __register_cpu_notifier and __unregister_cpu_notifier are noops for CONFIG_HOTPLUG_CPU=n. So what's exactly that is broken here?hmm, that's interesting, __unregister_cpu_notifier is always a noop if HOTPLUG_CPU=n, but __register_cpu_notifier is only a noop if HOTPLUG_CPU=n *and* MODULE. If !MODULE, __register_cpu_notifier doesOK, I've missed the MODULE partquoted
actually register! This was added by commit 47e627bc8c9a70392d2049e6af5bd55fae61fe53 ('hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU') and looks like it's to allow built-ins to register so they can notice during boot when cpus are initialized.I cannot say I wound understand the motivation but that is not really all that important.quoted
IMHO, that is the real problem - sure, without HOTPLUG_CPU, nobody should ever get a notification that a cpu is dying, but that doesn't mean builtins that register notifiers will never unregister their notifiers and then free them.Yes that is true. That suggests that __unregister_cpu_notifier should the the symmetric thing to the __register_cpu_notifier for CONFIG_MODULE, right?I meant the following. Completely untested
agreed, but also needs the non-__ version, and kernel/cpu.c needs tweaking to move those functions out of the #ifdef CONFIG_HOTPLUG_CPU section.
quoted hunk ↗ jump to hunk
---diff --git a/include/linux/cpu.h b/include/linux/cpu.h index 797d9c8e9a1b..8d7b473426af 100644 --- a/include/linux/cpu.h +++ b/include/linux/cpu.h@@ -120,6 +120,7 @@ extern void __unregister_cpu_notifier(struct notifier_block *nb); #ifndef MODULE extern int register_cpu_notifier(struct notifier_block *nb); extern int __register_cpu_notifier(struct notifier_block *nb); +extern void __unregister_cpu_notifier(struct notifier_block *nb); #else static inline int register_cpu_notifier(struct notifier_block *nb) { --Michal Hocko SUSE Labs
-- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>