Re: [PATCH v6 01/45] mm: shrinker: add infrastructure for dynamically allocating shrinker
From: Muchun Song
Date: Mon Sep 18 2023 - 22:37:33 EST
> On Sep 18, 2023, at 20:06, Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx> wrote:
>
>
>
> 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>
Reviewed-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
Thanks.