diff mbox series

[v2,7/9] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers

Message ID 20201214223722.232537-8-shy828301@gmail.com (mailing list archive)
State New
Headers show
Series Make shrinker's nr_deferred memcg aware | expand

Commit Message

Yang Shi Dec. 14, 2020, 10:37 p.m. UTC
Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need
allocate shrinker->nr_deferred for such shrinkers anymore.

Signed-off-by: Yang Shi <shy828301@gmail.com>
---
 mm/vmscan.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Comments

Dave Chinner Dec. 15, 2020, 3:05 a.m. UTC | #1
On Mon, Dec 14, 2020 at 02:37:20PM -0800, Yang Shi wrote:
> Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need
> allocate shrinker->nr_deferred for such shrinkers anymore.
> 
> Signed-off-by: Yang Shi <shy828301@gmail.com>
> ---
>  mm/vmscan.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bce8cf44eca2..8d5bfd818acd 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone
>   */
>  int prealloc_shrinker(struct shrinker *shrinker)
>  {
> -	unsigned int size = sizeof(*shrinker->nr_deferred);
> +	unsigned int size;
> +
> +	if (is_deferred_memcg_aware(shrinker)) {
> +		if (prealloc_memcg_shrinker(shrinker))
> +			return -ENOMEM;
> +		return 0;
> +	}
> +
> +	size = sizeof(*shrinker->nr_deferred);
>  
>  	if (shrinker->flags & SHRINKER_NUMA_AWARE)
>  		size *= nr_node_ids;
> @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker)
>  	if (!shrinker->nr_deferred)
>  		return -ENOMEM;
>  
> -	if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> -		if (prealloc_memcg_shrinker(shrinker))
> -			goto free_deferred;
> -	}
> -
>  	return 0;
> -
> -free_deferred:
> -	kfree(shrinker->nr_deferred);
> -	shrinker->nr_deferred = NULL;
> -	return -ENOMEM;
>  }

I'm trying to put my finger on it, but this seems wrong to me. If
memcgs are disabled, then prealloc_memcg_shrinker() needs to fail.
The preallocation code should not care about internal memcg details
like this.

	/*
	 * If the shrinker is memcg aware and memcgs are not
	 * enabled, clear the MEMCG flag and fall back to non-memcg
	 * behaviour for the shrinker.
	 */
	if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
		error = prealloc_memcg_shrinker(shrinker);
		if (!error)
			return 0;
		if (error != -ENOSYS)
			return error;

		/* memcgs not enabled! */
		shrinker->flags &= ~SHRINKER_MEMCG_AWARE;
	}

	size = sizeof(*shrinker->nr_deferred);
	....
	return 0;
}

This guarantees that only the shrinker instances taht have a
correctly set up memcg attached to them will have the
SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker
code, we only ever need to check for SHRINKER_MEMCG_AWARE to
determine what we should do....

>  void free_prealloced_shrinker(struct shrinker *shrinker)
>  {
> -	if (!shrinker->nr_deferred)
> +	if (is_deferred_memcg_aware(shrinker)) {
> +		unregister_memcg_shrinker(shrinker);
>  		return;
> +	}
>  
> -	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> -		unregister_memcg_shrinker(shrinker);
> +	if (!shrinker->nr_deferred)
> +		return;
>  
>  	kfree(shrinker->nr_deferred);
>  	shrinker->nr_deferred = NULL;

e.g. then this function can simply do:

{
	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
		return unregister_memcg_shrinker(shrinker);
	kfree(shrinker->nr_deferred);
	shrinker->nr_deferred = NULL;
}

Cheers,

Dave.
Yang Shi Dec. 15, 2020, 11:07 p.m. UTC | #2
On Mon, Dec 14, 2020 at 7:05 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Mon, Dec 14, 2020 at 02:37:20PM -0800, Yang Shi wrote:
> > Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need
> > allocate shrinker->nr_deferred for such shrinkers anymore.
> >
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > ---
> >  mm/vmscan.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index bce8cf44eca2..8d5bfd818acd 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone
> >   */
> >  int prealloc_shrinker(struct shrinker *shrinker)
> >  {
> > -     unsigned int size = sizeof(*shrinker->nr_deferred);
> > +     unsigned int size;
> > +
> > +     if (is_deferred_memcg_aware(shrinker)) {
> > +             if (prealloc_memcg_shrinker(shrinker))
> > +                     return -ENOMEM;
> > +             return 0;
> > +     }
> > +
> > +     size = sizeof(*shrinker->nr_deferred);
> >
> >       if (shrinker->flags & SHRINKER_NUMA_AWARE)
> >               size *= nr_node_ids;
> > @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker)
> >       if (!shrinker->nr_deferred)
> >               return -ENOMEM;
> >
> > -     if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> > -             if (prealloc_memcg_shrinker(shrinker))
> > -                     goto free_deferred;
> > -     }
> > -
> >       return 0;
> > -
> > -free_deferred:
> > -     kfree(shrinker->nr_deferred);
> > -     shrinker->nr_deferred = NULL;
> > -     return -ENOMEM;
> >  }
>
> I'm trying to put my finger on it, but this seems wrong to me. If
> memcgs are disabled, then prealloc_memcg_shrinker() needs to fail.
> The preallocation code should not care about internal memcg details
> like this.
>
>         /*
>          * If the shrinker is memcg aware and memcgs are not
>          * enabled, clear the MEMCG flag and fall back to non-memcg
>          * behaviour for the shrinker.
>          */
>         if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
>                 error = prealloc_memcg_shrinker(shrinker);
>                 if (!error)
>                         return 0;
>                 if (error != -ENOSYS)
>                         return error;
>
>                 /* memcgs not enabled! */
>                 shrinker->flags &= ~SHRINKER_MEMCG_AWARE;
>         }
>
>         size = sizeof(*shrinker->nr_deferred);
>         ....
>         return 0;
> }
>
> This guarantees that only the shrinker instances taht have a
> correctly set up memcg attached to them will have the
> SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker
> code, we only ever need to check for SHRINKER_MEMCG_AWARE to
> determine what we should do....

