Messages in this thread Patch in this message | | | Date | Mon, 18 Sep 2023 20:06:29 +0800 | Subject | Re: [PATCH v6 01/45] mm: shrinker: add infrastructure for dynamically allocating shrinker | From | Qi Zheng <> |
| |
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@bytedance.com> Date: Mon, 18 Sep 2023 10:41:15 +0000 Subject: [PATCH] mm: shrinker: some cleanup
Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- 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; >
| |