lkml.org 
[lkml]   [2021]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [v3 PATCH 05/11] mm: vmscan: use a new flag to indicate shrinker is registered
    From
    Date
    On 06.01.2021 01:58, 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.
    >
    > Signed-off-by: Yang Shi <shy828301@gmail.com>
    > ---
    > include/linux/shrinker.h | 7 ++++---
    > mm/vmscan.c | 13 +++++++++----
    > 2 files changed, 13 insertions(+), 7 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 8da765a85569..9761c7c27412 100644
    > --- a/mm/vmscan.c
    > +++ b/mm/vmscan.c
    > @@ -494,6 +494,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;

    In case of we introduce this new flag, we should kill old flag SHRINKER_REGISTERING,
    which are not needed anymore (we should you the new flag instead of that).

    > up_write(&shrinker_rwsem);
    > }
    >
    > @@ -513,13 +514,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);

    I do not think there are some users which registration may race with unregistration.
    So, I think we should check SHRINKER_REGISTERED unlocked similar to we used to check
    shrinker->nr_deferred unlocked.

    > + if (!(shrinker->flags & SHRINKER_REGISTERED)) {
    > + up_write(&shrinker_rwsem);
    > + return;
    > + }
    > 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;
    > }
    >

    \
     
     \ /
      Last update: 2021-01-06 11:24    [W:6.728 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site