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

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

[Yu Zhao]

__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.

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

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

[Yu Zhao]

__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.

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>

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

[Michal Hocko]

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?

> 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

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

[Michal Hocko]

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?

> 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

--
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>

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

[Dan Streetman]

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
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.

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.

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.

>
>> 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

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

[Dan Streetman]

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
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.

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.

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.

>
>> 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

--
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>

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

[Michal Hocko]

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

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

[Michal Hocko]

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>

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

[Michal Hocko]

On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> 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?

I meant the following. Completely untested
---
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

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

[Michal Hocko]

On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> 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?

I meant the following. Completely untested
---
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>

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

[Dan Streetman]

On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 02-12-16 15:38:48, Michal Hocko wrote:
>> 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?
>
> 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.


> ---
> 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

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

[Dan Streetman]

On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 02-12-16 15:38:48, Michal Hocko wrote:
>> 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?
>
> 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.


> ---
> 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>

[PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

[Let's CC more people - the thread started
http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com]

On Fri 02-12-16 09:56:26, Dan Streetman wrote:
> On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> >> 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?
> >
> > 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.

OK, this is still only compile tested. Yu Zhao, assuming you were able
to trigger the original problem could you test with the below patch
please?
---
>From c812fe4e519914aa37f092d3a0321038fadcdde7 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Fri, 2 Dec 2016 16:06:56 +0100
Subject: [PATCH] hotplug: make register and unregister notifier API symmetric

Yu Zhao has noticed that __unregister_cpu_notifier only unregisters its
notifiers when HOTPLUG_CPU=y while the registration might succeed even
when HOTPLUG_CPU=n if MODULE is enabled. This means that e.g. zswap
might keep a stale notifier on the list on the manual clean up during
the pool tear down and thus corrupt the list. Fix this issue by making
unregister APIs symmetric to the register so there are no surprises.

Fixes: 47e627bc8c9a ("[PATCH] hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU")
Cc: stable # zswap needs it 4.3+
Reported-by: Yu Zhao <yuzhao@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/cpu.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 797d9c8e9a1b..c8938eb21e34 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -105,22 +105,16 @@ extern bool cpuhp_tasks_frozen;
 		{ .notifier_call = fn, .priority = pri };	\
 	__register_cpu_notifier(&fn##_nb);			\
 }
-#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
-#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
 
-#ifdef CONFIG_HOTPLUG_CPU
 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);
 extern void __unregister_cpu_notifier(struct notifier_block *nb);
-#else
 
-#ifndef MODULE
-extern int register_cpu_notifier(struct notifier_block *nb);
-extern int __register_cpu_notifier(struct notifier_block *nb);
-#else
+#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
+#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+
 static inline int register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -130,7 +124,6 @@ static inline int __register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
 }
-#endif
 
 static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
-- 
2.10.2

-- 
Michal Hocko
SUSE Labs

[PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

[Let's CC more people - the thread started
http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com]

On Fri 02-12-16 09:56:26, Dan Streetman wrote:
> On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> >> 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?
> >
> > 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.

OK, this is still only compile tested. Yu Zhao, assuming you were able
to trigger the original problem could you test with the below patch
please?
---

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

Hi Michal,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc7 next-20161202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/hotplug-make-register-and-unregister-notifier-API-symmetric/20161203-114815
config: i386-randconfig-r0-201648 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/oprofile/built-in.o: In function `nmi_timer_shutdown':
>> nmi_timer_int.c:(.text+0x238b): undefined reference to `__unregister_cpu_notifier'
   arch/x86/oprofile/built-in.o: In function `nmi_shutdown':
   nmi_int.c:(.text+0x2793): undefined reference to `__unregister_cpu_notifier'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21659 bytes --]

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

On Sat 03-12-16 13:15:42, kbuild test robot wrote:
> Hi Michal,
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.9-rc7 next-20161202]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/hotplug-make-register-and-unregister-notifier-API-symmetric/20161203-114815
> config: i386-randconfig-r0-201648 (attached as .config)
> compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    arch/x86/oprofile/built-in.o: In function `nmi_timer_shutdown':
> >> nmi_timer_int.c:(.text+0x238b): undefined reference to `__unregister_cpu_notifier'
>    arch/x86/oprofile/built-in.o: In function `nmi_shutdown':
>    nmi_int.c:(.text+0x2793): undefined reference to `__unregister_cpu_notifier'

Ohh, right. I have missed that unregister functions definitions are
guarded as well. This patch should hopefully be correct finally.
Please note it also exports register/unregister callbacks when
CONFIG_HOTPLUG_CPU is not defined which is not really needed strictly
speaking because those are only used when !MODULE but I would rather
not make the code more complicated. If maintainers prefer I can guard
exports separately of course.
---
>From 29a6f62ac4407b6e22ce86ac81d3856e5d01fea3 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Fri, 2 Dec 2016 16:06:56 +0100
Subject: [PATCH] hotplug: make register and unregister notifier API symmetric

Yu Zhao has noticed that __unregister_cpu_notifier only unregisters its
notifiers when HOTPLUG_CPU=y while the registration might succeed even
when HOTPLUG_CPU=n if MODULE is enabled. This means that e.g. zswap
might keep a stale notifier on the list on the manual clean up during
the pool tear down and thus corrupt the list. Fix this issue by making
unregister APIs symmetric to the register so there are no surprises.

Fixes: 47e627bc8c9a ("[PATCH] hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU")
Cc: stable # zswap requires for 4.3+
Reported-by: Yu Zhao <yuzhao@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/cpu.h | 15 ++++-----------
 kernel/cpu.c        |  2 +-
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 797d9c8e9a1b..c8938eb21e34 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -105,22 +105,16 @@ extern bool cpuhp_tasks_frozen;
 		{ .notifier_call = fn, .priority = pri };	\
 	__register_cpu_notifier(&fn##_nb);			\
 }
-#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
-#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
 
-#ifdef CONFIG_HOTPLUG_CPU
 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);
 extern void __unregister_cpu_notifier(struct notifier_block *nb);
-#else
 
-#ifndef MODULE
-extern int register_cpu_notifier(struct notifier_block *nb);
-extern int __register_cpu_notifier(struct notifier_block *nb);
-#else
+#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
+#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+
 static inline int register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -130,7 +124,6 @@ static inline int __register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
 }
-#endif
 
 static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 341bf80f80bd..73fb59fda809 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -578,7 +578,6 @@ void __init cpuhp_threads_init(void)
 	kthread_unpark(this_cpu_read(cpuhp_state.thread));
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
 EXPORT_SYMBOL(register_cpu_notifier);
 EXPORT_SYMBOL(__register_cpu_notifier);
 void unregister_cpu_notifier(struct notifier_block *nb)
@@ -595,6 +594,7 @@ void __unregister_cpu_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(__unregister_cpu_notifier);
 
