Re: [PATCH v2 5/7] mm: provide shrinkers with names

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Apr 23, 2022 at 09:46:25AM +0200, Christophe JAILLET wrote:
> Hi,
> 
> Le 22/04/2022 à 22:26, Roman Gushchin a écrit :
> > Currently shrinkers are anonymous objects. For debugging purposes they
> > can be identified by count/scan function names, but it's not always
> > useful: e.g. for superblock's shrinkers it's nice to have at least
> > an idea of to which superblock the shrinker belongs.
> > 
> > This commit adds names to shrinkers. register_shrinker() and
> > prealloc_shrinker() functions are extended to take a format and
> > arguments to master a name. If CONFIG_SHRINKER_DEBUG is on,
> > the name is saved until the corresponding debugfs object is created,
> > otherwise it's simple ignored.
> > 
> > After this change the shrinker debugfs directory looks like:
> >    $ cd /sys/kernel/debug/shrinker/
> >    $ ls
> >      dqcache-16          sb-cgroup2-30    sb-hugetlbfs-33  sb-proc-41       sb-selinuxfs-22  sb-tmpfs-40    sb-zsmalloc-19
> >      kfree_rcu-0         sb-configfs-23   sb-iomem-12      sb-proc-44       sb-sockfs-8      sb-tmpfs-42    shadow-18
> >      sb-aio-20           sb-dax-11        sb-mqueue-21     sb-proc-45       sb-sysfs-26      sb-tmpfs-43    thp_deferred_split-10
> >      sb-anon_inodefs-15  sb-debugfs-7     sb-nsfs-4        sb-proc-47       sb-tmpfs-1       sb-tmpfs-46    thp_zero-9
> >      sb-bdev-3           sb-devpts-28     sb-pipefs-14     sb-pstore-31     sb-tmpfs-27      sb-tmpfs-49    xfs_buf-37
> >      sb-bpf-32           sb-devtmpfs-5    sb-proc-25       sb-rootfs-2      sb-tmpfs-29      sb-tracefs-13  xfs_inodegc-38
> >      sb-btrfs-24         sb-hugetlbfs-17  sb-proc-39       sb-securityfs-6  sb-tmpfs-35      sb-xfs-36      zspool-34
> > 
> > Signed-off-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> > ---
> 
> [...]
> 
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 59f91e392a2a..1b326b93155c 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -7383,7 +7383,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> >   	conf->shrinker.count_objects = raid5_cache_count;
> >   	conf->shrinker.batch = 128;
> >   	conf->shrinker.flags = 0;
> > -	if (register_shrinker(&conf->shrinker)) {
> > +	if (register_shrinker(&conf->shrinker, "md")) {
> 
> Based on pr_warn below, does it make sense to have something like:
>   register_shrinker(&conf->shrinker, "md-%s", mdname(mddev))
> 
> >   		pr_warn("md/raid:%s: couldn't register shrinker.\n",
> >   			mdname(mddev));
> >   		goto abort;

Good idea, will do, thanks!

> 
> [...]
> 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 121a54a1602b..6025cfda4932 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -613,7 +613,7 @@ static unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru,
> >   /*
> >    * Add a shrinker callback to be called from the vm.
> >    */
> > -int prealloc_shrinker(struct shrinker *shrinker)
> > +static int __prealloc_shrinker(struct shrinker *shrinker)
> >   {
> >   	unsigned int size;
> >   	int err;
> > @@ -637,6 +637,34 @@ int prealloc_shrinker(struct shrinker *shrinker)
> >   	return 0;
> >   }
> > +#ifdef CONFIG_SHRINKER_DEBUG
> > +int prealloc_shrinker(struct shrinker *shrinker, const char *fmt, ...)
> > +{
> > +	int err;
> > +	char buf[64];
> > +	va_list ap;
> > +
> > +	va_start(ap, fmt);
> > +	vscnprintf(buf, sizeof(buf), fmt, ap);
> > +	va_end(ap);
> > +
> > +	shrinker->name = kstrdup(buf, GFP_KERNEL);
> > +	if (!shrinker->name)
> > +		return -ENOMEM;
> 
> use kvasprintf_const() (and kfree_const() elsewhere) to simplify code and
> take advantage of kstrdup_const() in most cases?

Sure, good point.

> 
> > +
> > +	err = __prealloc_shrinker(shrinker);
> > +	if (err)
> > +		kfree(shrinker->name);
> > +
> > +	return err;
> > +}
> > +#else
> > +int prealloc_shrinker(struct shrinker *shrinker, const char *fmt, ...)
> > +{
> > +	return __prealloc_shrinker(shrinker);
> > +}
> > +#endif
> > +
> >   void free_prealloced_shrinker(struct shrinker *shrinker)
> >   {
> >   	if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> > @@ -648,6 +676,9 @@ void free_prealloced_shrinker(struct shrinker *shrinker)
> >   	kfree(shrinker->nr_deferred);
> >   	shrinker->nr_deferred = NULL;
> > +#ifdef CONFIG_SHRINKER_DEBUG
> > +	kfree(shrinker->name);
> > +#endif
> >   }
> >   void register_shrinker_prepared(struct shrinker *shrinker)
> > @@ -659,15 +690,38 @@ void register_shrinker_prepared(struct shrinker *shrinker)
> >   	up_write(&shrinker_rwsem);
> >   }
> > -int register_shrinker(struct shrinker *shrinker)
> > +static int __register_shrinker(struct shrinker *shrinker)
> >   {
> > -	int err = prealloc_shrinker(shrinker);
> > +	int err = __prealloc_shrinker(shrinker);
> >   	if (err)
> >   		return err;
> >   	register_shrinker_prepared(shrinker);
> >   	return 0;
> >   }
> > +
> > +#ifdef CONFIG_SHRINKER_DEBUG
> > +int register_shrinker(struct shrinker *shrinker, const char *fmt, ...)
> > +{
> > +	char buf[64];
> > +	va_list ap;
> > +
> > +	va_start(ap, fmt);
> > +	vscnprintf(buf, sizeof(buf), fmt, ap);
> > +	va_end(ap);
> > +
> > +	shrinker->name = kstrdup(buf, GFP_KERNEL);
> > +	if (!shrinker->name)
> > +		return -ENOMEM;
> > +
> 
> same as above.
> 
> > +	return __register_shrinker(shrinker);
> 
> Missing error handling and freeing of shrinker->name as done in
> prealloc_shrinker()?

Will check in the next version.

Thank you for taking a look!

Roman




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux