Hi Simon,

On 2023/7/28 20:17, Simon Horman wrote:

On Thu, Jul 27, 2023 at 04:04:18PM +0800, Qi Zheng wrote:

quoted

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.

quoted

setup.

Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>

...

quoted

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

quoted

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

...