Re: [PATCH v3 05/49] mm: shrinker: add infrastructure for dynamically allocating shrinker

[Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>]

Hi Simon,

On 2023/7/28 20:17, Simon Horman wrote:
> On Thu, Jul 27, 2023 at 04:04:18PM +0800, Qi Zheng wrote:
> > Currently, the shrinker instances can be divided into the following three
> > types:
> >
> > a) global shrinker instance statically defined in the kernel, such as
> >     workingset_shadow_shrinker.
> >
> > b) global shrinker instance statically defined in the kernel modules, such
> >     as mmu_shrinker in x86.
> >
> > c) shrinker instance embedded in other structures.
> >
> > For case a, the memory of shrinker instance is never freed. For case b,
> > the memory of shrinker instance will be freed after synchronize_rcu() when
> > the module is unloaded. For case c, the memory of shrinker instance will
> > be freed along with the structure it is embedded in.
> >
> > In preparation for implementing lockless slab shrink, we need to
> > dynamically allocate those shrinker instances in case c, then the memory
> > can be dynamically freed alone by calling kfree_rcu().
> >
> > So this commit adds the following new APIs for dynamically allocating
> > shrinker, and add a private_data field to struct shrinker to record and
> > get the original embedded structure.
> >
> > 1. shrinker_alloc()
> >
> > Used to allocate shrinker instance itself and related memory, it will
> > return a pointer to the shrinker instance on success and NULL on failure.
> >
> > 2. shrinker_register()
> >
> > Used to register the shrinker instance, which is same as the current
> > register_shrinker_prepared().
> >
> > 3. shrinker_free()
> >
> > Used to unregister (if needed) and free the shrinker instance.
> >
> > In order to simplify shrinker-related APIs and make shrinker more
> > independent of other kernel mechanisms, subsequent submissions will use
> > the above API to convert all shrinkers (including case a and b) to
> > dynamically allocated, and then remove all existing APIs.
> >
> > This will also have another advantage mentioned by Dave Chinner:
> >
> > ```
> > The other advantage of this is that it will break all the existing
> > out of tree code and third party modules using the old API and will
> > no longer work with a kernel using lockless slab shrinkers. They
> > need to break (both at the source and binary levels) to stop bad
> > things from happening due to using uncoverted shrinkers in the new
> nit: uncoverted -> unconverted
Thanks. Will fix.

> > setup.
> > ```
> >
> > Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> ...
>
> > diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c
> > index f1becfd45853..506257585408 100644
> > --- a/mm/shrinker_debug.c
> > +++ b/mm/shrinker_debug.c
> > @@ -191,6 +191,20 @@ int shrinker_debugfs_add(struct shrinker *shrinker)
> >         return 0;
> >   }
> >   
> > +int shrinker_debugfs_name_alloc(struct shrinker *shrinker, const char *fmt,
> > +                               va_list ap)
> > +{
> > +       shrinker->name = kvasprintf_const(GFP_KERNEL, fmt, ap);
> > +
> > +       return shrinker->name ? 0 : -ENOMEM;
> > +}
> > +
> > +void shrinker_debugfs_name_free(struct shrinker *shrinker)
> > +{
> > +       kfree_const(shrinker->name);
> > +       shrinker->name = NULL;
> > +}
> > +
> These functions have no prototype in this file,
> perhaps internal.h should be included?
The compiler can find these implementations, so I don't think there
is a need to include internal.h here?

Thanks,
Qi

> >   int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...)
> >   {
> >         struct dentry *entry;
> ...