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 <[email protected]> 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/[email protected]?part=1
