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

[Kent Overstreet <kent.overstreet@linux.dev>]

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.