Now zsmalloc can be registered as a zpool driver into zpool when
CONFIG_ZPOOL is enabled. During the init of zsmalloc, when error happens,
we need to do cleanup. But in current code, it will unregister a not yet
registered zsmalloc zpool driver(*zs_zpool_driver*).
This patch puts the cleanup in zs_init() instead of calling zs_exit()
where it will unregister a not-registered zpool driver.
Signed-off-by: Mahendran Ganesh <[email protected]>
---
mm/zsmalloc.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 839a48c..3d2bb36 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -907,10 +907,8 @@ static int zs_init(void)
__register_cpu_notifier(&zs_cpu_nb);
for_each_online_cpu(cpu) {
ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
- if (notifier_to_errno(ret)) {
- cpu_notifier_register_done();
+ if (notifier_to_errno(ret))
goto fail;
- }
}
cpu_notifier_register_done();
@@ -920,8 +918,14 @@ static int zs_init(void)
#endif
return 0;
+
fail:
- zs_exit();
+ for_each_online_cpu(cpu)
+ zs_cpu_notifier(NULL, CPU_UP_CANCELED, (void *)(long)cpu);
+ __unregister_cpu_notifier(&zs_cpu_nb);
+
+ cpu_notifier_register_done();
+
return notifier_to_errno(ret);
}
--
1.7.9.5
After patch [1], the zs_exit is only called in module exit.
So add __init/__exit to zs_init/zs_exit.
[1] mm/zsmalloc: avoid unregister a NOT-registered zsmalloc zpool driver
Signed-off-by: Mahendran Ganesh <[email protected]>
---
mm/zsmalloc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 3d2bb36..92af030 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -881,7 +881,7 @@ static struct notifier_block zs_cpu_nb = {
.notifier_call = zs_cpu_notifier
};
-static void zs_exit(void)
+static void __exit zs_exit(void)
{
int cpu;
@@ -898,7 +898,7 @@ static void zs_exit(void)
cpu_notifier_register_done();
}
-static int zs_init(void)
+static int __init zs_init(void)
{
int cpu, ret;
--
1.7.9.5
In previous code design, the zs_exit() will be called by zs_init().
So function zs_exit() is located before zs_init(). And after patch [1],
the zs_exit() will not be called by zs_init().
So we can move the zs_exit() after zs_init() and put these two module
init/exit functions to the end of the file which is the common style.
[1] mm/zsmalloc: avoid unregister a NOT-registered zsmalloc zpool driver
Signed-off-by: Mahendran Ganesh <[email protected]>
---
mm/zsmalloc.c | 96 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 92af030..4fcb7c9 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -881,54 +881,6 @@ static struct notifier_block zs_cpu_nb = {
.notifier_call = zs_cpu_notifier
};
-static void __exit zs_exit(void)
-{
- int cpu;
-
-#ifdef CONFIG_ZPOOL
- zpool_unregister_driver(&zs_zpool_driver);
-#endif
-
- cpu_notifier_register_begin();
-
- for_each_online_cpu(cpu)
- zs_cpu_notifier(NULL, CPU_DEAD, (void *)(long)cpu);
- __unregister_cpu_notifier(&zs_cpu_nb);
-
- cpu_notifier_register_done();
-}
-
-static int __init zs_init(void)
-{
- int cpu, ret;
-
- cpu_notifier_register_begin();
-
- __register_cpu_notifier(&zs_cpu_nb);
- for_each_online_cpu(cpu) {
- ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
- if (notifier_to_errno(ret))
- goto fail;
- }
-
- cpu_notifier_register_done();
-
-#ifdef CONFIG_ZPOOL
- zpool_register_driver(&zs_zpool_driver);
-#endif
-
- return 0;
-
-fail:
- for_each_online_cpu(cpu)
- zs_cpu_notifier(NULL, CPU_UP_CANCELED, (void *)(long)cpu);
- __unregister_cpu_notifier(&zs_cpu_nb);
-
- cpu_notifier_register_done();
-
- return notifier_to_errno(ret);
-}
-
/**
* zs_create_pool - Creates an allocation pool to work from.
* @flags: allocation flags used to allocate pool metadata
@@ -1187,6 +1139,54 @@ unsigned long zs_get_total_pages(struct zs_pool *pool)
}
EXPORT_SYMBOL_GPL(zs_get_total_pages);
+static int __init zs_init(void)
+{
+ int cpu, ret;
+
+ cpu_notifier_register_begin();
+
+ __register_cpu_notifier(&zs_cpu_nb);
+ for_each_online_cpu(cpu) {
+ ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
+ if (notifier_to_errno(ret))
+ goto fail;
+ }
+
+ cpu_notifier_register_done();
+
+#ifdef CONFIG_ZPOOL
+ zpool_register_driver(&zs_zpool_driver);
+#endif
+
+ return 0;
+
+fail:
+ for_each_online_cpu(cpu)
+ zs_cpu_notifier(NULL, CPU_UP_CANCELED, (void *)(long)cpu);
+ __unregister_cpu_notifier(&zs_cpu_nb);
+
+ cpu_notifier_register_done();
+
+ return notifier_to_errno(ret);
+}
+
+static void __exit zs_exit(void)
+{
+ int cpu;
+
+#ifdef CONFIG_ZPOOL
+ zpool_unregister_driver(&zs_zpool_driver);
+#endif
+
+ cpu_notifier_register_begin();
+
+ for_each_online_cpu(cpu)
+ zs_cpu_notifier(NULL, CPU_DEAD, (void *)(long)cpu);
+ __unregister_cpu_notifier(&zs_cpu_nb);
+
+ cpu_notifier_register_done();
+}
+
module_init(zs_init);
module_exit(zs_exit);
--
1.7.9.5
On (11/13/14 21:37), Mahendran Ganesh wrote:
> Now zsmalloc can be registered as a zpool driver into zpool when
> CONFIG_ZPOOL is enabled. During the init of zsmalloc, when error happens,
> we need to do cleanup. But in current code, it will unregister a not yet
> registered zsmalloc zpool driver(*zs_zpool_driver*).
>
> This patch puts the cleanup in zs_init() instead of calling zs_exit()
> where it will unregister a not-registered zpool driver.
>
> Signed-off-by: Mahendran Ganesh <[email protected]>
> ---
> mm/zsmalloc.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 839a48c..3d2bb36 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -907,10 +907,8 @@ static int zs_init(void)
> __register_cpu_notifier(&zs_cpu_nb);
> for_each_online_cpu(cpu) {
> ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
> - if (notifier_to_errno(ret)) {
> - cpu_notifier_register_done();
> + if (notifier_to_errno(ret))
> goto fail;
> - }
> }
>
> cpu_notifier_register_done();
> @@ -920,8 +918,14 @@ static int zs_init(void)
> #endif
>
> return 0;
> +
> fail:
> - zs_exit();
> + for_each_online_cpu(cpu)
> + zs_cpu_notifier(NULL, CPU_UP_CANCELED, (void *)(long)cpu);
> + __unregister_cpu_notifier(&zs_cpu_nb);
> +
> + cpu_notifier_register_done();
> +
> return notifier_to_errno(ret);
so we duplicate same code, and there is a bit confusing part
now: zs_cpu_notifier(CPU_UP_CANCELED) and zs_cpu_notifier(CPU_DEAD)
calls.
how about something like this?
Factor out zsmalloc cpu notifier unregistration code and call
it from both zs_exit() and zs_init() error path.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
mm/zsmalloc.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index b3b57ef..c4d2d60 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -881,14 +881,10 @@ static struct notifier_block zs_cpu_nb = {
.notifier_call = zs_cpu_notifier
};
-static void zs_exit(void)
+static inline void zs_unregister_cpu_notifier()
{
int cpu;
-#ifdef CONFIG_ZPOOL
- zpool_unregister_driver(&zs_zpool_driver);
-#endif
-
cpu_notifier_register_begin();
for_each_online_cpu(cpu)
@@ -898,6 +894,14 @@ static void zs_exit(void)
cpu_notifier_register_done();
}
+static void zs_exit(void)
+{
+#ifdef CONFIG_ZPOOL
+ zpool_unregister_driver(&zs_zpool_driver);
+#endif
+ zs_unregister_cpu_notifier();
+}
+
static int zs_init(void)
{
int cpu, ret;
@@ -907,10 +911,8 @@ static int zs_init(void)
__register_cpu_notifier(&zs_cpu_nb);
for_each_online_cpu(cpu) {
ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
- if (notifier_to_errno(ret)) {
- cpu_notifier_register_done();
+ if (notifier_to_errno(ret))
goto fail;
- }
}
cpu_notifier_register_done();
@@ -921,7 +923,7 @@ static int zs_init(void)
return 0;
fail:
- zs_exit();
+ zs_unregister_cpu_notifier();
return notifier_to_errno(ret);
}
On (11/14/14 00:22), Sergey Senozhatsky wrote:
> > Now zsmalloc can be registered as a zpool driver into zpool when
> > CONFIG_ZPOOL is enabled. During the init of zsmalloc, when error happens,
> > we need to do cleanup. But in current code, it will unregister a not yet
> > registered zsmalloc zpool driver(*zs_zpool_driver*).
> >
> > This patch puts the cleanup in zs_init() instead of calling zs_exit()
> > where it will unregister a not-registered zpool driver.
> >
> > Signed-off-by: Mahendran Ganesh <[email protected]>
> > ---
> > mm/zsmalloc.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 839a48c..3d2bb36 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -907,10 +907,8 @@ static int zs_init(void)
> > __register_cpu_notifier(&zs_cpu_nb);
> > for_each_online_cpu(cpu) {
> > ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
> > - if (notifier_to_errno(ret)) {
> > - cpu_notifier_register_done();
> > + if (notifier_to_errno(ret))
> > goto fail;
> > - }
> > }
> >
> > cpu_notifier_register_done();
> > @@ -920,8 +918,14 @@ static int zs_init(void)
> > #endif
> >
> > return 0;
> > +
> > fail:
> > - zs_exit();
> > + for_each_online_cpu(cpu)
> > + zs_cpu_notifier(NULL, CPU_UP_CANCELED, (void *)(long)cpu);
> > + __unregister_cpu_notifier(&zs_cpu_nb);
> > +
> > + cpu_notifier_register_done();
> > +
> > return notifier_to_errno(ret);
>
> so we duplicate same code, and there is a bit confusing part
> now: zs_cpu_notifier(CPU_UP_CANCELED) and zs_cpu_notifier(CPU_DEAD)
> calls.
>
>
> how about something like this?
arghhh... silly me. sorry! this wasn't even compile tested.
it should be zs_unregister_cpu_notifier(void). here it is.
Factor out zsmalloc cpu notifier unregistration code and call
it from both zs_exit() and zs_init() error path.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
mm/zsmalloc.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index b3b57ef..8dda7c7 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -881,14 +881,10 @@ static struct notifier_block zs_cpu_nb = {
.notifier_call = zs_cpu_notifier
};
-static void zs_exit(void)
+static void zs_unregister_cpu_notifier(void)
{
int cpu;
-#ifdef CONFIG_ZPOOL
- zpool_unregister_driver(&zs_zpool_driver);
-#endif
-
cpu_notifier_register_begin();
for_each_online_cpu(cpu)
@@ -898,6 +894,14 @@ static void zs_exit(void)
cpu_notifier_register_done();
}
+static void zs_exit(void)
+{
+#ifdef CONFIG_ZPOOL
+ zpool_unregister_driver(&zs_zpool_driver);
+#endif
+ zs_unregister_cpu_notifier();
+}
+
static int zs_init(void)
{
int cpu, ret;
@@ -907,10 +911,8 @@ static int zs_init(void)
__register_cpu_notifier(&zs_cpu_nb);
for_each_online_cpu(cpu) {
ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
- if (notifier_to_errno(ret)) {
- cpu_notifier_register_done();
+ if (notifier_to_errno(ret))
goto fail;
- }
}
cpu_notifier_register_done();
@@ -921,7 +923,7 @@ static int zs_init(void)
return 0;
fail:
- zs_exit();
+ zs_unregister_cpu_notifier();
return notifier_to_errno(ret);
}
On (11/14/14 00:30), Sergey Senozhatsky wrote:
> Factor out zsmalloc cpu notifier unregistration code and call
> it from both zs_exit() and zs_init() error path.
I should had have a good sleep before posting this.
shame on me!
v3
Signed-ogg-by: Sergey Senozhatsky <[email protected]>
---
mm/zsmalloc.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index b3b57ef..cd4efa1 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -881,14 +881,10 @@ static struct notifier_block zs_cpu_nb = {
.notifier_call = zs_cpu_notifier
};
-static void zs_exit(void)
+static void zs_unregister_cpu_notifier(void)
{
int cpu;
-#ifdef CONFIG_ZPOOL
- zpool_unregister_driver(&zs_zpool_driver);
-#endif
-
cpu_notifier_register_begin();
for_each_online_cpu(cpu)
@@ -898,6 +894,14 @@ static void zs_exit(void)
cpu_notifier_register_done();
}
+static void zs_exit(void)
+{
+#ifdef CONFIG_ZPOOL
+ zpool_unregister_driver(&zs_zpool_driver);
+#endif
+ zs_unregister_cpu_notifier();
+}
+
static int zs_init(void)
{
int cpu, ret;
@@ -921,7 +925,7 @@ static int zs_init(void)
return 0;
fail:
- zs_exit();
+ zs_unregister_cpu_notifier();
return notifier_to_errno(ret);
}
Hello Sergey,
On Fri, Nov 14, 2014 at 07:31:27AM +0900, Sergey Senozhatsky wrote:
> On (11/14/14 00:30), Sergey Senozhatsky wrote:
> > Factor out zsmalloc cpu notifier unregistration code and call
> > it from both zs_exit() and zs_init() error path.
>
> I should had have a good sleep before posting this.
> shame on me!
>
> v3
>
> Signed-ogg-by: Sergey Senozhatsky <[email protected]>
Looks good to me. a nitpick.
Could you factor out cpu register part as well as cpu unregister, too?
Then, please resend it with formal description and includes reported-by.
Thanks.
>
> ---
>
> mm/zsmalloc.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index b3b57ef..cd4efa1 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -881,14 +881,10 @@ static struct notifier_block zs_cpu_nb = {
> .notifier_call = zs_cpu_notifier
> };
>
> -static void zs_exit(void)
> +static void zs_unregister_cpu_notifier(void)
> {
> int cpu;
>
> -#ifdef CONFIG_ZPOOL
> - zpool_unregister_driver(&zs_zpool_driver);
> -#endif
> -
> cpu_notifier_register_begin();
>
> for_each_online_cpu(cpu)
> @@ -898,6 +894,14 @@ static void zs_exit(void)
> cpu_notifier_register_done();
> }
>
> +static void zs_exit(void)
> +{
> +#ifdef CONFIG_ZPOOL
> + zpool_unregister_driver(&zs_zpool_driver);
> +#endif
> + zs_unregister_cpu_notifier();
> +}
> +
> static int zs_init(void)
> {
> int cpu, ret;
> @@ -921,7 +925,7 @@ static int zs_init(void)
>
> return 0;
> fail:
> - zs_exit();
> + zs_unregister_cpu_notifier();
> return notifier_to_errno(ret);
> }
>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
--
Kind regards,
Minchan Kim
Hello
2014-11-13 23:22 GMT+08:00 Sergey Senozhatsky <[email protected]>:
> On (11/13/14 21:37), Mahendran Ganesh wrote:
>> Now zsmalloc can be registered as a zpool driver into zpool when
>> CONFIG_ZPOOL is enabled. During the init of zsmalloc, when error happens,
>> we need to do cleanup. But in current code, it will unregister a not yet
>> registered zsmalloc zpool driver(*zs_zpool_driver*).
>>
>> This patch puts the cleanup in zs_init() instead of calling zs_exit()
>> where it will unregister a not-registered zpool driver.
>>
>> Signed-off-by: Mahendran Ganesh <[email protected]>
>> ---
>> mm/zsmalloc.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
>> index 839a48c..3d2bb36 100644
>> --- a/mm/zsmalloc.c
>> +++ b/mm/zsmalloc.c
>> @@ -907,10 +907,8 @@ static int zs_init(void)
>> __register_cpu_notifier(&zs_cpu_nb);
>> for_each_online_cpu(cpu) {
>> ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
>> - if (notifier_to_errno(ret)) {
>> - cpu_notifier_register_done();
>> + if (notifier_to_errno(ret))
>> goto fail;
>> - }
>> }
>>
>> cpu_notifier_register_done();
>> @@ -920,8 +918,14 @@ static int zs_init(void)
>> #endif
>>
>> return 0;
>> +
>> fail:
>> - zs_exit();
>> + for_each_online_cpu(cpu)
>> + zs_cpu_notifier(NULL, CPU_UP_CANCELED, (void *)(long)cpu);
>> + __unregister_cpu_notifier(&zs_cpu_nb);
>> +
>> + cpu_notifier_register_done();
>> +
>> return notifier_to_errno(ret);
>
> so we duplicate same code, and there is a bit confusing part
> now: zs_cpu_notifier(CPU_UP_CANCELED) and zs_cpu_notifier(CPU_DEAD)
> calls.
Thanks for your review.
CPU_UP_CANCELED and CPU_DEAD have different scenario.
CPU_UP_CANCELED is better for fail to complete initialization. While
CPU_DEAD is better for exit(such as removing module)
Thanks.
>
> how about something like this?
>
> Factor out zsmalloc cpu notifier unregistration code and call
> it from both zs_exit() and zs_init() error path.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
>
> ---
>
> mm/zsmalloc.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index b3b57ef..c4d2d60 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -881,14 +881,10 @@ static struct notifier_block zs_cpu_nb = {
> .notifier_call = zs_cpu_notifier
> };
>
> -static void zs_exit(void)
> +static inline void zs_unregister_cpu_notifier()
> {
> int cpu;
>
> -#ifdef CONFIG_ZPOOL
> - zpool_unregister_driver(&zs_zpool_driver);
> -#endif
> -
> cpu_notifier_register_begin();
>
> for_each_online_cpu(cpu)
> @@ -898,6 +894,14 @@ static void zs_exit(void)
> cpu_notifier_register_done();
> }
>
> +static void zs_exit(void)
> +{
> +#ifdef CONFIG_ZPOOL
> + zpool_unregister_driver(&zs_zpool_driver);
> +#endif
> + zs_unregister_cpu_notifier();
> +}
> +
> static int zs_init(void)
> {
> int cpu, ret;
> @@ -907,10 +911,8 @@ static int zs_init(void)
> __register_cpu_notifier(&zs_cpu_nb);
> for_each_online_cpu(cpu) {
> ret = zs_cpu_notifier(NULL, CPU_UP_PREPARE, (void *)(long)cpu);
> - if (notifier_to_errno(ret)) {
> - cpu_notifier_register_done();
> + if (notifier_to_errno(ret))
> goto fail;
> - }
> }
>
> cpu_notifier_register_done();
> @@ -921,7 +923,7 @@ static int zs_init(void)
>
> return 0;
> fail:
> - zs_exit();
> + zs_unregister_cpu_notifier();
> return notifier_to_errno(ret);
> }
>
On (11/13/14 21:37), Mahendran Ganesh wrote:
> After patch [1], the zs_exit is only called in module exit.
> So add __init/__exit to zs_init/zs_exit.
>
> [1] mm/zsmalloc: avoid unregister a NOT-registered zsmalloc zpool driver
>
> Signed-off-by: Mahendran Ganesh <[email protected]>
makes sense.
-ss
> ---
> mm/zsmalloc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 3d2bb36..92af030 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -881,7 +881,7 @@ static struct notifier_block zs_cpu_nb = {
> .notifier_call = zs_cpu_notifier
> };
>
> -static void zs_exit(void)
> +static void __exit zs_exit(void)
> {
> int cpu;
>
> @@ -898,7 +898,7 @@ static void zs_exit(void)
> cpu_notifier_register_done();
> }
>
> -static int zs_init(void)
> +static int __init zs_init(void)
> {
> int cpu, ret;
>
> --
> 1.7.9.5
>