Thanks. I see your point. We could move the memcg specific details
into prealloc_memcg_shrinker().

It seems we have to acquire shrinker_rwsem before we check and modify
SHIRNKER_MEMCG_AWARE bit if we may clear it.

>
> >  void free_prealloced_shrinker(struct shrinker *shrinker)
> >  {
> > -     if (!shrinker->nr_deferred)
> > +     if (is_deferred_memcg_aware(shrinker)) {
> > +             unregister_memcg_shrinker(shrinker);
> >               return;
> > +     }
> >
> > -     if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> > -             unregister_memcg_shrinker(shrinker);
> > +     if (!shrinker->nr_deferred)
> > +             return;
> >
> >       kfree(shrinker->nr_deferred);
> >       shrinker->nr_deferred = NULL;
>
> e.g. then this function can simply do:
>
> {
>         if (shrinker->flags & SHRINKER_MEMCG_AWARE)
>                 return unregister_memcg_shrinker(shrinker);
>         kfree(shrinker->nr_deferred);
>         shrinker->nr_deferred = NULL;
> }
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
Yang Shi Dec. 18, 2020, 12:56 a.m. UTC | #3
On Tue, Dec 15, 2020 at 3:07 PM Yang Shi <shy828301@gmail.com> wrote:
>
> On Mon, Dec 14, 2020 at 7:05 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Mon, Dec 14, 2020 at 02:37:20PM -0800, Yang Shi wrote:
> > > Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need
> > > allocate shrinker->nr_deferred for such shrinkers anymore.
> > >
> > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > > ---
> > >  mm/vmscan.c | 28 ++++++++++++++--------------
> > >  1 file changed, 14 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index bce8cf44eca2..8d5bfd818acd 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone
> > >   */
> > >  int prealloc_shrinker(struct shrinker *shrinker)
> > >  {
> > > -     unsigned int size = sizeof(*shrinker->nr_deferred);
> > > +     unsigned int size;
> > > +
> > > +     if (is_deferred_memcg_aware(shrinker)) {
> > > +             if (prealloc_memcg_shrinker(shrinker))
> > > +                     return -ENOMEM;
> > > +             return 0;
> > > +     }
> > > +
> > > +     size = sizeof(*shrinker->nr_deferred);
> > >
> > >       if (shrinker->flags & SHRINKER_NUMA_AWARE)
> > >               size *= nr_node_ids;
> > > @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker)
> > >       if (!shrinker->nr_deferred)
> > >               return -ENOMEM;
> > >
> > > -     if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> > > -             if (prealloc_memcg_shrinker(shrinker))
> > > -                     goto free_deferred;
> > > -     }
> > > -
> > >       return 0;
> > > -
> > > -free_deferred:
> > > -     kfree(shrinker->nr_deferred);
> > > -     shrinker->nr_deferred = NULL;
> > > -     return -ENOMEM;
> > >  }
> >
> > I'm trying to put my finger on it, but this seems wrong to me. If
> > memcgs are disabled, then prealloc_memcg_shrinker() needs to fail.
> > The preallocation code should not care about internal memcg details
> > like this.
> >
> >         /*
> >          * If the shrinker is memcg aware and memcgs are not
> >          * enabled, clear the MEMCG flag and fall back to non-memcg
> >          * behaviour for the shrinker.
> >          */
> >         if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> >                 error = prealloc_memcg_shrinker(shrinker);
> >                 if (!error)
> >                         return 0;
> >                 if (error != -ENOSYS)
> >                         return error;
> >
> >                 /* memcgs not enabled! */
> >                 shrinker->flags &= ~SHRINKER_MEMCG_AWARE;
> >         }
> >
> >         size = sizeof(*shrinker->nr_deferred);
> >         ....
> >         return 0;
> > }
> >
> > This guarantees that only the shrinker instances taht have a
> > correctly set up memcg attached to them will have the
> > SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker
> > code, we only ever need to check for SHRINKER_MEMCG_AWARE to
> > determine what we should do....
>
> Thanks. I see your point. We could move the memcg specific details
> into prealloc_memcg_shrinker().
>
> It seems we have to acquire shrinker_rwsem before we check and modify
> SHIRNKER_MEMCG_AWARE bit if we may clear it.

