Introduce some helpers for dynamically allocating shrinker instance,
and their uses are as follows:
1. shrinker_alloc_and_init()
Used to allocate and initialize a shrinker instance, the priv_data
parameter is used to pass the pointer of the previously embedded
structure of the shrinker instance.
2. shrinker_free()
Used to free the shrinker instance when the registration of shrinker
fails.
3. unregister_and_free_shrinker()
Used to unregister and free the shrinker instance, and the kfree()
will be changed to kfree_rcu() later.
Signed-off-by: Qi Zheng <[email protected]>
---
include/linux/shrinker.h | 12 ++++++++++++
mm/vmscan.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 43e6fcabbf51..8e9ba6fa3fcc 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -107,6 +107,18 @@ extern void unregister_shrinker(struct shrinker *shrinker);
extern void free_prealloced_shrinker(struct shrinker *shrinker);
extern void synchronize_shrinkers(void);
+typedef unsigned long (*count_objects_cb)(struct shrinker *s,
+ struct shrink_control *sc);
+typedef unsigned long (*scan_objects_cb)(struct shrinker *s,
+ struct shrink_control *sc);
+
+struct shrinker *shrinker_alloc_and_init(count_objects_cb count,
+ scan_objects_cb scan, long batch,
+ int seeks, unsigned flags,
+ void *priv_data);
+void shrinker_free(struct shrinker *shrinker);
+void unregister_and_free_shrinker(struct shrinker *shrinker);
+
#ifdef CONFIG_SHRINKER_DEBUG
extern int shrinker_debugfs_add(struct shrinker *shrinker);
extern struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker,
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 45d17c7cc555..64ff598fbad9 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -809,6 +809,41 @@ void unregister_shrinker(struct shrinker *shrinker)
}
EXPORT_SYMBOL(unregister_shrinker);
+struct shrinker *shrinker_alloc_and_init(count_objects_cb count,
+ scan_objects_cb scan, long batch,
+ int seeks, unsigned flags,
+ void *priv_data)
+{
+ struct shrinker *shrinker;
+
+ shrinker = kzalloc(sizeof(struct shrinker), GFP_KERNEL);
+ if (!shrinker)
+ return NULL;
+
+ shrinker->count_objects = count;
+ shrinker->scan_objects = scan;
+ shrinker->batch = batch;
+ shrinker->seeks = seeks;
+ shrinker->flags = flags;
+ shrinker->private_data = priv_data;
+
+ return shrinker;
+}
+EXPORT_SYMBOL(shrinker_alloc_and_init);
+
+void shrinker_free(struct shrinker *shrinker)
+{
+ kfree(shrinker);
+}
+EXPORT_SYMBOL(shrinker_free);
+
+void unregister_and_free_shrinker(struct shrinker *shrinker)
+{
+ unregister_shrinker(shrinker);
+ kfree(shrinker);
+}
+EXPORT_SYMBOL(unregister_and_free_shrinker);
+
/**
* synchronize_shrinkers - Wait for all running shrinkers to complete.
*
--
2.30.2
Hi Dave,
On 2023/6/23 14:12, Dave Chinner wrote:
> On Thu, Jun 22, 2023 at 04:53:08PM +0800, Qi Zheng wrote:
>> Introduce some helpers for dynamically allocating shrinker instance,
>> and their uses are as follows:
>>
>> 1. shrinker_alloc_and_init()
>>
>> Used to allocate and initialize a shrinker instance, the priv_data
>> parameter is used to pass the pointer of the previously embedded
>> structure of the shrinker instance.
>>
>> 2. shrinker_free()
>>
>> Used to free the shrinker instance when the registration of shrinker
>> fails.
>>
>> 3. unregister_and_free_shrinker()
>>
>> Used to unregister and free the shrinker instance, and the kfree()
>> will be changed to kfree_rcu() later.
>>
>> Signed-off-by: Qi Zheng <[email protected]>
>> ---
>> include/linux/shrinker.h | 12 ++++++++++++
>> mm/vmscan.c | 35 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 47 insertions(+)
>>
>> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
>> index 43e6fcabbf51..8e9ba6fa3fcc 100644
>> --- a/include/linux/shrinker.h
>> +++ b/include/linux/shrinker.h
>> @@ -107,6 +107,18 @@ extern void unregister_shrinker(struct shrinker *shrinker);
>> extern void free_prealloced_shrinker(struct shrinker *shrinker);
>> extern void synchronize_shrinkers(void);
>>
>> +typedef unsigned long (*count_objects_cb)(struct shrinker *s,
>> + struct shrink_control *sc);
>> +typedef unsigned long (*scan_objects_cb)(struct shrinker *s,
>> + struct shrink_control *sc);
>> +
>> +struct shrinker *shrinker_alloc_and_init(count_objects_cb count,
>> + scan_objects_cb scan, long batch,
>> + int seeks, unsigned flags,
>> + void *priv_data);
>> +void shrinker_free(struct shrinker *shrinker);
>> +void unregister_and_free_shrinker(struct shrinker *shrinker);
>
> Hmmmm. Not exactly how I envisioned this to be done.
>
> Ok, this will definitely work, but I don't think it is an
> improvement. It's certainly not what I was thinking of when I
> suggested dynamically allocating shrinkers.
>
> The main issue is that this doesn't simplify the API - it expands it
> and creates a minefield of old and new functions that have to be
> used in exactly the right order for the right things to happen.
>
> What I was thinking of was moving the entire shrinker setup code
> over to the prealloc/register_prepared() algorithm, where the setup
> is already separated from the activation of the shrinker.
>
> That is, we start by renaming prealloc_shrinker() to
> shrinker_alloc(), adding a flags field to tell it everything that it
> needs to alloc (i.e. the NUMA/MEMCG_AWARE flags) and having it
> returned a fully allocated shrinker ready to register. Initially
> this also contains an internal flag to say the shrinker was
> allocated so that unregister_shrinker() knows to free it.
>
> The caller then fills out the shrinker functions, seeks, etc. just
> like the do now, and then calls register_shrinker_prepared() to make
> the shrinker active when it wants to turn it on.
>
> When it is time to tear down the shrinker, no API needs to change.
> unregister_shrinker() does all the shutdown and frees all the
> internal memory like it does now. If the shrinker is also marked as
> allocated, it frees the shrinker via RCU, too.
>
> Once everything is converted to this API, we then remove
> register_shrinker(), rename register_shrinker_prepared() to
> shrinker_register(), rename unregister_shrinker to
> shrinker_unregister(), get rid of the internal "allocated" flag
> and always free the shrinker.
IIUC, you mean that we also need to convert the original statically
defined shrinker instances to dynamically allocated.
I think this is a good idea, it helps to simplify the APIs and also
remove special handling for case a and b (mentioned in cover letter).
>
> At the end of the patchset, every shrinker should be set
> up in a manner like this:
>
>
> sb->shrinker = shrinker_alloc(SHRINKER_MEMCG_AWARE|SHRINKER_NUMA_AWARE,
> "sb-%s", type->name);
> if (!sb->shrinker)
> return -ENOMEM;
>
> sb->shrinker->count_objects = super_cache_count;
> sb->shrinker->scan_objects = super_cache_scan;
> sb->shrinker->batch = 1024;
> sb->shrinker->private = sb;
>
> .....
>
> shrinker_register(sb->shrinker);
>
> And teardown is just a call to shrinker_unregister(sb->shrinker)
> as it is now.
>
> i.e. the entire shrinker regsitration API is now just three
> functions, down from the current four, and much simpler than the
> the seven functions this patch set results in...
>
> 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
> setup.
Got it. And totally agree.
I will do it in the v2.
Thanks,
Qi
>
> -Dave.