Re: [RFC 1/7] drm/ttm/pool: Add ttm_pool_alloc_node() for caller-selected NUMA node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/ttm: high-order alloc failure on memoryless nodes in ttm_pool_nid()
- [Low] drm/ttm: code duplication and doc mismatch in ttm_pool_alloc_node()
--

commit 7c445098356d9efa9b5af93394f82d04655f28ad
Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

drm/ttm/pool: Add ttm_pool_alloc_node() for caller-selected NUMA node

This commit adds ttm_pool_alloc_node() to allow callers to specify a NUMA
node when allocating backing pages for a ttm_tt object, while rejecting
explicit NUMA node requests on pools that use dma_alloc_attrs().

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 1bf37023fed66..54d2f6b5c229c 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -158,9 +158,9 @@ static int ttm_pool_nid(struct ttm_pool *pool)
>  	return nid;
>  }
>  
> -/* Allocate pages of size 1 << order with the given gfp_flags */
> +/* Allocate pages of size 1 << order with the given gfp_flags on node @nid */
>  static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
> -					unsigned int order)
> +					unsigned int order, int nid)
>  {
>  	const unsigned int beneficial_order = ttm_pool_beneficial_order(pool);
>  	unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
> @@ -189,7 +189,7 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
>  	}
>  
>  	if (!ttm_pool_uses_dma_alloc(pool)) {
> -		p = alloc_pages_node(pool->nid, gfp_flags, order);
> +		p = alloc_pages_node(nid, gfp_flags, order);

[Severity: Medium]
Can resolving NUMA_NO_NODE via ttm_pool_nid() before calling
alloc_pages_node() cause higher-order allocations to fail on memoryless CPU
NUMA nodes (CONFIG_HAVE_MEMORYLESS_NODES)?

Previously, ttm_pool_alloc_page() passed pool->nid directly to
alloc_pages_node(). When pool->nid was NUMA_NO_NODE,
alloc_pages_node_noprof() translated NUMA_NO_NODE to numa_mem_id() (the
nearest NUMA node with memory).

With this change, ttm_pool_alloc(), ttm_pool_alloc_node() (when nid is
NUMA_NO_NODE), and ttm_pool_restore_and_alloc() resolve nid up front via
ttm_pool_nid():

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_nid() {
	int nid = NUMA_NO_NODE;
	if (pool)
		nid = pool->nid;
	if (nid == NUMA_NO_NODE)
		nid = numa_node_id();
	return nid;
}

In ttm_pool_alloc_page(), allocations with order > 0 set __GFP_THISNODE in
gfp_flags:

	if (order)
		gfp_flags |= __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN |
			__GFP_THISNODE;

Because alloc_pages_node() now receives numa_node_id() instead of
NUMA_NO_NODE, it no longer substitutes numa_mem_id(). When running on a CPU
attached to a memoryless NUMA node, ZONELIST_NOFALLBACK has no memory zones,
causing all order > 0 allocations to fail and fall back to order 0.

In addition, any order-0 WC/UC pages allocated via fallback will reside on a
memory-bearing node (page_to_nid(p) != numa_node_id()), so
ttm_pool_type_give() places them on page_to_nid(p)'s list_lru while
ttm_pool_type_take(pt, nid) searches the empty list_lru of the memoryless
numa_node_id().

Should ttm_pool_nid() use numa_mem_id() instead of numa_node_id() when nid is
NUMA_NO_NODE?

[ ... ]

> @@ -900,10 +901,52 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
>  
>  	ttm_pool_alloc_state_init(tt, &alloc);
>  
> -	return __ttm_pool_alloc(pool, tt, ctx, &alloc, NULL);
> +	return __ttm_pool_alloc(pool, tt, ctx, &alloc, NULL, ttm_pool_nid(pool));
>  }
>  EXPORT_SYMBOL(ttm_pool_alloc);
>  
> +/**
> + * ttm_pool_alloc_node - Fill a ttm_tt object, placing pages on a given node
> + *
> + * @pool: ttm_pool to use
> + * @tt: ttm_tt object to fill
> + * @ctx: operation context
> + * @nid: NUMA node id to allocate the backing system pages from, or
> + * NUMA_NO_NODE to use the pool's default node (@ttm_pool.nid)
> + *
> + * Like ttm_pool_alloc(), but allows the caller to override the NUMA node the
> + * backing system pages are allocated from. This only affects the
> + * non-coherent (non dma-alloc) allocation path; coherent/dma pools continue
> + * to use the device-local node. Intended for drivers that expose a
> + * user-selectable host memory NUMA placement.
> + *
> + * Returns: 0 on success, -EOPNOTSUPP if @nid is specified on a coherent/dma
> + * pool, negative error code otherwise.
> + */
> +int ttm_pool_alloc_node(struct ttm_pool *pool, struct ttm_tt *tt,
> +			struct ttm_operation_ctx *ctx, int nid)
> +{
> +	struct ttm_pool_alloc_state alloc;
> +
> +	if (WARN_ON(ttm_tt_is_backed_up(tt)))
> +		return -EINVAL;
> +
> +	/*
> +	 * Coherent/dma pools allocate via dma_alloc_attrs(), which places pages
> +	 * per device DMA constraints and cannot honor a caller-requested node.
> +	 */
> +	if (nid != NUMA_NO_NODE && ttm_pool_uses_dma_alloc(pool))
> +		return -EOPNOTSUPP;
> +
> +	if (nid == NUMA_NO_NODE)
> +		nid = ttm_pool_nid(pool);
> +
> +	ttm_pool_alloc_state_init(tt, &alloc);
> +
> +	return __ttm_pool_alloc(pool, tt, ctx, &alloc, NULL, nid);
> +}
> +EXPORT_SYMBOL(ttm_pool_alloc_node);

[Severity: Low]
This isn't a bug, but there are a couple of inconsistencies between the
documentation, the commit message, and the implementation here.

First, the kernel-doc description for ttm_pool_alloc_node() states that
"coherent/dma pools continue to use the device-local node", which contradicts
both the Returns section ("-EOPNOTSUPP if @nid is specified on a coherent/dma
pool") and the implementation when nid != NUMA_NO_NODE.

Second, the commit message mentions that "ttm_pool_alloc() passes
NUMA_NO_NODE, keeping current behaviour" and that "__ttm_pool_alloc(),
ttm_pool_alloc_page(), ttm_pool_type_take() already take a nid", whereas this
patch adds the nid parameter to __ttm_pool_alloc() and ttm_pool_alloc_page(),
and ttm_pool_alloc() duplicates the setup and calls __ttm_pool_alloc()
directly.

Could ttm_pool_alloc() simply call ttm_pool_alloc_node(pool, tt, ctx,
NUMA_NO_NODE), and the kernel-doc and commit message be updated to match?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102101803.455921-9-himal.prasad.ghimiray@intel.com?part=1