From: Michal Hocko <mhocko@kernel.org> To: Dan Streetman <ddstreet@ieee.org> Cc: Yu Zhao <yuzhao@google.com>, Seth Jennings <sjenning@redhat.com>, Linux-MM <linux-mm@kvack.org>, linux-kernel <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Date: Fri, 2 Dec 2016 15:38:49 +0100 [thread overview] Message-ID: <20161202143848.GP6830@dhcp22.suse.cz> (raw) In-Reply-To: <CALZtONBhvHNpGW4u1a8pVQeHx_8dX17vnFS52rrYbWA5dOtQ8w@mail.gmail.com> On Fri 02-12-16 09:24:35, Dan Streetman wrote: > On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko <mhocko@kernel.org> 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@google.com> > >> --- > >> 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@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> > > > > -- > > Michal Hocko > > SUSE Labs -- Michal Hocko SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@kernel.org> To: Dan Streetman <ddstreet@ieee.org> Cc: Yu Zhao <yuzhao@google.com>, Seth Jennings <sjenning@redhat.com>, Linux-MM <linux-mm@kvack.org>, linux-kernel <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Date: Fri, 2 Dec 2016 15:38:49 +0100 [thread overview] Message-ID: <20161202143848.GP6830@dhcp22.suse.cz> (raw) In-Reply-To: <CALZtONBhvHNpGW4u1a8pVQeHx_8dX17vnFS52rrYbWA5dOtQ8w@mail.gmail.com> On Fri 02-12-16 09:24:35, Dan Streetman wrote: > On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko <mhocko@kernel.org> 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@google.com> > >> --- > >> 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@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> > > > > -- > > Michal Hocko > > SUSE Labs -- 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>
next prev parent reply other threads:[~2016-12-02 14:39 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-11-30 21:15 [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Yu Zhao 2016-11-30 21:15 ` Yu Zhao 2016-12-02 13:46 ` Michal Hocko 2016-12-02 13:46 ` Michal Hocko 2016-12-02 14:24 ` Dan Streetman 2016-12-02 14:24 ` Dan Streetman 2016-12-02 14:38 ` Michal Hocko [this message] 2016-12-02 14:38 ` Michal Hocko 2016-12-02 14:44 ` Michal Hocko 2016-12-02 14:44 ` Michal Hocko 2016-12-02 14:56 ` Dan Streetman 2016-12-02 14:56 ` Dan Streetman 2016-12-02 15:19 ` [PATCH] hotplug: make register and unregister notifier API symmetric Michal Hocko 2016-12-02 15:19 ` Michal Hocko 2016-12-03 5:15 ` kbuild test robot 2016-12-05 6:08 ` Michal Hocko 2016-12-05 6:08 ` Michal Hocko 2016-12-03 7:18 ` kbuild test robot 2016-12-05 20:59 ` Yu Zhao 2016-12-05 20:59 ` Yu Zhao 2016-12-06 9:30 ` Michal Hocko 2016-12-06 9:30 ` Michal Hocko 2016-12-05 21:11 ` [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Yu Zhao 2016-12-05 21:11 ` Yu Zhao
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20161202143848.GP6830@dhcp22.suse.cz \ --to=mhocko@kernel.org \ --cc=ddstreet@ieee.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=sjenning@redhat.com \ --cc=yuzhao@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.