Messages in this thread |  | | From | Christophe JAILLET <> | Subject | Re: [PATCH v2 5/7] mm: provide shrinkers with names | Date | Sat, 23 Apr 2022 09:46:25 +0200 |
| |
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@linux.dev> > ---
[...]
> 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;
[...]
> 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?
> + > + 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()?
CJ
> +} > +#else > +int register_shrinker(struct shrinker *shrinker, const char *fmt, ...) > +{ > + return __register_shrinker(shrinker); > +} > +#endif > EXPORT_SYMBOL(register_shrinker); > > /*
[...]
|  |