lkml.org 
[lkml]   [2021]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [v5 PATCH 06/11] mm: vmscan: use a new flag to indicate shrinker is registered
    From
    Date
    On 1/28/21 12:33 AM, Yang Shi wrote:
    > Currently registered shrinker is indicated by non-NULL shrinker->nr_deferred.
    > This approach is fine with nr_deferred at the 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.
    >
    > Remove SHRINKER_REGISTERING since we could check if shrinker is registered
    > successfully by the new flag.
    >
    > Signed-off-by: Yang Shi <shy828301@gmail.com>
    > ---
    > include/linux/shrinker.h | 7 ++++---
    > mm/vmscan.c | 27 +++++++++------------------
    > 2 files changed, 13 insertions(+), 21 deletions(-)
    >
    > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
    > index 0f80123650e2..1eac79ce57d4 100644
    > --- a/include/linux/shrinker.h
    > +++ b/include/linux/shrinker.h
    > @@ -79,13 +79,14 @@ struct shrinker {
    > #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
    >
    > /* Flags */
    > -#define SHRINKER_NUMA_AWARE (1 << 0)
    > -#define SHRINKER_MEMCG_AWARE (1 << 1)
    > +#define SHRINKER_REGISTERED (1 << 0)
    > +#define SHRINKER_NUMA_AWARE (1 << 1)
    > +#define SHRINKER_MEMCG_AWARE (1 << 2)
    > /*
    > * It just makes sense when the shrinker is also MEMCG_AWARE for now,
    > * non-MEMCG_AWARE shrinker should not have this flag set.
    > */
    > -#define SHRINKER_NONSLAB (1 << 2)
    > +#define SHRINKER_NONSLAB (1 << 3)
    >
    > extern int prealloc_shrinker(struct shrinker *shrinker);
    > extern void register_shrinker_prepared(struct shrinker *shrinker);
    > diff --git a/mm/vmscan.c b/mm/vmscan.c
    > index 92e917033797..256896d157d4 100644
    > --- a/mm/vmscan.c
    > +++ b/mm/vmscan.c
    > @@ -308,19 +308,6 @@ void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id)
    > }
    > }
    >
    > -/*
    > - * We allow subsystems to populate their shrinker-related
    > - * LRU lists before register_shrinker_prepared() is called
    > - * for the shrinker, since we don't want to impose
    > - * restrictions on their internal registration order.
    > - * In this case shrink_slab_memcg() may find corresponding
    > - * bit is set in the shrinkers map.
    > - *
    > - * This value is used by the function to detect registering
    > - * shrinkers and to skip do_shrink_slab() calls for them.
    > - */
    > -#define SHRINKER_REGISTERING ((struct shrinker *)~0UL)
    > -
    > static DEFINE_IDR(shrinker_idr);
    >
    > static int prealloc_memcg_shrinker(struct shrinker *shrinker)
    > @@ -329,7 +316,7 @@ static int prealloc_memcg_shrinker(struct shrinker *shrinker)
    >
    > down_write(&shrinker_rwsem);
    > /* This may call shrinker, so it must use down_read_trylock() */
    > - id = idr_alloc(&shrinker_idr, SHRINKER_REGISTERING, 0, 0, GFP_KERNEL);
    > + id = idr_alloc(&shrinker_idr, NULL, 0, 0, GFP_KERNEL);

    Is it still needed to register a NULL and then replace it with real pointer,
    given the SHRINKER_REGISTERED flag?

    > if (id < 0)
    > goto unlock;
    >
    > @@ -496,6 +483,7 @@ void register_shrinker_prepared(struct shrinker *shrinker)
    > if (shrinker->flags & SHRINKER_MEMCG_AWARE)
    > idr_replace(&shrinker_idr, shrinker, shrinker->id);
    > #endif
    > + shrinker->flags |= SHRINKER_REGISTERED;
    > up_write(&shrinker_rwsem);
    > }
    >
    > @@ -515,13 +503,16 @@ EXPORT_SYMBOL(register_shrinker);
    > */
    > void unregister_shrinker(struct shrinker *shrinker)
    > {
    > - if (!shrinker->nr_deferred)
    > + if (!(shrinker->flags & SHRINKER_REGISTERED))
    > return;
    > - if (shrinker->flags & SHRINKER_MEMCG_AWARE)
    > - unregister_memcg_shrinker(shrinker);
    > +
    > down_write(&shrinker_rwsem);
    > list_del(&shrinker->list);
    > + shrinker->flags &= ~SHRINKER_REGISTERED;
    > up_write(&shrinker_rwsem);
    > +
    > + if (shrinker->flags & SHRINKER_MEMCG_AWARE)
    > + unregister_memcg_shrinker(shrinker);
    > kfree(shrinker->nr_deferred);
    > shrinker->nr_deferred = NULL;
    > }
    > @@ -687,7 +678,7 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
    > struct shrinker *shrinker;
    >
    > shrinker = idr_find(&shrinker_idr, i);
    > - if (unlikely(!shrinker || shrinker == SHRINKER_REGISTERING)) {
    > + if (unlikely(!shrinker || !(shrinker->flags & SHRINKER_REGISTERED))) {
    > if (!shrinker)
    > clear_bit(i, info->map);
    > continue;
    >

    \
     
     \ /
      Last update: 2021-01-28 18:58    [W:2.522 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site