On Fri 02-12-16 09:24:35, Dan Streetman wrote:
> On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko <mhocko@xxxxxxxxxx> wrote:
> > On Wed 30-11-16 13:15:16, Yu Zhao wrote:
> >> __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 does
OK, I've missed the MODULE part
> 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.
> 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?
> Changing zswap is only working around the symptom; instead, hotplug
> should be changed to provide unregister_cpu_notifier in all cases
> where register_cpu_notifier is provided.
Agreed.
> >> Signe-off-by: Yu Zhao <yuzhao@xxxxxxxxxx>
> >> ---
> >> mm/zswap.c | 14 ++++++++++++++
> >> 1 file changed, 14 insertions(+)
> >>
> >> diff --git a/mm/zswap.c b/mm/zswap.c
> >> index 275b22c..2915f44 100644
> >> --- a/mm/zswap.c
> >> +++ b/mm/zswap.c
> >> @@ -118,7 +118,9 @@ struct zswap_pool {
> >> struct kref kref;
> >> struct list_head list;
> >> struct work_struct work;
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> struct notifier_block notifier;
> >> +#endif
> >> char tfm_name[CRYPTO_MAX_ALG_NAME];
> >> };
> >>
> >> @@ -448,6 +450,7 @@ static int __zswap_cpu_comp_notifier(struct zswap_pool *pool,
> >> return NOTIFY_OK;
> >> }
> >>
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> static int zswap_cpu_comp_notifier(struct notifier_block *nb,
> >> unsigned long action, void *pcpu)
> >> {
> >> @@ -456,27 +459,34 @@ static int zswap_cpu_comp_notifier(struct notifier_block *nb,
> >>
> >> return __zswap_cpu_comp_notifier(pool, action, cpu);
> >> }
> >> +#endif
> >>
> >> static int zswap_cpu_comp_init(struct zswap_pool *pool)
> >> {
> >> unsigned long cpu;
> >>
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> memset(&pool->notifier, 0, sizeof(pool->notifier));
> >> pool->notifier.notifier_call = zswap_cpu_comp_notifier;
> >>
> >> cpu_notifier_register_begin();
> >> +#endif
> >> for_each_online_cpu(cpu)
> >> if (__zswap_cpu_comp_notifier(pool, CPU_UP_PREPARE, cpu) ==
> >> NOTIFY_BAD)
> >> goto cleanup;
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> __register_cpu_notifier(&pool->notifier);
> >> cpu_notifier_register_done();
> >> +#endif
> >> return 0;
> >>
> >> cleanup:
> >> for_each_online_cpu(cpu)
> >> __zswap_cpu_comp_notifier(pool, CPU_UP_CANCELED, cpu);
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> cpu_notifier_register_done();
> >> +#endif
> >> return -ENOMEM;
> >> }
> >>
> >> @@ -484,11 +494,15 @@ static void zswap_cpu_comp_destroy(struct zswap_pool *pool)
> >> {
> >> unsigned long cpu;
> >>
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> cpu_notifier_register_begin();
> >> +#endif
> >> for_each_online_cpu(cpu)
> >> __zswap_cpu_comp_notifier(pool, CPU_UP_CANCELED, cpu);
> >> +#ifdef CONFIG_HOTPLUG_CPU
> >> __unregister_cpu_notifier(&pool->notifier);
> >> cpu_notifier_register_done();
> >> +#endif
> >> }
> >>
> >> /*********************************
> >> --
> >> 2.8.0.rc3.226.g39d4020
> >>
> >> --
> >> 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/ .
> >> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
> >
> > --
> > Michal Hocko
> > SUSE Labs
--
Michal Hocko
SUSE Labs
--
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/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>