[PATCH] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[PATCH] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Sergey Senozhatsky]

We don't let user to disable shrinker in zsmalloc (once
it's been enabled), so no need to check ->shrinker_enabled
in zs_shrinker_count(), at the moment at least.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 mm/zsmalloc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 7ad5e54..8ba247d 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1822,9 +1822,6 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
 	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
 			shrinker);
 
-	if (!pool->shrinker_enabled)
-		return 0;
-
 	for (i = zs_size_classes - 1; i >= 0; i--) {
 		class = pool->size_class[i];
 		if (!class)
-- 
2.6.1.134.g4b1fd35

[PATCH] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Sergey Senozhatsky]

We don't let user to disable shrinker in zsmalloc (once
it's been enabled), so no need to check ->shrinker_enabled
in zs_shrinker_count(), at the moment at least.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 mm/zsmalloc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 7ad5e54..8ba247d 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1822,9 +1822,6 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
 	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
 			shrinker);
 
-	if (!pool->shrinker_enabled)
-		return 0;
-
 	for (i = zs_size_classes - 1; i >= 0; i--) {
 		class = pool->size_class[i];
 		if (!class)
-- 
2.6.1.134.g4b1fd35

--
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] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Minchan Kim]

On Wed, Oct 14, 2015 at 10:57:59AM +0900, Sergey Senozhatsky wrote:
> We don't let user to disable shrinker in zsmalloc (once
> it's been enabled), so no need to check ->shrinker_enabled
> in zs_shrinker_count(), at the moment at least.

I'm in favor of removing shrinker disable feature with this patch(
although we didn't implement it yet) because if there is some problem
of compaction, we should reveal and fix it without hiding with the
feature.

One thing I want is if we decide it, let's remove all things
about shrinker_enabled(ie, variable).
If we might need it later, we could introduce it easily.

Thanks.

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  mm/zsmalloc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 7ad5e54..8ba247d 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1822,9 +1822,6 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
>  	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
>  			shrinker);
>  
> -	if (!pool->shrinker_enabled)
> -		return 0;
> -
>  	for (i = zs_size_classes - 1; i >= 0; i--) {
>  		class = pool->size_class[i];
>  		if (!class)
> -- 
> 2.6.1.134.g4b1fd35
> 

Re: [PATCH] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Minchan Kim]

On Wed, Oct 14, 2015 at 10:57:59AM +0900, Sergey Senozhatsky wrote:
> We don't let user to disable shrinker in zsmalloc (once
> it's been enabled), so no need to check ->shrinker_enabled
> in zs_shrinker_count(), at the moment at least.

I'm in favor of removing shrinker disable feature with this patch(
although we didn't implement it yet) because if there is some problem
of compaction, we should reveal and fix it without hiding with the
feature.

One thing I want is if we decide it, let's remove all things
about shrinker_enabled(ie, variable).
If we might need it later, we could introduce it easily.

Thanks.

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  mm/zsmalloc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 7ad5e54..8ba247d 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1822,9 +1822,6 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
>  	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
>  			shrinker);
>  
> -	if (!pool->shrinker_enabled)
> -		return 0;
> -
>  	for (i = zs_size_classes - 1; i >= 0; i--) {
>  		class = pool->size_class[i];
>  		if (!class)
> -- 
> 2.6.1.134.g4b1fd35
> 

--
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] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Sergey Senozhatsky]

On (10/15/15 11:29), Minchan Kim wrote:
[..]
> I'm in favor of removing shrinker disable feature with this patch(
> although we didn't implement it yet) because if there is some problem
> of compaction, we should reveal and fix it without hiding with the
> feature.
> 

sure.

> One thing I want is if we decide it, let's remove all things
> about shrinker_enabled(ie, variable).
> If we might need it later, we could introduce it easily.

well, do we really want to make the shrinker a vital part of zsmalloc?

it's not that we will tighten the dependency between zsmalloc and
shrinker, we will introduce it instead. in a sense that, at the moment,
zsmalloc is, let's say, ignorant to shrinker registration errors
(shrinker registration implementation is internal to shrinker), because
there is no direct impact on zsmalloc functionality -- zsmalloc will not
be able to release some pages (there are if-s here: first, zsmalloc
shrinker callback may even not be called; second, zsmalloc may not be
albe to migrate objects and release objects).

no really strong opinion against, but at the same time zsmalloc will
have another point of failure (again, zsmalloc should not be aware of
shrinker registration implementation and why it may fail).

so... I can prepare a new patch later today.

	-ss

> 
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  mm/zsmalloc.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 7ad5e54..8ba247d 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -1822,9 +1822,6 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
> >  	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
> >  			shrinker);
> >  
> > -	if (!pool->shrinker_enabled)
> > -		return 0;
> > -
> >  	for (i = zs_size_classes - 1; i >= 0; i--) {
> >  		class = pool->size_class[i];
> >  		if (!class)
> > -- 
> > 2.6.1.134.g4b1fd35
> > 
> 

Re: [PATCH] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Sergey Senozhatsky]

On (10/15/15 11:29), Minchan Kim wrote:
[..]
> I'm in favor of removing shrinker disable feature with this patch(
> although we didn't implement it yet) because if there is some problem
> of compaction, we should reveal and fix it without hiding with the
> feature.
> 

sure.

> One thing I want is if we decide it, let's remove all things
> about shrinker_enabled(ie, variable).
> If we might need it later, we could introduce it easily.

well, do we really want to make the shrinker a vital part of zsmalloc?

it's not that we will tighten the dependency between zsmalloc and
shrinker, we will introduce it instead. in a sense that, at the moment,
zsmalloc is, let's say, ignorant to shrinker registration errors
(shrinker registration implementation is internal to shrinker), because
there is no direct impact on zsmalloc functionality -- zsmalloc will not
be able to release some pages (there are if-s here: first, zsmalloc
shrinker callback may even not be called; second, zsmalloc may not be
albe to migrate objects and release objects).

no really strong opinion against, but at the same time zsmalloc will
have another point of failure (again, zsmalloc should not be aware of
shrinker registration implementation and why it may fail).

so... I can prepare a new patch later today.

	-ss

> 
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  mm/zsmalloc.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 7ad5e54..8ba247d 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -1822,9 +1822,6 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
> >  	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
> >  			shrinker);
> >  
> > -	if (!pool->shrinker_enabled)
> > -		return 0;
> > -
> >  	for (i = zs_size_classes - 1; i >= 0; i--) {
> >  		class = pool->size_class[i];
> >  		if (!class)
> > -- 
> > 2.6.1.134.g4b1fd35
> > 
> 

--
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] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Minchan Kim]

On Thu, Oct 15, 2015 at 12:53:17PM +0900, Sergey Senozhatsky wrote:
> On (10/15/15 11:29), Minchan Kim wrote:
> [..]
> > I'm in favor of removing shrinker disable feature with this patch(
> > although we didn't implement it yet) because if there is some problem
> > of compaction, we should reveal and fix it without hiding with the
> > feature.
> > 
> 
> sure.
> 
> > One thing I want is if we decide it, let's remove all things
> > about shrinker_enabled(ie, variable).
> > If we might need it later, we could introduce it easily.
> 
> well, do we really want to make the shrinker a vital part of zsmalloc?
> 
> it's not that we will tighten the dependency between zsmalloc and
> shrinker, we will introduce it instead. in a sense that, at the moment,
> zsmalloc is, let's say, ignorant to shrinker registration errors
> (shrinker registration implementation is internal to shrinker), because
> there is no direct impact on zsmalloc functionality -- zsmalloc will not
> be able to release some pages (there are if-s here: first, zsmalloc
> shrinker callback may even not be called; second, zsmalloc may not be
> albe to migrate objects and release objects).
> 
> no really strong opinion against, but at the same time zsmalloc will
> have another point of failure (again, zsmalloc should not be aware of
> shrinker registration implementation and why it may fail).
> 
> so... I can prepare a new patch later today.

I misunderstood your description. I thought you wanted to remove
codes for disabling auto-compaction by user because I really don't
want it like same reason of VM's compaction. My bad.

You woke up my brain, I remember the reason.
Thanks.

Acked-by: Minchan Kim <minchan@kernel.org>

Re: [PATCH] zsmalloc: don't test shrinker_enabled in zs_shrinker_count()

[Minchan Kim]

On Thu, Oct 15, 2015 at 12:53:17PM +0900, Sergey Senozhatsky wrote:
> On (10/15/15 11:29), Minchan Kim wrote:
> [..]
> > I'm in favor of removing shrinker disable feature with this patch(
> > although we didn't implement it yet) because if there is some problem
> > of compaction, we should reveal and fix it without hiding with the
> > feature.
> > 
> 
> sure.
> 
> > One thing I want is if we decide it, let's remove all things
> > about shrinker_enabled(ie, variable).
> > If we might need it later, we could introduce it easily.
> 
> well, do we really want to make the shrinker a vital part of zsmalloc?
> 
> it's not that we will tighten the dependency between zsmalloc and
> shrinker, we will introduce it instead. in a sense that, at the moment,
> zsmalloc is, let's say, ignorant to shrinker registration errors
> (shrinker registration implementation is internal to shrinker), because
> there is no direct impact on zsmalloc functionality -- zsmalloc will not
> be able to release some pages (there are if-s here: first, zsmalloc
> shrinker callback may even not be called; second, zsmalloc may not be
> albe to migrate objects and release objects).
> 
> no really strong opinion against, but at the same time zsmalloc will
> have another point of failure (again, zsmalloc should not be aware of
> shrinker registration implementation and why it may fail).
> 
> so... I can prepare a new patch later today.

I misunderstood your description. I thought you wanted to remove
codes for disabling auto-compaction by user because I really don't
want it like same reason of VM's compaction. My bad.

You woke up my brain, I remember the reason.
Thanks.

Acked-by: Minchan Kim <minchan@kernel.org>

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