Re: [RFC PATCH v2 11/25] iommu/dma: Add per-CPU block caches to PISA allocators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] iommu/dma: cross-node per-CPU cache refill in
  pisa_alloc_bulk()
- [Medium] iommu/dma: hardcoded order 3 in test_gfp_pisa() fails on
  16K/64K pages
- [Low] iommu/dma: changelog omits pisa_page_can_recycle() and
  pisa.compact
- [Low] iommu/dma: short allocation in pisa_pcp_alloc() on poisoned
  block
--

commit 6d45ca2b93f2e0d3d72f8ae4375bcf8901944ad1
Author: Luigi Rizzo <lrizzo@google.com>

> iommu/dma: Add per-CPU block caches to PISA allocators
>
> Every block allocation and free takes the allocator's lock. Highly
> concurrent users e.g. socket tx fragments, can cause significant lock
> contention.
>
> Add pisa_enable_pcp(), an opt-in per-CPU front cache, similar
> to the page allocator's per-CPU lists:
>
> - Each per-CPU cache keeps up to 64 free blocks, and refills it from
>   the main allocator in batches of 32.
> - returned blocks belonging to the local NUMA node are kept in the local
>   cache.
> - returned blocks belonging to a remote NUMA node are returned to the
>   proper per-node allocator.
> The cache is fully integrated with the shrinker and CPU offlining to
> drain the cache.
>
> Enable per-CPU caches for the per-node order-0 RW and SKB_FRAG_PAGE_ORDER
> RO allocators.
>
> Signed-off-by: Luigi Rizzo <lrizzo@google.com>

[Severity: Low]
This isn't a bug, but this patch also adds and exports
pisa_page_can_recycle(), the pisa.compact module parameter, and the
per-allocator block and page accounting used for soft compaction, none of
which are mentioned in the commit message.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index 25e4645310678..913706a503341 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c

[ ... ]

> +/* Allocate @nr_pages blocks through this CPU's cache, refilling in batches on miss. */
> +static unsigned long pisa_pcp_alloc(struct pisa_alloc *pa, gfp_t gfp, unsigned long nr_pages,
> +				    struct page **page_array, bool frozen)
> +{
> +	unsigned int pcp_batch = READ_ONCE(pisa_pcp_batch);
> +	unsigned long got, n, i, j;
> +
> +	got = pisa_pcp_pop(pa, nr_pages, page_array);
> +	if (got < nr_pages) {
> +		if (nr_pages - got >= pcp_batch) {
> +			got += __pisa_alloc_bulk(pa, gfp, nr_pages - got,
> +						 page_array + got, false, true);
> +		} else {
> +			struct page *batch[PISA_PCP_BATCH];
> +
> +			n = __pisa_alloc_bulk(pa, gfp, pcp_batch, batch, false, true);
> +			for (i = 0; i < n && got < nr_pages; i++)
> +				page_array[got++] = batch[i];
> +			if (i < n)
> +				pisa_pcp_push(pa, batch + i, n - i);
> +		}
> +	}
> +
> +	for (i = j = 0; i < got; i++) {
> +		struct page *block = page_array[i];
> +
> +		page_array[i] = NULL;
> +		/* Poisoned while cached: leak it, as pisa_scan_bitmap() does. */
> +		if (pisa_block_poisoned(pa, block))
> +			continue;
> +		pisa_block_prep(block);
> +		if (want_init_on_alloc(gfp)) {
> +			memset(page_address(block), 0, PAGE_SIZE << pa->order);
> +			if (frozen) /* Order zeroing before caller publishes the page. */
> +				smp_wmb();
> +		}
> +		if (!frozen)
> +			page_ref_unfreeze(block, 1);
> +		page_array[j++] = block;
> +	}
> +	return j;
> +}
> +
>  /**
>   * pisa_alloc_bulk - Allocate up to @nr_pages blocks from @pa
>   * @pa: Allocator to allocate from
> @@ -681,11 +952,16 @@ unsigned long pisa_alloc_bulk(struct pisa_alloc *pa, gfp_t gfp,
>  			i++;
>  			continue;
>  		}
> +
>  		/* Fill the run of @n NULL entries at @i. */
>  		for (n = 1; i + n < nr_pages && !page_array[i + n]; n++)
>  			;
> -		got = __pisa_alloc_bulk(pa, gfp, n, page_array + i,
> -					want_init_on_alloc(gfp), frozen);
> +		/* Pairs with smp_store_release() in pisa_enable_pcp(). */
> +		if (smp_load_acquire(&pa->pcp))
> +			got = pisa_pcp_alloc(pa, gfp, n, page_array + i, frozen);
> +		else
> +			got = __pisa_alloc_bulk(pa, gfp, n, page_array + i,
> +						want_init_on_alloc(gfp), frozen);
>  		i += got;
>  		if (got < n)
>  			break;

[Severity: Low]
If a block popped from pa->pcp was poisoned while cached, does
pisa_pcp_alloc() return fewer than n blocks without refilling a
replacement?  Unlike pisa_scan_bitmap(), skipping the poisoned block after
the refill stage causes pisa_alloc_bulk() to see got < n and abort the
allocation early even when healthy blocks are available.

[ ... ]

> @@ -876,6 +1158,17 @@ bool __pisa_free_page(struct page *page)
>  	if (want_init_on_free())
>  		memset(page_address(page), 0, PAGE_SIZE << pa->order);
>  
> +	/* Pairs with cmpxchg_release() in pisa_enable_pcp(). */
> +	if (smp_load_acquire(&pa->pcp) && pa->nid == numa_mem_id() &&
> +	    page_to_nid(page) == numa_mem_id()) {
> +		if (pisa_page_can_recycle(page)) {
> +			pisa_pcp_push(pa, &page, 1);
> +			return true;
> +		}
> +		if (this_cpu_read(pa->pcp->count))
> +			pisa_pcp_drain_cpu(pa, raw_smp_processor_id());
> +	}

[Severity: Medium]
Can pisa_alloc_bulk() -> pisa_pcp_alloc() populate pa->pcp on a CPU where
numa_mem_id() != pa->nid?

__pisa_free_page() only pushes freed blocks to or compact-drains pa->pcp
when pa->nid == numa_mem_id(), but pisa_alloc_bulk() calls
pisa_pcp_alloc() without checking whether the current CPU belongs to
pa->nid (and __pisa_alloc_bulk() can also sleep and migrate the task
before pisa_pcp_push() runs).

On a multi-node NUMA system, a cross-node allocation refills 32 blocks and
leaves up to 31 blocks cached in that remote CPU's pa->pcp, where normal
frees never compact-drain them and CPUs on pa->nid never pop them, pinning
their backing 2MB PISA pages in pa->blocks_in_use.  This also causes
test_pcp_cache() and test_alloc_and_recycle() in drivers/iommu/pisa-kunit.c
to fail KUNIT_EXPECT_PTR_EQ(test, p2, p1) whenever the KUnit kthread runs
on a CPU with numa_mem_id() != first_memory_node: p1 = pisa_alloc(pa, ...)
refills 32 blocks into the remote CPU's pa->pcp and returns batch[0],
__free_page(p1) bypasses pa->pcp and returns p1 to the bitmap, and
p2 = pisa_alloc(pa, ...) pops batch[31] from pa->pcp instead of p1.

> diff --git a/drivers/iommu/pisa-kunit.c b/drivers/iommu/pisa-kunit.c
> index 72ee8986b4802..5bbb540eb505e 100644
> --- a/drivers/iommu/pisa-kunit.c
> +++ b/drivers/iommu/pisa-kunit.c

[ ... ]

> +static void test_gfp_pisa(struct kunit *test)
> +{

[ ... ]

> +	p_ro = alloc_pages(GFP_KERNEL | __GFP_COMP | __GFP_PISA_RO, 3);
> +	KUNIT_ASSERT_NOT_NULL(test, p_ro);
> +	if (p_ro) {
> +		KUNIT_EXPECT_TRUE(test, is_pisa_page(p_ro));
> +		KUNIT_EXPECT_EQ(test, pisa_meta_of_pfn(page_to_pfn(p_ro))->prot,
> +				IOMMU_READ);
> +		__free_pages(p_ro, 3);
> +	}

[Severity: Medium]
Should test_gfp_pisa() pass SKB_FRAG_PAGE_ORDER (or 2) instead of
hardcoding order 3?  pisa_init() only creates read-only per-node
allocators for orders 2 and SKB_FRAG_PAGE_ORDER, and on arm64 with 16KB or
64KB base pages SKB_FRAG_PAGE_ORDER is 1 or 0, so order 3 has no read-only
PISA allocator and falls back to the buddy allocator, failing
is_pisa_page(p_ro).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182533.3594984-1-lrizzo@google.com?part=11