Hi Dave,

Is it possible that shrinker register races with shrinker unregister?
It seems impossible to me by a quick visual code inspection. But I'm
not a VFS expert so I'm not quite sure.

If it is impossible the implementation would be quite simple otherwise
we need move shrinker_rwsem acquire/release to
prealloc_shrinker/free_prealloced_shrinker/unregister_shrinker to
protect SHRINKER_MEMCG_AWARE update.

>
> >
> > >  void free_prealloced_shrinker(struct shrinker *shrinker)
> > >  {
> > > -     if (!shrinker->nr_deferred)
> > > +     if (is_deferred_memcg_aware(shrinker)) {
> > > +             unregister_memcg_shrinker(shrinker);
> > >               return;
> > > +     }
> > >
> > > -     if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> > > -             unregister_memcg_shrinker(shrinker);
> > > +     if (!shrinker->nr_deferred)
> > > +             return;
> > >
> > >       kfree(shrinker->nr_deferred);
> > >       shrinker->nr_deferred = NULL;
> >
> > e.g. then this function can simply do:
> >
> > {
> >         if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> >                 return unregister_memcg_shrinker(shrinker);
> >         kfree(shrinker->nr_deferred);
> >         shrinker->nr_deferred = NULL;
> > }
> >
> > Cheers,
> >
> > Dave.
> > --
> > Dave Chinner
> > david@fromorbit.com
Dave Chinner Dec. 18, 2020, 1:09 a.m. UTC | #4
On Thu, Dec 17, 2020 at 04:56:48PM -0800, Yang Shi wrote:
> On Tue, Dec 15, 2020 at 3:07 PM Yang Shi <shy828301@gmail.com> wrote:
> > > This guarantees that only the shrinker instances taht have a
> > > correctly set up memcg attached to them will have the
> > > SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker
> > > code, we only ever need to check for SHRINKER_MEMCG_AWARE to
> > > determine what we should do....
> >
> > Thanks. I see your point. We could move the memcg specific details
> > into prealloc_memcg_shrinker().
> >
> > It seems we have to acquire shrinker_rwsem before we check and modify
> > SHIRNKER_MEMCG_AWARE bit if we may clear it.
> 
> Hi Dave,
> 
> Is it possible that shrinker register races with shrinker unregister?
> It seems impossible to me by a quick visual code inspection. But I'm
> not a VFS expert so I'm not quite sure.

Uh, if you have a shrinker racing to register and unregister, you've
got a major bug in your object initialisation/teardown code. i.e.
calling reagister/unregister at the same time for the same shrinker
is a bug, pure and simple.

Cheers,

Dave.
diff mbox series

Patch

diff --git a/mm/vmscan.c b/mm/vmscan.c
index bce8cf44eca2..8d5bfd818acd 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -420,7 +420,15 @@  unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone
  */
 int prealloc_shrinker(struct shrinker *shrinker)
 {
-	unsigned int size = sizeof(*shrinker->nr_deferred);
+	unsigned int size;
+
+	if (is_deferred_memcg_aware(shrinker)) {
+		if (prealloc_memcg_shrinker(shrinker))
+			return -ENOMEM;
+		return 0;
+	}
+
+	size = sizeof(*shrinker->nr_deferred);
 
 	if (shrinker->flags & SHRINKER_NUMA_AWARE)
 		size *= nr_node_ids;
@@ -429,26 +437,18 @@  int prealloc_shrinker(struct shrinker *shrinker)
 	if (!shrinker->nr_deferred)
 		return -ENOMEM;
 
-	if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
-		if (prealloc_memcg_shrinker(shrinker))
-			goto free_deferred;
-	}
-
 	return 0;
-
-free_deferred:
-	kfree(shrinker->nr_deferred);
-	shrinker->nr_deferred = NULL;
-	return -ENOMEM;
 }
 
 void free_prealloced_shrinker(struct shrinker *shrinker)
 {
-	if (!shrinker->nr_deferred)
+	if (is_deferred_memcg_aware(shrinker)) {
+		unregister_memcg_shrinker(shrinker);
 		return;
+	}
 
-	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
-		unregister_memcg_shrinker(shrinker);
+	if (!shrinker->nr_deferred)
+		return;
 
 	kfree(shrinker->nr_deferred);
 	shrinker->nr_deferred = NULL;