Re: [PATCH v6 01/45] mm: shrinker: add infrastructure for dynamically allocating shrinker
From: Qi Zheng
Date: Mon Sep 18 2023 - 08:09:05 EST
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;