+#ifdef CONFIG_HOTPLUG_CPU
 /**
  * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
  * @cpu: a CPU id
-- 
2.10.2

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

On Sat 03-12-16 13:15:42, kbuild test robot wrote:
> Hi Michal,
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.9-rc7 next-20161202]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/hotplug-make-register-and-unregister-notifier-API-symmetric/20161203-114815
> config: i386-randconfig-r0-201648 (attached as .config)
> compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    arch/x86/oprofile/built-in.o: In function `nmi_timer_shutdown':
> >> nmi_timer_int.c:(.text+0x238b): undefined reference to `__unregister_cpu_notifier'
>    arch/x86/oprofile/built-in.o: In function `nmi_shutdown':
>    nmi_int.c:(.text+0x2793): undefined reference to `__unregister_cpu_notifier'

Ohh, right. I have missed that unregister functions definitions are
guarded as well. This patch should hopefully be correct finally.
Please note it also exports register/unregister callbacks when
CONFIG_HOTPLUG_CPU is not defined which is not really needed strictly
speaking because those are only used when !MODULE but I would rather
not make the code more complicated. If maintainers prefer I can guard
exports separately of course.
---

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

Hi Michal,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc7 next-20161202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/hotplug-make-register-and-unregister-notifier-API-symmetric/20161203-114815
config: i386-randconfig-s1-201648 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   mm/built-in.o: In function `zs_unregister_cpu_notifier':
>> zsmalloc.c:(.text.unlikely+0xc7e): undefined reference to `__unregister_cpu_notifier'
   arch/x86/oprofile/built-in.o: In function `nmi_timer_shutdown':
   nmi_timer_int.c:(.text+0x1c7d): undefined reference to `__unregister_cpu_notifier'
   arch/x86/oprofile/built-in.o: In function `nmi_shutdown':
   nmi_int.c:(.text+0x2319): undefined reference to `__unregister_cpu_notifier'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21074 bytes --]

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[Yu Zhao]

On Fri, Dec 02, 2016 at 04:19:36PM +0100, Michal Hocko wrote:
> [Let's CC more people - the thread started
> http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com]
> 
> On Fri 02-12-16 09:56:26, Dan Streetman wrote:
> > On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> > >> 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?
> > >
> > > 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.
> 
> OK, this is still only compile tested. Yu Zhao, assuming you were able
> to trigger the original problem could you test with the below patch
> please?

This patch (plus the latest fix in this thread) solves the problem.

Just for the record, the problem is when CONFIG_HOTPLUG_CPU=n, changing
/sys/module/zswap/parameters/compressor multiple times will cause:

[  144.964346] BUG: unable to handle kernel paging request at ffff880658a2be78
[  144.971337] IP: [<ffffffffa290b00b>] raw_notifier_chain_register+0x1b/0x40
<snipped>
[  145.122628] Call Trace:
[  145.125086]  [<ffffffffa28e5cf8>] __register_cpu_notifier+0x18/0x20
[  145.131350]  [<ffffffffa2a5dd73>] zswap_pool_create+0x273/0x400
[  145.137268]  [<ffffffffa2a5e0fc>] __zswap_param_set+0x1fc/0x300
[  145.143188]  [<ffffffffa2944c1d>] ? trace_hardirqs_on+0xd/0x10
[  145.149018]  [<ffffffffa2908798>] ? kernel_param_lock+0x28/0x30
[  145.154940]  [<ffffffffa2a3e8cf>] ? __might_fault+0x4f/0xa0
[  145.160511]  [<ffffffffa2a5e237>] zswap_compressor_param_set+0x17/0x20
[  145.167035]  [<ffffffffa2908d3c>] param_attr_store+0x5c/0xb0
[  145.172694]  [<ffffffffa290848d>] module_attr_store+0x1d/0x30
[  145.178443]  [<ffffffffa2b2b41f>] sysfs_kf_write+0x4f/0x70
[  145.183925]  [<ffffffffa2b2a5b9>] kernfs_fop_write+0x149/0x180
[  145.189761]  [<ffffffffa2a99248>] __vfs_write+0x18/0x40
[  145.194982]  [<ffffffffa2a9a412>] vfs_write+0xb2/0x1a0
[  145.200122]  [<ffffffffa2a9a732>] SyS_write+0x52/0xa0
[  145.205177]  [<ffffffffa2ff4d97>] entry_SYSCALL_64_fastpath+0x12/0x17

> ---
> From c812fe4e519914aa37f092d3a0321038fadcdde7 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Fri, 2 Dec 2016 16:06:56 +0100
> Subject: [PATCH] hotplug: make register and unregister notifier API symmetric
> 
> Yu Zhao has noticed that __unregister_cpu_notifier only unregisters its
> notifiers when HOTPLUG_CPU=y while the registration might succeed even
> when HOTPLUG_CPU=n if MODULE is enabled. This means that e.g. zswap
> might keep a stale notifier on the list on the manual clean up during
> the pool tear down and thus corrupt the list. Fix this issue by making
> unregister APIs symmetric to the register so there are no surprises.
> 
> Fixes: 47e627bc8c9a ("[PATCH] hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU")
> Cc: stable # zswap needs it 4.3+
> Reported-by: Yu Zhao <yuzhao@google.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/cpu.h | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 797d9c8e9a1b..c8938eb21e34 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -105,22 +105,16 @@ extern bool cpuhp_tasks_frozen;
>  		{ .notifier_call = fn, .priority = pri };	\
>  	__register_cpu_notifier(&fn##_nb);			\
>  }
> -#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
> -#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> -#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> -#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
>  
> -#ifdef CONFIG_HOTPLUG_CPU
>  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);
>  extern void __unregister_cpu_notifier(struct notifier_block *nb);
> -#else
>  
> -#ifndef MODULE
> -extern int register_cpu_notifier(struct notifier_block *nb);
> -extern int __register_cpu_notifier(struct notifier_block *nb);
> -#else
> +#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
> +#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> +#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> +
>  static inline int register_cpu_notifier(struct notifier_block *nb)
>  {
>  	return 0;
> @@ -130,7 +124,6 @@ static inline int __register_cpu_notifier(struct notifier_block *nb)
>  {
>  	return 0;
>  }
> -#endif
>  
>  static inline void unregister_cpu_notifier(struct notifier_block *nb)
>  {
> -- 
> 2.10.2
> 
> -- 
> Michal Hocko
> SUSE Labs

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[Yu Zhao]

On Fri, Dec 02, 2016 at 04:19:36PM +0100, Michal Hocko wrote:
> [Let's CC more people - the thread started
> http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com]
> 
> On Fri 02-12-16 09:56:26, Dan Streetman wrote:
> > On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> > >> 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?
> > >
> > > 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.
> 
> OK, this is still only compile tested. Yu Zhao, assuming you were able
> to trigger the original problem could you test with the below patch
> please?

This patch (plus the latest fix in this thread) solves the problem.

Just for the record, the problem is when CONFIG_HOTPLUG_CPU=n, changing
/sys/module/zswap/parameters/compressor multiple times will cause:

[  144.964346] BUG: unable to handle kernel paging request at ffff880658a2be78
[  144.971337] IP: [<ffffffffa290b00b>] raw_notifier_chain_register+0x1b/0x40
<snipped>
[  145.122628] Call Trace:
[  145.125086]  [<ffffffffa28e5cf8>] __register_cpu_notifier+0x18/0x20
[  145.131350]  [<ffffffffa2a5dd73>] zswap_pool_create+0x273/0x400
[  145.137268]  [<ffffffffa2a5e0fc>] __zswap_param_set+0x1fc/0x300
[  145.143188]  [<ffffffffa2944c1d>] ? trace_hardirqs_on+0xd/0x10
[  145.149018]  [<ffffffffa2908798>] ? kernel_param_lock+0x28/0x30
[  145.154940]  [<ffffffffa2a3e8cf>] ? __might_fault+0x4f/0xa0
[  145.160511]  [<ffffffffa2a5e237>] zswap_compressor_param_set+0x17/0x20
[  145.167035]  [<ffffffffa2908d3c>] param_attr_store+0x5c/0xb0
[  145.172694]  [<ffffffffa290848d>] module_attr_store+0x1d/0x30
[  145.178443]  [<ffffffffa2b2b41f>] sysfs_kf_write+0x4f/0x70
[  145.183925]  [<ffffffffa2b2a5b9>] kernfs_fop_write+0x149/0x180
[  145.189761]  [<ffffffffa2a99248>] __vfs_write+0x18/0x40
[  145.194982]  [<ffffffffa2a9a412>] vfs_write+0xb2/0x1a0
[  145.200122]  [<ffffffffa2a9a732>] SyS_write+0x52/0xa0
[  145.205177]  [<ffffffffa2ff4d97>] entry_SYSCALL_64_fastpath+0x12/0x17

> ---
> From c812fe4e519914aa37f092d3a0321038fadcdde7 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Fri, 2 Dec 2016 16:06:56 +0100
> Subject: [PATCH] hotplug: make register and unregister notifier API symmetric
> 
> Yu Zhao has noticed that __unregister_cpu_notifier only unregisters its
> notifiers when HOTPLUG_CPU=y while the registration might succeed even
> when HOTPLUG_CPU=n if MODULE is enabled. This means that e.g. zswap
> might keep a stale notifier on the list on the manual clean up during
> the pool tear down and thus corrupt the list. Fix this issue by making
> unregister APIs symmetric to the register so there are no surprises.
> 
> Fixes: 47e627bc8c9a ("[PATCH] hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU")
> Cc: stable # zswap needs it 4.3+
> Reported-by: Yu Zhao <yuzhao@google.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/cpu.h | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 797d9c8e9a1b..c8938eb21e34 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -105,22 +105,16 @@ extern bool cpuhp_tasks_frozen;
>  		{ .notifier_call = fn, .priority = pri };	\
>  	__register_cpu_notifier(&fn##_nb);			\
>  }
> -#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
> -#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> -#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> -#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
>  
> -#ifdef CONFIG_HOTPLUG_CPU
>  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);
>  extern void __unregister_cpu_notifier(struct notifier_block *nb);
> -#else
>  
> -#ifndef MODULE
> -extern int register_cpu_notifier(struct notifier_block *nb);
> -extern int __register_cpu_notifier(struct notifier_block *nb);
> -#else
> +#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
> +#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> +#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
> +
>  static inline int register_cpu_notifier(struct notifier_block *nb)
>  {
>  	return 0;
> @@ -130,7 +124,6 @@ static inline int __register_cpu_notifier(struct notifier_block *nb)
>  {
>  	return 0;
>  }
> -#endif
>  
>  static inline void unregister_cpu_notifier(struct notifier_block *nb)
>  {
> -- 
> 2.10.2
> 
> -- 
> 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>

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

On Mon 05-12-16 12:59:02, Yu Zhao wrote:
> On Fri, Dec 02, 2016 at 04:19:36PM +0100, Michal Hocko wrote:
> > [Let's CC more people - the thread started
> > http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com]
> > 
> > On Fri 02-12-16 09:56:26, Dan Streetman wrote:
> > > On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > > On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> > > >> 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?
> > > >
> > > > 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.
> > 
> > OK, this is still only compile tested. Yu Zhao, assuming you were able
> > to trigger the original problem could you test with the below patch
> > please?
> 
> This patch (plus the latest fix in this thread) solves the problem.
> 
> Just for the record, the problem is when CONFIG_HOTPLUG_CPU=n, changing
> /sys/module/zswap/parameters/compressor multiple times will cause:
> 
> [  144.964346] BUG: unable to handle kernel paging request at ffff880658a2be78
> [  144.971337] IP: [<ffffffffa290b00b>] raw_notifier_chain_register+0x1b/0x40
> <snipped>
> [  145.122628] Call Trace:
> [  145.125086]  [<ffffffffa28e5cf8>] __register_cpu_notifier+0x18/0x20
> [  145.131350]  [<ffffffffa2a5dd73>] zswap_pool_create+0x273/0x400
> [  145.137268]  [<ffffffffa2a5e0fc>] __zswap_param_set+0x1fc/0x300
> [  145.143188]  [<ffffffffa2944c1d>] ? trace_hardirqs_on+0xd/0x10
> [  145.149018]  [<ffffffffa2908798>] ? kernel_param_lock+0x28/0x30
> [  145.154940]  [<ffffffffa2a3e8cf>] ? __might_fault+0x4f/0xa0
> [  145.160511]  [<ffffffffa2a5e237>] zswap_compressor_param_set+0x17/0x20
> [  145.167035]  [<ffffffffa2908d3c>] param_attr_store+0x5c/0xb0
> [  145.172694]  [<ffffffffa290848d>] module_attr_store+0x1d/0x30
> [  145.178443]  [<ffffffffa2b2b41f>] sysfs_kf_write+0x4f/0x70
> [  145.183925]  [<ffffffffa2b2a5b9>] kernfs_fop_write+0x149/0x180
> [  145.189761]  [<ffffffffa2a99248>] __vfs_write+0x18/0x40
> [  145.194982]  [<ffffffffa2a9a412>] vfs_write+0xb2/0x1a0
> [  145.200122]  [<ffffffffa2a9a732>] SyS_write+0x52/0xa0
> [  145.205177]  [<ffffffffa2ff4d97>] entry_SYSCALL_64_fastpath+0x12/0x17

Thanks for this additional information which I have added to the
changelog. I have also added your Tested-by unless you have any
objections and will repost soon.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

On Mon 05-12-16 12:59:02, Yu Zhao wrote:
> On Fri, Dec 02, 2016 at 04:19:36PM +0100, Michal Hocko wrote:
> > [Let's CC more people - the thread started
> > http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com]
> > 
> > On Fri 02-12-16 09:56:26, Dan Streetman wrote:
> > > On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > > On Fri 02-12-16 15:38:48, Michal Hocko wrote:
> > > >> 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?
> > > >
> > > > 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.
> > 
> > OK, this is still only compile tested. Yu Zhao, assuming you were able
> > to trigger the original problem could you test with the below patch
> > please?
> 
> This patch (plus the latest fix in this thread) solves the problem.
> 
> Just for the record, the problem is when CONFIG_HOTPLUG_CPU=n, changing
> /sys/module/zswap/parameters/compressor multiple times will cause:
> 
> [  144.964346] BUG: unable to handle kernel paging request at ffff880658a2be78
> [  144.971337] IP: [<ffffffffa290b00b>] raw_notifier_chain_register+0x1b/0x40
> <snipped>
> [  145.122628] Call Trace:
> [  145.125086]  [<ffffffffa28e5cf8>] __register_cpu_notifier+0x18/0x20
> [  145.131350]  [<ffffffffa2a5dd73>] zswap_pool_create+0x273/0x400
> [  145.137268]  [<ffffffffa2a5e0fc>] __zswap_param_set+0x1fc/0x300
> [  145.143188]  [<ffffffffa2944c1d>] ? trace_hardirqs_on+0xd/0x10
> [  145.149018]  [<ffffffffa2908798>] ? kernel_param_lock+0x28/0x30
> [  145.154940]  [<ffffffffa2a3e8cf>] ? __might_fault+0x4f/0xa0
> [  145.160511]  [<ffffffffa2a5e237>] zswap_compressor_param_set+0x17/0x20
> [  145.167035]  [<ffffffffa2908d3c>] param_attr_store+0x5c/0xb0
> [  145.172694]  [<ffffffffa290848d>] module_attr_store+0x1d/0x30
> [  145.178443]  [<ffffffffa2b2b41f>] sysfs_kf_write+0x4f/0x70
> [  145.183925]  [<ffffffffa2b2a5b9>] kernfs_fop_write+0x149/0x180
> [  145.189761]  [<ffffffffa2a99248>] __vfs_write+0x18/0x40
> [  145.194982]  [<ffffffffa2a9a412>] vfs_write+0xb2/0x1a0
> [  145.200122]  [<ffffffffa2a9a732>] SyS_write+0x52/0xa0
> [  145.205177]  [<ffffffffa2ff4d97>] entry_SYSCALL_64_fastpath+0x12/0x17

Thanks for this additional information which I have added to the
changelog. I have also added your Tested-by unless you have any
objections and will repost soon.
-- 
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>

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

[Yu Zhao]

On Fri, Dec 02, 2016 at 02:46:06PM +0100, Michal Hocko 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?

Well, I'm not a fan of ifdef and I don't like the unnecessary memory
usage (notifier_block) and lock (cpu_notifier_register_begin/done)
either.

Just pointing this out, having no problem living with your hotplug
fixes.

> 
> > 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

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

[Yu Zhao]

On Fri, Dec 02, 2016 at 02:46:06PM +0100, Michal Hocko 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?

Well, I'm not a fan of ifdef and I don't like the unnecessary memory
usage (notifier_block) and lock (cpu_notifier_register_begin/done)
either.

Just pointing this out, having no problem living with your hotplug
fixes.

> 
> > 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

--
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>

[PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

Yu Zhao has noticed that __unregister_cpu_notifier only unregisters its
notifiers when HOTPLUG_CPU=y while the registration might succeed even
when HOTPLUG_CPU=n if MODULE is enabled. This means that e.g. zswap
might keep a stale notifier on the list on the manual clean up during
the pool tear down and thus corrupt the list. Resulting in the following

[  144.964346] BUG: unable to handle kernel paging request at ffff880658a2be78
[  144.971337] IP: [<ffffffffa290b00b>] raw_notifier_chain_register+0x1b/0x40
<snipped>
[  145.122628] Call Trace:
[  145.125086]  [<ffffffffa28e5cf8>] __register_cpu_notifier+0x18/0x20
[  145.131350]  [<ffffffffa2a5dd73>] zswap_pool_create+0x273/0x400
[  145.137268]  [<ffffffffa2a5e0fc>] __zswap_param_set+0x1fc/0x300
[  145.143188]  [<ffffffffa2944c1d>] ? trace_hardirqs_on+0xd/0x10
[  145.149018]  [<ffffffffa2908798>] ? kernel_param_lock+0x28/0x30
[  145.154940]  [<ffffffffa2a3e8cf>] ? __might_fault+0x4f/0xa0
[  145.160511]  [<ffffffffa2a5e237>] zswap_compressor_param_set+0x17/0x20
[  145.167035]  [<ffffffffa2908d3c>] param_attr_store+0x5c/0xb0
[  145.172694]  [<ffffffffa290848d>] module_attr_store+0x1d/0x30
[  145.178443]  [<ffffffffa2b2b41f>] sysfs_kf_write+0x4f/0x70
[  145.183925]  [<ffffffffa2b2a5b9>] kernfs_fop_write+0x149/0x180
[  145.189761]  [<ffffffffa2a99248>] __vfs_write+0x18/0x40
[  145.194982]  [<ffffffffa2a9a412>] vfs_write+0xb2/0x1a0
[  145.200122]  [<ffffffffa2a9a732>] SyS_write+0x52/0xa0
[  145.205177]  [<ffffffffa2ff4d97>] entry_SYSCALL_64_fastpath+0x12/0x17

This can be even triggered manually by changing
/sys/module/zswap/parameters/compressor multiple times.

Fix this issue by making unregister APIs symmetric to the register so
there are no surprises.

Fixes: 47e627bc8c9a ("[PATCH] hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU")
Cc: stable # zswap requires for 4.3+
Reported-and-tested-by: Yu Zhao <yuzhao@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Hi,
the previous version of the patch has been sent [1] and the reporter has
confirmed it works as expected. I have also added more information to
the changelog. Does this looks sensible?

[1] http://lkml.kernel.org/r/20161205060840.GC30758@dhcp22.suse.cz

 include/linux/cpu.h | 15 ++++-----------
 kernel/cpu.c        |  2 +-
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 797d9c8e9a1b..c8938eb21e34 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -105,22 +105,16 @@ extern bool cpuhp_tasks_frozen;
 		{ .notifier_call = fn, .priority = pri };	\
 	__register_cpu_notifier(&fn##_nb);			\
 }
-#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
-#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
 
-#ifdef CONFIG_HOTPLUG_CPU
 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);
 extern void __unregister_cpu_notifier(struct notifier_block *nb);
-#else
 
-#ifndef MODULE
-extern int register_cpu_notifier(struct notifier_block *nb);
-extern int __register_cpu_notifier(struct notifier_block *nb);
-#else
+#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
+#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+
 static inline int register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -130,7 +124,6 @@ static inline int __register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
 }
-#endif
 
 static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 341bf80f80bd..73fb59fda809 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -578,7 +578,6 @@ void __init cpuhp_threads_init(void)
 	kthread_unpark(this_cpu_read(cpuhp_state.thread));
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
 EXPORT_SYMBOL(register_cpu_notifier);
 EXPORT_SYMBOL(__register_cpu_notifier);
 void unregister_cpu_notifier(struct notifier_block *nb)
@@ -595,6 +594,7 @@ void __unregister_cpu_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(__unregister_cpu_notifier);
 
+#ifdef CONFIG_HOTPLUG_CPU
 /**
  * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
  * @cpu: a CPU id
-- 
2.10.2

[PATCH] hotplug: make register and unregister notifier API symmetric

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

Yu Zhao has noticed that __unregister_cpu_notifier only unregisters its
notifiers when HOTPLUG_CPU=y while the registration might succeed even
when HOTPLUG_CPU=n if MODULE is enabled. This means that e.g. zswap
might keep a stale notifier on the list on the manual clean up during
the pool tear down and thus corrupt the list. Resulting in the following

[  144.964346] BUG: unable to handle kernel paging request at ffff880658a2be78
[  144.971337] IP: [<ffffffffa290b00b>] raw_notifier_chain_register+0x1b/0x40
<snipped>
[  145.122628] Call Trace:
[  145.125086]  [<ffffffffa28e5cf8>] __register_cpu_notifier+0x18/0x20
[  145.131350]  [<ffffffffa2a5dd73>] zswap_pool_create+0x273/0x400
[  145.137268]  [<ffffffffa2a5e0fc>] __zswap_param_set+0x1fc/0x300
[  145.143188]  [<ffffffffa2944c1d>] ? trace_hardirqs_on+0xd/0x10
[  145.149018]  [<ffffffffa2908798>] ? kernel_param_lock+0x28/0x30
[  145.154940]  [<ffffffffa2a3e8cf>] ? __might_fault+0x4f/0xa0
[  145.160511]  [<ffffffffa2a5e237>] zswap_compressor_param_set+0x17/0x20
[  145.167035]  [<ffffffffa2908d3c>] param_attr_store+0x5c/0xb0
[  145.172694]  [<ffffffffa290848d>] module_attr_store+0x1d/0x30
[  145.178443]  [<ffffffffa2b2b41f>] sysfs_kf_write+0x4f/0x70
[  145.183925]  [<ffffffffa2b2a5b9>] kernfs_fop_write+0x149/0x180
[  145.189761]  [<ffffffffa2a99248>] __vfs_write+0x18/0x40
[  145.194982]  [<ffffffffa2a9a412>] vfs_write+0xb2/0x1a0
[  145.200122]  [<ffffffffa2a9a732>] SyS_write+0x52/0xa0
[  145.205177]  [<ffffffffa2ff4d97>] entry_SYSCALL_64_fastpath+0x12/0x17

This can be even triggered manually by changing
/sys/module/zswap/parameters/compressor multiple times.

Fix this issue by making unregister APIs symmetric to the register so
there are no surprises.

Fixes: 47e627bc8c9a ("[PATCH] hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU")
Cc: stable # zswap requires for 4.3+
Reported-and-tested-by: Yu Zhao <yuzhao@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Hi,
the previous version of the patch has been sent [1] and the reporter has
confirmed it works as expected. I have also added more information to
the changelog. Does this looks sensible?

[1] http://lkml.kernel.org/r/20161205060840.GC30758@dhcp22.suse.cz

 include/linux/cpu.h | 15 ++++-----------
 kernel/cpu.c        |  2 +-
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 797d9c8e9a1b..c8938eb21e34 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -105,22 +105,16 @@ extern bool cpuhp_tasks_frozen;
 		{ .notifier_call = fn, .priority = pri };	\
 	__register_cpu_notifier(&fn##_nb);			\
 }
-#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
-#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
-#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
 
-#ifdef CONFIG_HOTPLUG_CPU
 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);
 extern void __unregister_cpu_notifier(struct notifier_block *nb);
-#else
 
-#ifndef MODULE
-extern int register_cpu_notifier(struct notifier_block *nb);
-extern int __register_cpu_notifier(struct notifier_block *nb);
-#else
+#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */
+#define cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+#define __cpu_notifier(fn, pri)	do { (void)(fn); } while (0)
+
 static inline int register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -130,7 +124,6 @@ static inline int __register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
 }
-#endif
 
 static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 341bf80f80bd..73fb59fda809 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -578,7 +578,6 @@ void __init cpuhp_threads_init(void)
 	kthread_unpark(this_cpu_read(cpuhp_state.thread));
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
 EXPORT_SYMBOL(register_cpu_notifier);
 EXPORT_SYMBOL(__register_cpu_notifier);
 void unregister_cpu_notifier(struct notifier_block *nb)
@@ -595,6 +594,7 @@ void __unregister_cpu_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(__unregister_cpu_notifier);
 
+#ifdef CONFIG_HOTPLUG_CPU
 /**
  * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
  * @cpu: a CPU id
-- 
2.10.2

--
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>