diff mbox series

[4/9] mm: vmscan: use a new flag to indicate shrinker is registered

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

Commit Message

Yang Shi Dec. 2, 2020, 6:27 p.m. UTC
Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
This approach is fine with nr_deferred atthe shrinker level, but the following
patches will move MEMCG_AWARE shrinkers' nr_deferred to memcg level, so their
shrinker->nr_deferred would always be NULL.  This would prevent the shrinkers
from unregistering correctly.

Introduce a new "state" field to indicate if shrinker is registered or not.
We could use the highest bit of flags, but it may be a little bit complicated to
extract that bit and the flags is accessed frequently by vmscan (every time shrinker
is called).  So add a new field in "struct shrinker", we may waster a little bit
memory, but it should be very few since there should be not too many registered
shrinkers on a normal system.

Signed-off-by: Yang Shi <shy828301@gmail.com>
---
 include/linux/shrinker.h |  4 ++++
 mm/vmscan.c              | 13 +++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

Comments

Roman Gushchin Dec. 3, 2020, 3:01 a.m. UTC | #1
On Wed, Dec 02, 2020 at 10:27:20AM -0800, Yang Shi wrote:
> Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
> This approach is fine with nr_deferred atthe shrinker level, but the following
> patches will move MEMCG_AWARE shrinkers' nr_deferred to memcg level, so their
> shrinker->nr_deferred would always be NULL.  This would prevent the shrinkers
> from unregistering correctly.
> 
> Introduce a new "state" field to indicate if shrinker is registered or not.
> We could use the highest bit of flags, but it may be a little bit complicated to
> extract that bit and the flags is accessed frequently by vmscan (every time shrinker
> is called).  So add a new field in "struct shrinker", we may waster a little bit
> memory, but it should be very few since there should be not too many registered
> shrinkers on a normal system.
> 
> Signed-off-by: Yang Shi <shy828301@gmail.com>
> ---
>  include/linux/shrinker.h |  4 ++++
>  mm/vmscan.c              | 13 +++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index 0f80123650e2..0bb5be88e41d 100644
> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -35,6 +35,9 @@ struct shrink_control {
>  
>  #define SHRINK_STOP (~0UL)
>  #define SHRINK_EMPTY (~0UL - 1)
> +
> +#define SHRINKER_REGISTERED	0x1
> +
>  /*
>   * A callback you can register to apply pressure to ageable caches.
>   *
> @@ -66,6 +69,7 @@ struct shrinker {
>  	long batch;	/* reclaim batch size, 0 = default */
>  	int seeks;	/* seeks to recreate an obj */
>  	unsigned flags;
> +	unsigned state;

Hm, can't it be another flag? It seems like we have a plenty of free bits.

>  
>  	/* These are for internal use */
>  	struct list_head list;
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 457ce04eebf2..0d628299e55c 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -378,6 +378,7 @@ void register_shrinker_prepared(struct shrinker *shrinker)
>  	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
>  		idr_replace(&shrinker_idr, shrinker, shrinker->id);
>  #endif
> +	shrinker->state |= SHRINKER_REGISTERED;
>  	up_write(&shrinker_rwsem);
>  }
>  
> @@ -397,13 +398,17 @@ EXPORT_SYMBOL(register_shrinker);
>   */
>  void unregister_shrinker(struct shrinker *shrinker)
>  {
> -	if (!shrinker->nr_deferred)
> -		return;
> -	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> -		unregister_memcg_shrinker(shrinker);
>  	down_write(&shrinker_rwsem);
> +	if (!shrinker->state) {
> +		up_write(&shrinker_rwsem);
> +		return;
> +	}
>  	list_del(&shrinker->list);
> +	shrinker->state &= ~SHRINKER_REGISTERED;
>  	up_write(&shrinker_rwsem);
> +
> +	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> +		unregister_memcg_shrinker(shrinker);
>  	kfree(shrinker->nr_deferred);
>  	shrinker->nr_deferred = NULL;
>  }
> -- 
> 2.26.2
>
Yang Shi Dec. 3, 2020, 4:59 a.m. UTC | #2
On Wed, Dec 2, 2020 at 7:01 PM Roman Gushchin <guro@fb.com> wrote:
>
> On Wed, Dec 02, 2020 at 10:27:20AM -0800, Yang Shi wrote:
> > Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
> > This approach is fine with nr_deferred atthe shrinker level, but the following
> > patches will move MEMCG_AWARE shrinkers' nr_deferred to memcg level, so their
> > shrinker->nr_deferred would always be NULL.  This would prevent the shrinkers
> > from unregistering correctly.
> >
> > Introduce a new "state" field to indicate if shrinker is registered or not.
> > We could use the highest bit of flags, but it may be a little bit complicated to
> > extract that bit and the flags is accessed frequently by vmscan (every time shrinker
> > is called).  So add a new field in "struct shrinker", we may waster a little bit
> > memory, but it should be very few since there should be not too many registered
> > shrinkers on a normal system.
> >
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > ---
> >  include/linux/shrinker.h |  4 ++++
> >  mm/vmscan.c              | 13 +++++++++----
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > index 0f80123650e2..0bb5be88e41d 100644
> > --- a/include/linux/shrinker.h
> > +++ b/include/linux/shrinker.h
> > @@ -35,6 +35,9 @@ struct shrink_control {
> >
> >  #define SHRINK_STOP (~0UL)
> >  #define SHRINK_EMPTY (~0UL - 1)
> > +
> > +#define SHRINKER_REGISTERED  0x1
> > +
> >  /*
> >   * A callback you can register to apply pressure to ageable caches.
> >   *
> > @@ -66,6 +69,7 @@ struct shrinker {
> >       long batch;     /* reclaim batch size, 0 = default */
> >       int seeks;      /* seeks to recreate an obj */
> >       unsigned flags;
> > +     unsigned state;
>
> Hm, can't it be another flag? It seems like we have a plenty of free bits.

I thought about this too. But I was not convinced by myself that
messing flags with state is a good practice. We may add more flags in
the future, so we may end up having something like:

flag
flag
flag
state
flag
flag
...

Maybe we could use the highest bit for state?

>
> >
> >       /* These are for internal use */
> >       struct list_head list;
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 457ce04eebf2..0d628299e55c 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -378,6 +378,7 @@ void register_shrinker_prepared(struct shrinker *shrinker)
> >       if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> >               idr_replace(&shrinker_idr, shrinker, shrinker->id);
> >  #endif
> > +     shrinker->state |= SHRINKER_REGISTERED;
> >       up_write(&shrinker_rwsem);
> >  }
> >
> > @@ -397,13 +398,17 @@ EXPORT_SYMBOL(register_shrinker);
> >   */
> >  void unregister_shrinker(struct shrinker *shrinker)
> >  {
> > -     if (!shrinker->nr_deferred)
> > -             return;
> > -     if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> > -             unregister_memcg_shrinker(shrinker);
> >       down_write(&shrinker_rwsem);
> > +     if (!shrinker->state) {
> > +             up_write(&shrinker_rwsem);
> > +             return;
> > +     }
> >       list_del(&shrinker->list);
> > +     shrinker->state &= ~SHRINKER_REGISTERED;
> >       up_write(&shrinker_rwsem);
> > +
> > +     if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> > +             unregister_memcg_shrinker(shrinker);
> >       kfree(shrinker->nr_deferred);
> >       shrinker->nr_deferred = NULL;
> >  }
> > --
> > 2.26.2
> >
Roman Gushchin Dec. 3, 2020, 8:08 p.m. UTC | #3
On Wed, Dec 02, 2020 at 08:59:40PM -0800, Yang Shi wrote:
> On Wed, Dec 2, 2020 at 7:01 PM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Wed, Dec 02, 2020 at 10:27:20AM -0800, Yang Shi wrote:
> > > Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
> > > This approach is fine with nr_deferred atthe shrinker level, but the following
> > > patches will move MEMCG_AWARE shrinkers' nr_deferred to memcg level, so their
> > > shrinker->nr_deferred would always be NULL.  This would prevent the shrinkers
> > > from unregistering correctly.
> > >
> > > Introduce a new "state" field to indicate if shrinker is registered or not.
> > > We could use the highest bit of flags, but it may be a little bit complicated to
> > > extract that bit and the flags is accessed frequently by vmscan (every time shrinker
> > > is called).  So add a new field in "struct shrinker", we may waster a little bit
> > > memory, but it should be very few since there should be not too many registered
> > > shrinkers on a normal system.
> > >
> > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > > ---
> > >  include/linux/shrinker.h |  4 ++++
> > >  mm/vmscan.c              | 13 +++++++++----
> > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > > index 0f80123650e2..0bb5be88e41d 100644
> > > --- a/include/linux/shrinker.h
> > > +++ b/include/linux/shrinker.h
> > > @@ -35,6 +35,9 @@ struct shrink_control {
> > >
> > >  #define SHRINK_STOP (~0UL)
> > >  #define SHRINK_EMPTY (~0UL - 1)
> > > +
> > > +#define SHRINKER_REGISTERED  0x1
> > > +
> > >  /*
> > >   * A callback you can register to apply pressure to ageable caches.
> > >   *
> > > @@ -66,6 +69,7 @@ struct shrinker {
> > >       long batch;     /* reclaim batch size, 0 = default */
> > >       int seeks;      /* seeks to recreate an obj */
> > >       unsigned flags;
> > > +     unsigned state;
> >
> > Hm, can't it be another flag? It seems like we have a plenty of free bits.
> 
> I thought about this too. But I was not convinced by myself that
> messing flags with state is a good practice. We may add more flags in
> the future, so we may end up having something like:
> 
> flag
> flag
> flag
> state
> flag
> flag
> ...
> 
> Maybe we could use the highest bit for state?

Or just
state
flag
flag
flag
flag
flag
...

?
Yang Shi Dec. 3, 2020, 10:25 p.m. UTC | #4
On Thu, Dec 3, 2020 at 12:09 PM Roman Gushchin <guro@fb.com> wrote:
>
> On Wed, Dec 02, 2020 at 08:59:40PM -0800, Yang Shi wrote:
> > On Wed, Dec 2, 2020 at 7:01 PM Roman Gushchin <guro@fb.com> wrote:
> > >
> > > On Wed, Dec 02, 2020 at 10:27:20AM -0800, Yang Shi wrote:
> > > > Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
> > > > This approach is fine with nr_deferred atthe shrinker level, but the following
> > > > patches will move MEMCG_AWARE shrinkers' nr_deferred to memcg level, so their
> > > > shrinker->nr_deferred would always be NULL.  This would prevent the shrinkers
> > > > from unregistering correctly.
> > > >
> > > > Introduce a new "state" field to indicate if shrinker is registered or not.
> > > > We could use the highest bit of flags, but it may be a little bit complicated to
> > > > extract that bit and the flags is accessed frequently by vmscan (every time shrinker
> > > > is called).  So add a new field in "struct shrinker", we may waster a little bit
> > > > memory, but it should be very few since there should be not too many registered
> > > > shrinkers on a normal system.
> > > >
> > > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > > > ---
> > > >  include/linux/shrinker.h |  4 ++++
> > > >  mm/vmscan.c              | 13 +++++++++----
> > > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > > > index 0f80123650e2..0bb5be88e41d 100644
> > > > --- a/include/linux/shrinker.h
> > > > +++ b/include/linux/shrinker.h
> > > > @@ -35,6 +35,9 @@ struct shrink_control {
> > > >
> > > >  #define SHRINK_STOP (~0UL)
> > > >  #define SHRINK_EMPTY (~0UL - 1)
> > > > +
> > > > +#define SHRINKER_REGISTERED  0x1
> > > > +
> > > >  /*
> > > >   * A callback you can register to apply pressure to ageable caches.
> > > >   *
> > > > @@ -66,6 +69,7 @@ struct shrinker {
> > > >       long batch;     /* reclaim batch size, 0 = default */
> > > >       int seeks;      /* seeks to recreate an obj */
> > > >       unsigned flags;
> > > > +     unsigned state;
> > >
> > > Hm, can't it be another flag? It seems like we have a plenty of free bits.
> >
> > I thought about this too. But I was not convinced by myself that
> > messing flags with state is a good practice. We may add more flags in
> > the future, so we may end up having something like:
> >
> > flag
> > flag
> > flag
> > state
> > flag
> > flag
> > ...
> >
> > Maybe we could use the highest bit for state?
>
> Or just
> state
> flag
> flag
> flag
> flag
> flag
> ...
>
> ?

It is fine too. We should not add more states in foreseeable future.
Johannes Weiner Dec. 4, 2020, 6:52 p.m. UTC | #5
On Thu, Dec 03, 2020 at 02:25:20PM -0800, Yang Shi wrote:
> On Thu, Dec 3, 2020 at 12:09 PM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Wed, Dec 02, 2020 at 08:59:40PM -0800, Yang Shi wrote:
> > > On Wed, Dec 2, 2020 at 7:01 PM Roman Gushchin <guro@fb.com> wrote:
> > > >
> > > > On Wed, Dec 02, 2020 at 10:27:20AM -0800, Yang Shi wrote:
> > > > > Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
> > > > > This approach is fine with nr_deferred atthe shrinker level, but the following
> > > > > patches will move MEMCG_AWARE shrinkers' nr_deferred to memcg level, so their
> > > > > shrinker->nr_deferred would always be NULL.  This would prevent the shrinkers
> > > > > from unregistering correctly.
> > > > >
> > > > > Introduce a new "state" field to indicate if shrinker is registered or not.
> > > > > We could use the highest bit of flags, but it may be a little bit complicated to
> > > > > extract that bit and the flags is accessed frequently by vmscan (every time shrinker
> > > > > is called).  So add a new field in "struct shrinker", we may waster a little bit
> > > > > memory, but it should be very few since there should be not too many registered
> > > > > shrinkers on a normal system.
> > > > >
> > > > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > > > > ---
> > > > >  include/linux/shrinker.h |  4 ++++
> > > > >  mm/vmscan.c              | 13 +++++++++----
> > > > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > > > > index 0f80123650e2..0bb5be88e41d 100644
> > > > > --- a/include/linux/shrinker.h
> > > > > +++ b/include/linux/shrinker.h
> > > > > @@ -35,6 +35,9 @@ struct shrink_control {
> > > > >
> > > > >  #define SHRINK_STOP (~0UL)
> > > > >  #define SHRINK_EMPTY (~0UL - 1)
> > > > > +
> > > > > +#define SHRINKER_REGISTERED  0x1
> > > > > +
> > > > >  /*
> > > > >   * A callback you can register to apply pressure to ageable caches.
> > > > >   *
> > > > > @@ -66,6 +69,7 @@ struct shrinker {
> > > > >       long batch;     /* reclaim batch size, 0 = default */
> > > > >       int seeks;      /* seeks to recreate an obj */
> > > > >       unsigned flags;
> > > > > +     unsigned state;
> > > >
> > > > Hm, can't it be another flag? It seems like we have a plenty of free bits.
> > >
> > > I thought about this too. But I was not convinced by myself that
> > > messing flags with state is a good practice. We may add more flags in
> > > the future, so we may end up having something like:
> > >
> > > flag
> > > flag
> > > flag
> > > state
> > > flag
> > > flag
> > > ...
> > >
> > > Maybe we could use the highest bit for state?
> >
> > Or just
> > state
> > flag
> > flag
> > flag
> > flag
> > flag
> > ...
> >
> > ?
> 
> It is fine too. We should not add more states in foreseeable future.

It's always possible to shuffle things around for cleanup later on,
too. We don't have to provide binary compatibility for existing flags,
and changing a couple of adjacent bits isn't a big deal to keep things
neat. Or am I missing something?
Yang Shi Dec. 4, 2020, 9:24 p.m. UTC | #6
On Fri, Dec 4, 2020 at 10:54 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Thu, Dec 03, 2020 at 02:25:20PM -0800, Yang Shi wrote:
> > On Thu, Dec 3, 2020 at 12:09 PM Roman Gushchin <guro@fb.com> wrote:
> > >
> > > On Wed, Dec 02, 2020 at 08:59:40PM -0800, Yang Shi wrote:
> > > > On Wed, Dec 2, 2020 at 7:01 PM Roman Gushchin <guro@fb.com> wrote:
> > > > >
> > > > > On Wed, Dec 02, 2020 at 10:27:20AM -0800, Yang Shi wrote:
> > > > > > Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
> > > > > > This approach is fine with nr_deferred atthe shrinker level, but the following
> > > > > > patches will move MEMCG_AWARE shrinkers' nr_deferred to memcg level, so their
> > > > > > shrinker->nr_deferred would always be NULL.  This would prevent the shrinkers
> > > > > > from unregistering correctly.
> > > > > >
> > > > > > Introduce a new "state" field to indicate if shrinker is registered or not.
> > > > > > We could use the highest bit of flags, but it may be a little bit complicated to
> > > > > > extract that bit and the flags is accessed frequently by vmscan (every time shrinker
> > > > > > is called).  So add a new field in "struct shrinker", we may waster a little bit
> > > > > > memory, but it should be very few since there should be not too many registered
> > > > > > shrinkers on a normal system.
> > > > > >
> > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com>
> > > > > > ---
> > > > > >  include/linux/shrinker.h |  4 ++++
> > > > > >  mm/vmscan.c              | 13 +++++++++----
> > > > > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > > > > > index 0f80123650e2..0bb5be88e41d 100644
> > > > > > --- a/include/linux/shrinker.h
> > > > > > +++ b/include/linux/shrinker.h
> > > > > > @@ -35,6 +35,9 @@ struct shrink_control {
> > > > > >
> > > > > >  #define SHRINK_STOP (~0UL)
> > > > > >  #define SHRINK_EMPTY (~0UL - 1)
> > > > > > +
> > > > > > +#define SHRINKER_REGISTERED  0x1
> > > > > > +
> > > > > >  /*
> > > > > >   * A callback you can register to apply pressure to ageable caches.
> > > > > >   *
> > > > > > @@ -66,6 +69,7 @@ struct shrinker {
> > > > > >       long batch;     /* reclaim batch size, 0 = default */
> > > > > >       int seeks;      /* seeks to recreate an obj */
> > > > > >       unsigned flags;
> > > > > > +     unsigned state;
> > > > >
> > > > > Hm, can't it be another flag? It seems like we have a plenty of free bits.
> > > >
> > > > I thought about this too. But I was not convinced by myself that
> > > > messing flags with state is a good practice. We may add more flags in
> > > > the future, so we may end up having something like:
> > > >
> > > > flag
> > > > flag
> > > > flag
> > > > state
> > > > flag
> > > > flag
> > > > ...
> > > >
> > > > Maybe we could use the highest bit for state?
> > >
> > > Or just
> > > state
> > > flag
> > > flag
> > > flag
> > > flag
> > > flag
> > > ...
> > >
> > > ?
> >
> > It is fine too. We should not add more states in foreseeable future.
>
> It's always possible to shuffle things around for cleanup later on,
> too. We don't have to provide binary compatibility for existing flags,
> and changing a couple of adjacent bits isn't a big deal to keep things
> neat. Or am I missing something?

No. It is definitely not a big deal.
diff mbox series

Patch

diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 0f80123650e2..0bb5be88e41d 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -35,6 +35,9 @@  struct shrink_control {
 
 #define SHRINK_STOP (~0UL)
 #define SHRINK_EMPTY (~0UL - 1)
+
+#define SHRINKER_REGISTERED	0x1
+
 /*
  * A callback you can register to apply pressure to ageable caches.
  *
@@ -66,6 +69,7 @@  struct shrinker {
 	long batch;	/* reclaim batch size, 0 = default */
 	int seeks;	/* seeks to recreate an obj */
 	unsigned flags;
+	unsigned state;
 
 	/* These are for internal use */
 	struct list_head list;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 457ce04eebf2..0d628299e55c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -378,6 +378,7 @@  void register_shrinker_prepared(struct shrinker *shrinker)
 	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
 		idr_replace(&shrinker_idr, shrinker, shrinker->id);
 #endif
+	shrinker->state |= SHRINKER_REGISTERED;
 	up_write(&shrinker_rwsem);
 }
 
@@ -397,13 +398,17 @@  EXPORT_SYMBOL(register_shrinker);
  */
 void unregister_shrinker(struct shrinker *shrinker)
 {
-	if (!shrinker->nr_deferred)
-		return;
-	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
-		unregister_memcg_shrinker(shrinker);
 	down_write(&shrinker_rwsem);
+	if (!shrinker->state) {
+		up_write(&shrinker_rwsem);
+		return;
+	}
 	list_del(&shrinker->list);
+	shrinker->state &= ~SHRINKER_REGISTERED;
 	up_write(&shrinker_rwsem);
+
+	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
+		unregister_memcg_shrinker(shrinker);
 	kfree(shrinker->nr_deferred);
 	shrinker->nr_deferred = NULL;
 }