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

SeongJae Park
2026-04-29T14:56:54Z

On Wed, 29 Apr 2026 17:03:56 +0800 "zhen.ni" <zhen.ni@easystack.cn> wrote:

>
>
> 在 2026/4/29 09:28, SeongJae Park 写道:
> > On Tue, 28 Apr 2026 15:11:11 +0800 Zhen Ni <zhen.ni@easystack.cn> wrote:
[...]
> >> @@ -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.

Thank you for kindly sharing the context. Now I understand why READ_ONCE()
cannot be used. But, is plain load/store safe enough for nodemask_t?
Shouldn't it still be protected against races?

[...]
> >> +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.

Thank you for kindly explaining the reason. But, do we really need to support
"-1" input? Couldn't we just redefine the interface?

[...]
> >> + 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.

Thank you for kindly accepting my humble suggestions.

>
> Thanks for the detailed review!

Thank you for sharing this nice work, too!

Thanks,
SJ …