Re: [PATCH v3 3/4] mm/page_owner: add NUMA node filter with nodelist support

["zhen.ni" <zhen.ni@easystack.cn>]

在 2026/4/29 09:28, SeongJae Park 写道:
> On Tue, 28 Apr 2026 15:11:11 +0800 Zhen Ni <zhen.ni@easystack.cn> wrote:
> 
>> Add NUMA node filtering functionality to page_owner to allow
>> filtering pages by specific NUMA node(s) using nodelist format.
>>
>> The filter allows users to focus on pages from specific NUMA nodes,
>> which is useful for NUMA-aware memory allocation analysis and debugging.
>>
>> Supported input formats:
>> - Single node: echo "2" > nid
>> - Multiple nodes: echo "0,2,3" > nid
>> - Node range: echo "0-3" > nid
>> - Mixed format: echo "0,2-4,7" > nid
>> - Disable filter: echo "-1" > nid
>>
>> Link: https://lore.kernel.org/linux-mm/20260417154638.22370-4-zhen.ni@easystack.cn/
>> Link: https://lore.kernel.org/linux-mm/20260419155540.376847-4-zhen.ni@easystack.cn/
> 
> Seems the above two links are for v1 and v2 of this patch.  I think putting
> those with the context at commentary area [1] could be useful.
> 
Good suggestion.
>> Suggested-by: Zi Yan <ziy@nvidia.com>
>> Signed-off-by: Zhen Ni <zhen.ni@easystack.cn>
>> ---
> [...]
>> diff --git a/mm/page_owner.c b/mm/page_owner.c
>> index 6d87b6948cfa..e674a374669a 100644
>> --- a/mm/page_owner.c
>> +++ b/mm/page_owner.c
>> @@ -685,6 +685,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>   	struct page_ext *page_ext;
>>   	struct page_owner *page_owner;
>>   	depot_stack_handle_t handle;
>> +	nodemask_t mask;
>>   
>>   	if (!static_branch_unlikely(&page_owner_inited))
>>   		return -EINVAL;
>> @@ -698,6 +699,8 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>   	while (!pfn_valid(pfn) && (pfn & (MAX_ORDER_NR_PAGES - 1)) != 0)
>>   		pfn++;
>>   
>> +	mask = owner_filter.nid_mask;
>> +
> 
> READ_ONCE() was used for owner_filter.print_mode.  Should nid_mask also read
> using READ_ONCE()?
> 
The reason is that `owner_filter.nid_mask` is a nodemask_t, which is a
128-byte structure. READ_ONCE() only supports types up to 8 bytes and 
will trigger a compile-time assertion failure for larger structures.

