Re: [PATCH v5 27/36] mm: add NODE_PRIVATE_CAP_USER_NUMA for userland numa controls

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Relative node indices are incorrectly treated as absolute node IDs for private node checking in mpol_set_nodemask.
- [Medium] Hugetlb folio migration to private nodes unconditionally fails because the hugetlb allocator ignores ALLOC_ZONELIST_PRIVATE.

Pre-existing issues:
- [High] Returning ACTION_AGAIN on pte_offset_map_lock() failure causes an infinite loop in the page table walker.
--

--- Patch [27]: [PATCH v5 27/36] mm: add NODE_PRIVATE_CAP_USER_NUMA for userland numa controls ---
commit 90c010ac54e1befaa9a30ca8c81fdac4b3524857
Author: Gregory Price <[email protected]>

mm: add NODE_PRIVATE_CAP_USER_NUMA for userland numa controls

This commit provides a mechanism to opt private nodes into userland numa
management. It adds node_allows_user_numa() to encapsulate whether a node
supports userland NUMA controls and modifies the memory policy and migration
interfaces to support these nodes.

> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index a3ffb09897489..fe42a510590a2 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -434,13 +434,14 @@ static int mpol_set_nodemask(struct mempolicy *pol,
>  
>  	/*
>  	 * Private nodes are not in cpuset.mems, so they're always stripped.
> -	 * Driver-allocated policies will already have MPOL_F_PRIVATE set,
> -	 * if that's the case, add back in the requested set of private nodes.
> +	 * Driver-allocated policies (MPOL_F_PRIVATE) and CAP_USER_NUMA private
> +	 * nodes should be added back into the nodemask.
>  	 */
>  	for_each_node_mask(nid, *nodes) {
>  		if (!node_is_private(nid))
>  			continue;
> -		if (pol->flags & MPOL_F_PRIVATE)
> +		if ((pol->flags & MPOL_F_PRIVATE) ||
> +		    node_allows_user_numa(nid))
>  			node_set(nid, nsc->mask2);
>  	}

[Severity: Medium]
When MPOL_F_RELATIVE_NODES is set, the *nodes mask contains relative indices 
instead of absolute node IDs. Does this loop incorrectly evaluate those 
relative indices as absolute node IDs when calling node_is_private(nid)?

If the relative index happens to match an absolute private node ID, it appears
that node could be erroneously added to the final policy mask.

[ ... ]

> @@ -752,7 +753,8 @@ static int queue_folios_pte_range(pmd_t *pmd, unsigned long addr,
>  			continue;
>  		}
>  		folio = vm_normal_folio(vma, addr, ptent);
> -		if (!folio || folio_is_private_managed(folio))
> +		if (!folio || folio_is_zone_device(folio) ||
> +		    !node_allows_user_numa(folio_nid(folio)))
>  			continue;
>  		if (folio_test_large(folio) && max_nr != 1)
>  			nr = folio_pte_batch(folio, pte, ptent, max_nr);

[Severity: High]
This is a pre-existing issue, but earlier in queue_folios_pte_range() we
return ACTION_AGAIN if pte_offset_map_lock() fails:

queue_folios_pte_range()
    ...
    mapped_pte = pte = pte_offset_map_lock(walk->mm, pmd, addr, &ptl);
    if (!pte) {
        walk->action = ACTION_AGAIN;
        return 0;
    }

Does setting ACTION_AGAIN cause an infinite retry loop in the pagewalker 
when pte_offset_map_lock() fails non-transiently, such as when encountering
migration entries?

> diff --git a/mm/migrate.c b/mm/migrate.c
> index d20674c07b947..b548d79352a38 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2231,7 +2231,8 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private)
>  	if (is_highmem_idx(zidx) || zidx == ZONE_MOVABLE)
>  		gfp_mask |= __GFP_HIGHMEM;
>  
> -	return __folio_alloc(gfp_mask, order, nid, mtc->nmask, ALLOC_DEFAULT);
> +	return __folio_alloc(gfp_mask, order, nid, mtc->nmask,
> +			     mtc->alloc_flags);
>  }

[Severity: Medium]
This correctly updates the standard folio allocation to use mtc->alloc_flags,
but earlier in alloc_migration_target(), the hugetlb allocation path does
not pass this flag:

    if (folio_test_hugetlb(src)) {
        ...
        return alloc_hugetlb_folio_nodemask(h, nid, mtc->nmask, gfp_mask,
                                            htlb_allow_alloc_fallback(mtc->reason));
    }

Will hugetlb migration to private nodes fail unconditionally because it misses
the ALLOC_ZONELIST_PRIVATE flag needed by the hugetlb allocator?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=27