Re: [PATCH v6 01/45] mm: shrinker: add infrastructure for dynamically allocating shrinker

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2023/9/18 17:03, Muchun Song wrote:



...

@@ -95,6 +97,11 @@ struct shrinker {
   * non-MEMCG_AWARE shrinker should not have this flag set.
   */
  #define SHRINKER_NONSLAB    (1 << 3)
+#define SHRINKER_ALLOCATED    (1 << 4)

It is better to add a comment here to tell users
it is only used by internals of shrinker. The users
are not supposed to pass this flag to shrink APIs.

OK, will annotate SHRINKER_REGISTERED and SHRINKER_ALLOCATED
as flags used internally.


+
+struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...);
+void shrinker_register(struct shrinker *shrinker);
+void shrinker_free(struct shrinker *shrinker);
  extern int __printf(2, 3) prealloc_shrinker(struct shrinker *shrinker,
                          const char *fmt, ...);
diff --git a/mm/internal.h b/mm/internal.h
index 0471d6326d01..5587cae20ebf 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1161,6 +1161,9 @@ unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg,
  #ifdef CONFIG_SHRINKER_DEBUG
  extern int shrinker_debugfs_add(struct shrinker *shrinker);
+extern int shrinker_debugfs_name_alloc(struct shrinker *shrinker,
+                       const char *fmt, va_list ap);
+extern void shrinker_debugfs_name_free(struct shrinker *shrinker);
  extern struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker,
                            int *debugfs_id);
  extern void shrinker_debugfs_remove(struct dentry *debugfs_entry,
@@ -1170,6 +1173,14 @@ static inline int shrinker_debugfs_add(struct shrinker *shrinker)
  {
      return 0;
  }
+static inline int shrinker_debugfs_name_alloc(struct shrinker *shrinker,
+                          const char *fmt, va_list ap)
+{
+    return 0;
+}
+static inline void shrinker_debugfs_name_free(struct shrinker *shrinker)
+{
+}
  static inline struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker,
                               int *debugfs_id)
  {
diff --git a/mm/shrinker.c b/mm/shrinker.c
index a16cd448b924..201211a67827 100644
--- a/mm/shrinker.c
+++ b/mm/shrinker.c
@@ -550,6 +550,108 @@ unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg,
      return freed;
  }
+struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...)
+{
+    struct shrinker *shrinker;
+    unsigned int size;
+    va_list ap;
+    int err;
+
+    shrinker = kzalloc(sizeof(struct shrinker), GFP_KERNEL);
+    if (!shrinker)
+        return NULL;
+
+    va_start(ap, fmt);
+    err = shrinker_debugfs_name_alloc(shrinker, fmt, ap);
+    va_end(ap);
+    if (err)
+        goto err_name;
+
+    shrinker->flags = flags | SHRINKER_ALLOCATED;
+    shrinker->seeks = DEFAULT_SEEKS;
+
+    if (flags & SHRINKER_MEMCG_AWARE) {
+        err = prealloc_memcg_shrinker(shrinker);
+        if (err == -ENOSYS)
+            shrinker->flags &= ~SHRINKER_MEMCG_AWARE;
+        else if (err == 0)
+            goto done;
+        else
+            goto err_flags;

Actually, the code here is a little confusing me when I fist look
at it. I think there could be some improvements here. Something
like:

         if (flags & SHRINKER_MEMCG_AWARE) {
                 err = prealloc_memcg_shrinker(shrinker);
                 if (err == -ENOSYS) {
                        /* Memcg is not supported and fallback to non-memcg-aware shrinker. */
                         shrinker->flags &= ~SHRINKER_MEMCG_AWARE;
                         goto non-memcg;
                 }

                 if (err)
                     goto err_flags;
                 return shrinker;
         }

non-memcg:
         [...]
         return shrinker;

In this case, the code becomes more clear (at least for me). We have split the code into two part, one is handling memcg-aware case, another is non-memcg-aware case. Any side will have a explicit "return" keyword to return once succeeds.
It is a little implicit that the previous one uses "goto done".

And the tag of "non-memcg" is also a good annotation to tell us the following
code handles non-memcg-aware case.

Make sense, will do.


+    }
+
+    /*
+     * The nr_deferred is available on per memcg level for memcg aware
+     * shrinkers, so only allocate nr_deferred in the following cases:
+     *  - non memcg aware shrinkers
+     *  - !CONFIG_MEMCG
+     *  - memcg is disabled by kernel command line
+     */
+    size = sizeof(*shrinker->nr_deferred);
+    if (flags & SHRINKER_NUMA_AWARE)
+        size *= nr_node_ids;
+
+    shrinker->nr_deferred = kzalloc(size, GFP_KERNEL);
+    if (!shrinker->nr_deferred)
+        goto err_flags;
+
+done:
+    return shrinker;
+
+err_flags:
+    shrinker_debugfs_name_free(shrinker);
+err_name:
+    kfree(shrinker);
+    return NULL;
+}
+EXPORT_SYMBOL_GPL(shrinker_alloc);
+
+void shrinker_register(struct shrinker *shrinker)
+{
+    if (unlikely(!(shrinker->flags & SHRINKER_ALLOCATED))) {
+        pr_warn("Must use shrinker_alloc() to dynamically allocate the shrinker");
+        return;
+    }
+
+    down_write(&shrinker_rwsem);
+    list_add_tail(&shrinker->list, &shrinker_list);
+    shrinker->flags |= SHRINKER_REGISTERED;
+    shrinker_debugfs_add(shrinker);
+    up_write(&shrinker_rwsem);
+}
+EXPORT_SYMBOL_GPL(shrinker_register);
+
+void shrinker_free(struct shrinker *shrinker)
+{
+    struct dentry *debugfs_entry = NULL;
+    int debugfs_id;
+
+    if (!shrinker)
+        return;
+
+    down_write(&shrinker_rwsem);
+    if (shrinker->flags & SHRINKER_REGISTERED) {
+        list_del(&shrinker->list);
+        debugfs_entry = shrinker_debugfs_detach(shrinker, &debugfs_id);
+        shrinker->flags &= ~SHRINKER_REGISTERED;
+    } else {
+        shrinker_debugfs_name_free(shrinker);

We could remove shrinker_debugfs_name_free() calling from
shrinker_debugfs_detach(), then we could call
shrinker_debugfs_name_free() anyway, otherwise, it it a little
weird for me. And the srinker name is allocated from shrinker_alloc(),
so I think it it reasonable for shrinker_free() to free the
shrinker name.

OK, will do.


Thanks.

+    }
+
+    if (shrinker->flags & SHRINKER_MEMCG_AWARE)
+        unregister_memcg_shrinker(shrinker);
+    up_write(&shrinker_rwsem);
+
+    if (debugfs_entry)
+        shrinker_debugfs_remove(debugfs_entry, debugfs_id);
+
+    kfree(shrinker->nr_deferred);
+    shrinker->nr_deferred = NULL;
+
+    kfree(shrinker);
+}
+EXPORT_SYMBOL_GPL(shrinker_free);
+
  /*
   * Add a shrinker callback to be called from the vm.
   */
diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c
index e4ce509f619e..38452f539f40 100644
--- a/mm/shrinker_debug.c
+++ b/mm/shrinker_debug.c
@@ -193,6 +193,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;
+}

It it better to move both helpers to internal.h and mark them as inline
since both are very simple enough.

OK, will do.

Hi Andrew, below is the cleanup patch, which has a small conflict
with [PATCH v6 41/45]:

From 5bc2b77484f5cd4616e510158f91c8877bd033ad Mon Sep 17 00:00:00 2001
From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
Date: Mon, 18 Sep 2023 10:41:15 +0000
Subject: [PATCH] mm: shrinker: some cleanup

Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
---
 include/linux/shrinker.h | 14 ++++++++------
 mm/internal.h            | 17 ++++++++++++++---
 mm/shrinker.c            | 20 ++++++++++++--------
 mm/shrinker_debug.c      | 16 ----------------
 4 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 3f3fd9974ce5..f4a5249f00b2 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -88,16 +88,18 @@ struct shrinker {
 };
 #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */

-/* Flags */
-#define SHRINKER_REGISTERED	(1 << 0)
-#define SHRINKER_NUMA_AWARE	(1 << 1)
-#define SHRINKER_MEMCG_AWARE	(1 << 2)
+/* Internal flags */
+#define SHRINKER_REGISTERED	BIT(0)
+#define SHRINKER_ALLOCATED	BIT(1)
+
+/* Flags for users to use */
+#define SHRINKER_NUMA_AWARE	BIT(2)
+#define SHRINKER_MEMCG_AWARE	BIT(3)
 /*
  * 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 << 3)
-#define SHRINKER_ALLOCATED	(1 << 4)
+#define SHRINKER_NONSLAB	BIT(4)

 struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...);
 void shrinker_register(struct shrinker *shrinker);
diff --git a/mm/internal.h b/mm/internal.h
index b9a116dce28e..0f418a11c7a8 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1161,10 +1161,21 @@ unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg,
 			  int priority);

 #ifdef CONFIG_SHRINKER_DEBUG
+static inline 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;
+}
+
+static inline void shrinker_debugfs_name_free(struct shrinker *shrinker)
+{
+	kfree_const(shrinker->name);
+	shrinker->name = NULL;
+}
+
 extern int shrinker_debugfs_add(struct shrinker *shrinker);
-extern int shrinker_debugfs_name_alloc(struct shrinker *shrinker,
-				       const char *fmt, va_list ap);
-extern void shrinker_debugfs_name_free(struct shrinker *shrinker);
 extern struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker,
 					      int *debugfs_id);
 extern void shrinker_debugfs_remove(struct dentry *debugfs_entry,
diff --git a/mm/shrinker.c b/mm/shrinker.c
index 201211a67827..d1032a4d5684 100644
--- a/mm/shrinker.c
+++ b/mm/shrinker.c
@@ -572,18 +572,23 @@ struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...)

 	if (flags & SHRINKER_MEMCG_AWARE) {
 		err = prealloc_memcg_shrinker(shrinker);
-		if (err == -ENOSYS)
+		if (err == -ENOSYS) {
+			/* Memcg is not supported, fallback to non-memcg-aware shrinker. */
 			shrinker->flags &= ~SHRINKER_MEMCG_AWARE;
-		else if (err == 0)
-			goto done;
-		else
+			goto non_memcg;
+		}
+
+		if (err)
 			goto err_flags;
+
+		return shrinker;
 	}

+non_memcg:
 	/*
 	 * The nr_deferred is available on per memcg level for memcg aware
 	 * shrinkers, so only allocate nr_deferred in the following cases:
-	 *  - non memcg aware shrinkers
+	 *  - non-memcg-aware shrinkers
 	 *  - !CONFIG_MEMCG
 	 *  - memcg is disabled by kernel command line
 	 */
@@ -595,7 +600,6 @@ struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...)
 	if (!shrinker->nr_deferred)
 		goto err_flags;

-done:
 	return shrinker;

 err_flags:
@@ -634,10 +638,10 @@ void shrinker_free(struct shrinker *shrinker)
 		list_del(&shrinker->list);
 		debugfs_entry = shrinker_debugfs_detach(shrinker, &debugfs_id);
 		shrinker->flags &= ~SHRINKER_REGISTERED;
-	} else {
-		shrinker_debugfs_name_free(shrinker);
 	}

+	shrinker_debugfs_name_free(shrinker);
+
 	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
 		unregister_memcg_shrinker(shrinker);
 	up_write(&shrinker_rwsem);
diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c
index 38452f539f40..24aebe7c24cc 100644
--- a/mm/shrinker_debug.c
+++ b/mm/shrinker_debug.c
@@ -193,20 +193,6 @@ 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;
-}
-
int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...)
 {
 	struct dentry *entry;
@@ -255,8 +241,6 @@ struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker,

 	lockdep_assert_held(&shrinker_rwsem);

-	shrinker_debugfs_name_free(shrinker);
-
 	*debugfs_id = entry ? shrinker->debugfs_id : -1;
 	shrinker->debugfs_entry = NULL;

--
2.30.2


Thanks.

+
  int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...)
  {
      struct dentry *entry;
@@ -241,8 +255,7 @@ struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker,
      lockdep_assert_held(&shrinker_rwsem);
-    kfree_const(shrinker->name);
-    shrinker->name = NULL;
+    shrinker_debugfs_name_free(shrinker);
      *debugfs_id = entry ? shrinker->debugfs_id : -1;
      shrinker->debugfs_entry = NULL;




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux