Re: [PATCH v5 6/6] mm: shrinkers: add scan interface for shrinker debugfs

From: Andrew Morton <akpm@linux-foundation.org>
To: Roman Gushchin <roman.gushchin@linux.dev>
Cc: linux-mm@kvack.org, Dave Chinner <dchinner@redhat.com>,
	linux-kernel@vger.kernel.org,
	Kent Overstreet <kent.overstreet@gmail.com>,
	Hillf Danton <hdanton@sina.com>,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	Muchun Song <songmuchun@bytedance.com>
Subject: Re: [PATCH v5 6/6] mm: shrinkers: add scan interface for shrinker debugfs
Date: Wed, 1 Jun 2022 14:23:51 -0700	[thread overview]
Message-ID: <20220601142351.5e04fea5586ca51898d8785f@linux-foundation.org> (raw)
In-Reply-To: <20220601032227.4076670-7-roman.gushchin@linux.dev>

On Tue, 31 May 2022 20:22:27 -0700 Roman Gushchin <roman.gushchin@linux.dev> wrote:

> Add a scan interface which allows to trigger scanning of a particular
> shrinker and specify memcg and numa node. It's useful for testing,
> debugging and profiling of a specific scan_objects() callback.
> Unlike alternatives (creating a real memory pressure and dropping
> caches via /proc/sys/vm/drop_caches) this interface allows to interact
> with only one shrinker at once. Also, if a shrinker is misreporting
> the number of objects (as some do), it doesn't affect scanning.
> 
> ..
>
> --- a/mm/shrinker_debug.c
> +++ b/mm/shrinker_debug.c
> @@ -99,6 +99,78 @@ static int shrinker_debugfs_count_show(struct seq_file *m, void *v)
>  }
>  DEFINE_SHOW_ATTRIBUTE(shrinker_debugfs_count);
>  
> +static int shrinker_debugfs_scan_open(struct inode *inode, struct file *file)
> +{
> +	file->private_data = inode->i_private;
> +	return nonseekable_open(inode, file);
> +}
> +
> +static ssize_t shrinker_debugfs_scan_write(struct file *file,
> +					   const char __user *buf,
> +					   size_t size, loff_t *pos)
> +{
> +	struct shrinker *shrinker = file->private_data;
> +	unsigned long nr_to_scan = 0, ino;
> +	struct shrink_control sc = {
> +		.gfp_mask = GFP_KERNEL,
> +	};
> +	struct mem_cgroup *memcg = NULL;
> +	int nid;
> +	char kbuf[72];
> +	int read_len = size < (sizeof(kbuf) - 1) ? size : (sizeof(kbuf) - 1);

size_t or ulong would be more appropriate.

> +	ssize_t ret;
> +
> +	if (copy_from_user(kbuf, buf, read_len))
> +		return -EFAULT;
> +	kbuf[read_len] = '\0';
> +
> +	if (sscanf(kbuf, "%lu %d %lu", &ino, &nid, &nr_to_scan) < 2)

Was it intentional to permit more than three args?

> +		return -EINVAL;
> +
> +	if (nid < 0 || nid >= nr_node_ids)
> +		return -EINVAL;
> +
> +	if (nr_to_scan == 0)
> +		return size;
> +
> +	if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> +		memcg = mem_cgroup_get_from_ino(ino);
> +		if (!memcg || IS_ERR(memcg))
> +			return -ENOENT;
> +
> +		if (!mem_cgroup_online(memcg)) {
> +			mem_cgroup_put(memcg);
> +			return -ENOENT;
> +		}
> +	} else if (ino != 0) {
> +		return -EINVAL;
> +	}
> +
> +	ret = down_read_killable(&shrinker_rwsem);
> +	if (ret) {
> +		mem_cgroup_put(memcg);
> +		return ret;
> +	}
> +
> +	sc.nid = nid;
> +	sc.memcg = memcg;
> +	sc.nr_to_scan = nr_to_scan;
> +	sc.nr_scanned = nr_to_scan;
> +
> +	shrinker->scan_objects(shrinker, &sc);
> +
> +	up_read(&shrinker_rwsem);
> +	mem_cgroup_put(memcg);
> +
> +	return size;
> +}