This was actually an issue in v2 - the AI review tool (sashiko.dev) and
Andrew both caught the compilation error with READ_ONCE/WRITE_ONCE on
nodemask_t, so v3 removed them.
>>   	/* Find an allocated page */
>>   	for (; pfn < max_pfn; pfn++) {
>>   		/*
>> @@ -730,6 +733,14 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>   		if (unlikely(!page_ext))
>>   			continue;
>>   
>> +		/* NUMA node filter using bitmask */
>> +		if (!nodes_empty(mask)) {
>> +			int nid = page_to_nid(page);
>> +
>> +			if (!node_isset(nid, mask))
>> +				goto ext_put_continue;
>> +		}
>> +
>>   		/*
>>   		 * Some pages could be missed by concurrent allocation or free,
>>   		 * because we don't hold the zone lock.
>> @@ -1009,6 +1020,75 @@ DEFINE_SIMPLE_ATTRIBUTE(page_owner_print_mode_fops,
>>   			&page_owner_print_mode_get,
>>   			&page_owner_print_mode_set, "%lld");
>>   
>> +static ssize_t nid_filter_write(struct file *file,
>> +				 const char __user *buf,
>> +				 size_t count, loff_t *ppos)
>> +{
>> +	char *kbuf;
>> +	nodemask_t mask;
>> +	int ret;
>> +	int val;
>> +
>> +	/*
>> +	 * Limit input size to handle worst-case nodelist (all nodes).
>> +	 * Worst case per node: ",NNNNN" (comma + 5-digit node number) = 6 bytes.
>> +	 * Formula: 100 bytes overhead + 6 * MAX_NUMNODES
>> +	 */
>> +	if (count > (100 + 6 * MAX_NUMNODES))
>> +		return -EINVAL;
>> +
>> +	kbuf = kmalloc(count + 1, GFP_KERNEL);
>> +	if (!kbuf)
>> +		return -ENOMEM;
>> +
>> +	if (copy_from_user(kbuf, buf, count)) {
>> +		ret = -EFAULT;
>> +		goto out_free;
>> +	}
>> +	kbuf[count] = '\0';
>> +
>> +	/* Support: "-1" to clear, or nodelist format like "0", "0,2", "0-3" */
>> +	if (kstrtoint(kbuf, 10, &val) == 0 && val == -1)
>> +		nodes_clear(mask);
>> +	else if (nodelist_parse(kbuf, mask)) {
>> +		ret = -EINVAL;
>> +		goto out_free;
>> +	}
> 
> Doesn't empty string input to nodelist_parse() clears the mask?  Can't it be
> reused?
> 
Yes, empty input (echo > nid) works because nodelist_parse() handles it
correctly. However, nodelist_parse() - which is implemented via
bitmap_parselist() - cannot handle "-1" as it's not a valid range format
and would return an error. The explicit "-1" check is necessary to 
support `echo "-1" > nid` without returning an error.

So the "-1" check handles a case that nodelist_parse() cannot handle.
>> +
>> +	owner_filter.nid_mask = mask;
>> +	ret = count;
>> +
>> +out_free:
>> +	kfree(kbuf);
>> +	return ret;
>> +}
>> +
>> +static int nid_filter_show(struct seq_file *m, void *v)
>> +{
>> +	nodemask_t mask = owner_filter.nid_mask;
>> +
>> +	if (nodes_empty(mask))
>> +		seq_puts(m, "-1\n");
>> +	else
>> +		seq_printf(m, "%*pbl\n", nodemask_pr_args(&mask));
>> +
>> +	return 0;
>> +}
>> +
>> +static int nid_filter_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, nid_filter_show, NULL);
>> +}
>> +
>> +static const struct file_operations nid_filter_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.open		= nid_filter_open,
>> +	.read		= seq_read,
>> +	.llseek		= seq_lseek,
>> +	.write		= nid_filter_write,
>> +	.release	= single_release,
>> +};
>> +
>>   
>>   static int __init pageowner_init(void)
>>   {
>> @@ -1024,6 +1104,8 @@ static int __init pageowner_init(void)
>>   	filter_dir = debugfs_create_dir("page_owner_filter", NULL);
>>   	debugfs_create_file("print_mode", 0600, filter_dir, NULL,
>>   			    &page_owner_print_mode_fops);
>> +	debugfs_create_file("nid", 0600, filter_dir, NULL,
>> +			    &nid_filter_fops);
> 
> Why don't you use 'page_owner_' prefix like other fops, for consistency?
> 
For consistency with the other file_operations
in this module (page_owner_fops, page_owner_threshold_fops,
page_owner_print_mode_fops), I'll rename nid_filter_fops to
page_owner_nid_filter_fops.

I'll incorporate these improvements in the next version.

Thanks for the detailed review!
>>   
>>   	dir = debugfs_create_dir("page_owner_stacks", NULL);
>>   	debugfs_create_file("show_stacks", 0400, dir,
>> -- 
>> 2.20.1
> 
> [1] https://docs.kernel.org/process/submitting-patches.html#commentary
> 
> 
> Thanks,
> SJ
> 
> 
Best regards,
Zhen Ni