Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1534984

Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y

From Michal Hocko <mhocko@kernel.org>
Newsgroups linux.kernel
Subject Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y
Date 2016-12-02 15:50 +0100
Message-ID <sJXjX-58s-19@gated-at.bofh.it> (permalink)
References <sJksh-2R0-31@gated-at.bofh.it> <sJWnV-4tM-85@gated-at.bofh.it> <sJXjX-58s-21@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Yu Zhao <yuzhao@google.com> - 2016-11-30 22:20 +0100
  Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Michal Hocko <mhocko@kernel.org> - 2016-12-02 14:50 +0100
    Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Michal Hocko <mhocko@kernel.org> - 2016-12-02 15:50 +0100
      Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Michal Hocko <mhocko@kernel.org> - 2016-12-02 15:50 +0100
        Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Dan Streetman <ddstreet@ieee.org> - 2016-12-02 16:00 +0100
          [PATCH] hotplug: make register and unregister notifier API symmetric Michal Hocko <mhocko@kernel.org> - 2016-12-02 16:20 +0100
            Re: [PATCH] hotplug: make register and unregister notifier API  symmetric kbuild test robot <lkp@intel.com> - 2016-12-03 06:20 +0100
              Re: [PATCH] hotplug: make register and unregister notifier API  symmetric Michal Hocko <mhocko@kernel.org> - 2016-12-05 07:10 +0100
            Re: [PATCH] hotplug: make register and unregister notifier API  symmetric kbuild test robot <lkp@intel.com> - 2016-12-03 08:20 +0100
            Re: [PATCH] hotplug: make register and unregister notifier API  symmetric Yu Zhao <yuzhao@google.com> - 2016-12-05 22:00 +0100
              Re: [PATCH] hotplug: make register and unregister notifier API  symmetric Michal Hocko <mhocko@kernel.org> - 2016-12-06 10:40 +0100
    Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Dan Streetman <ddstreet@ieee.org> - 2016-12-02 15:50 +0100
    Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Yu Zhao <yuzhao@google.com> - 2016-12-05 22:20 +0100

csiph-web