Re: [PATCH 02/10] mm: shrinker: Add a .to_text() method for shrinkers

[Qi Zheng <zhengqi.arch@bytedance.com>]

On 2024/8/26 11:04, Kent Overstreet wrote:
> On Mon, Aug 26, 2024 at 11:01:50AM GMT, Qi Zheng wrote:
>>
>>
>> On 2024/8/25 03:10, Kent Overstreet wrote:
>>> This adds a new callback method to shrinkers which they can use to
>>> describe anything relevant to memory reclaim about their internal state,
>>> for example object dirtyness.
>>>
>>> This patch also adds shrinkers_to_text(), which reports on the top 10
>>> shrinkers - by object count - in sorted order, to be used in OOM
>>> reporting.
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Qi Zheng <zhengqi.arch@bytedance.com>
>>> Cc: Roman Gushchin <roman.gushchin@linux.dev>
>>> Cc: linux-mm@kvack.org
>>> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
>>> ---
>>>    include/linux/shrinker.h |  7 +++-
>>>    mm/shrinker.c            | 73 +++++++++++++++++++++++++++++++++++++++-
>>>    2 files changed, 78 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
>>> index 1a00be90d93a..6193612617a1 100644
>>> --- a/include/linux/shrinker.h
>>> +++ b/include/linux/shrinker.h
>>> @@ -24,6 +24,8 @@ struct shrinker_info {
>>>    	struct shrinker_info_unit *unit[];
>>>    };
>>> +struct seq_buf;
>>> +
>>>    /*
>>>     * This struct is used to pass information from page reclaim to the shrinkers.
>>>     * We consolidate the values for easier extension later.
>>> @@ -80,10 +82,12 @@ struct shrink_control {
>>>     * @flags determine the shrinker abilities, like numa awareness
>>>     */
>>>    struct shrinker {
>>> +	const char *name;
>>>    	unsigned long (*count_objects)(struct shrinker *,
>>>    				       struct shrink_control *sc);
>>>    	unsigned long (*scan_objects)(struct shrinker *,
>>>    				      struct shrink_control *sc);
>>> +	void (*to_text)(struct seq_buf *, struct shrinker *);
>>>    	long batch;	/* reclaim batch size, 0 = default */
>>>    	int seeks;	/* seeks to recreate an obj */
>>> @@ -110,7 +114,6 @@ struct shrinker {
>>>    #endif
>>>    #ifdef CONFIG_SHRINKER_DEBUG
>>>    	int debugfs_id;
>>> -	const char *name;
>>>    	struct dentry *debugfs_entry;
>>>    #endif
>>>    	/* objs pending delete, per node */
>>> @@ -135,6 +138,8 @@ __printf(2, 3)
>>>    struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...);
>>>    void shrinker_register(struct shrinker *shrinker);
>>>    void shrinker_free(struct shrinker *shrinker);
>>> +void shrinker_to_text(struct seq_buf *, struct shrinker *);
>>> +void shrinkers_to_text(struct seq_buf *);
>>>    static inline bool shrinker_try_get(struct shrinker *shrinker)
>>>    {
>>> diff --git a/mm/shrinker.c b/mm/shrinker.c
>>> index dc5d2a6fcfc4..ad52c269bb48 100644
>>> --- a/mm/shrinker.c
>>> +++ b/mm/shrinker.c
>>> @@ -1,8 +1,9 @@
>>>    // SPDX-License-Identifier: GPL-2.0
>>>    #include <linux/memcontrol.h>
>>> +#include <linux/rculist.h>
>>>    #include <linux/rwsem.h>
>>> +#include <linux/seq_buf.h>
>>>    #include <linux/shrinker.h>
>>> -#include <linux/rculist.h>
>>>    #include <trace/events/vmscan.h>
>>>    #include "internal.h"
>>> @@ -807,3 +808,73 @@ void shrinker_free(struct shrinker *shrinker)
>>>    	call_rcu(&shrinker->rcu, shrinker_free_rcu_cb);
>>>    }
>>>    EXPORT_SYMBOL_GPL(shrinker_free);
>>> +
>>> +void shrinker_to_text(struct seq_buf *out, struct shrinker *shrinker)
>>> +{
>>> +	struct shrink_control sc = { .gfp_mask = GFP_KERNEL, };
>>> +
>>> +	seq_buf_puts(out, shrinker->name);
>>> +	seq_buf_printf(out, " objects: %lu\n", shrinker->count_objects(shrinker, &sc));
>>> +
>>> +	if (shrinker->to_text) {
>>> +		shrinker->to_text(out, shrinker);
>>> +		seq_buf_puts(out, "\n");
>>> +	}
>>> +}
>>> +
>>> +/**
>>> + * shrinkers_to_text - Report on shrinkers with highest usage
>>> + *
>>> + * This reports on the top 10 shrinkers, by object counts, in sorted order:
>>> + * intended to be used for OOM reporting.
>>> + */
>>> +void shrinkers_to_text(struct seq_buf *out)
>>> +{
>>> +	struct shrinker *shrinker;
>>> +	struct shrinker_by_mem {
>>> +		struct shrinker	*shrinker;
>>> +		unsigned long	mem;
>>> +	} shrinkers_by_mem[10];
>>> +	int i, nr = 0;
>>> +
>>> +	if (!mutex_trylock(&shrinker_mutex)) {
>>> +		seq_buf_puts(out, "(couldn't take shrinker lock)");
>>> +		return;
>>> +	}
>>
>> I remember I pointed out that the RCU + refcount method should be used
>> here. Otherwise you will block other shrinkers from
>> registering/unregistering, etc.
> 
> The more complex iteration isn't needed here - this is a slowpath
> function and we're not doing anything blocking inside it.

The shrinker list may be very long, and can you guarantee that shrinkers
other than bcachefs won't do anything blocking? ;)