Re: [RFC PATCH v2 15/25] iommu/dma: Add background page scrubber for PISA allocators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] iommu/dma: redundant memset() of scrubbed blocks in
  pisa_alloc_coherent()
- [Low] iommu/dma: race with async refill when trimming reservoir in
  KUnit test
--

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

iommu/dma: Add background page scrubber for PISA allocators

This commit adds an optional demand-driven background page scrubber for
PISA allocators that zeroes dirty blocks on each NUMA node's work item and
moves them to clean_bitmap.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index b4a25c65bf97e..4b61ec84ed6d1 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c

[ ... ]

> @@ -1166,11 +1339,13 @@ static struct page *pisa_take_contig(struct pisa_meta *meta, unsigned int nblock
>  		} else {
>  			__clear_bit(off + i, meta->dirty_bitmap);
>  			meta->nr_dirty--;
> +			dirty++;
>  		}
>  		pisa_block_prep(page + i);
>  		page_ref_unfreeze(page + i, 1);
>  	}
>  	pa->blocks_in_use += nblocks;
> +	pa->scrub_demand = min(pa->scrub_demand + dirty, PISA_SCRUB_DEMAND_MAX);
>  	pisa_place_meta(meta);
>  	return page;
>  }
> @@ -1185,7 +1360,7 @@ void *pisa_alloc_coherent(struct page **pagep, struct device *dev,
>  			  struct iommu_domain *domain, size_t size,
>  			  gfp_t gfp, unsigned long attrs)
>  {
> -	bool should_release = false;
> +	bool should_scrub = false, should_release = false;
>  	struct list_head *lists[3];
>  	unsigned long align_mask, flags;
>  	unsigned int nblocks, i;
> @@ -1234,10 +1409,14 @@ void *pisa_alloc_coherent(struct page **pagep, struct device *dev,
>  		}
>  	}
>  	if (!page) {
> +		should_scrub |= pa->scrub_demand >= PISA_SCRUB_TRIGGER;
>  		spin_unlock_irqrestore(&pa->lock, flags);
>  		meta = pisa_alloc_split_page(0, gfp, pa->nid);
> -		if (IS_ERR(meta))
> +		if (IS_ERR(meta)) {
> +			if (should_scrub)
> +				pisa_schedule_scrub(pa->nid);
>  			return NULL;
> +		}
>  		spin_lock_irqsave(&pa->lock, flags);
>  		meta->pa = pa;
>  		meta->prot = pa->prot;
> @@ -1247,10 +1426,13 @@ void *pisa_alloc_coherent(struct page **pagep, struct device *dev,
>  		if (unlikely(!page))
>  			should_release = pisa_place_meta(meta);
>  	}
> +	should_scrub |= pa->scrub_demand >= PISA_SCRUB_TRIGGER;
>  	spin_unlock_irqrestore(&pa->lock, flags);
>  
>  	if (should_release)
>  		pisa_schedule_reclaim();
> +	if (should_scrub)
> +		pisa_schedule_scrub(pa->nid);
>  	if (!page)
>  		return NULL;
>  	*pagep = page;

[Severity: Medium]
Does pisa_alloc_coherent() end up zeroing blocks twice when background
scrubbing is enabled?

pisa_take_contig() charges dirty blocks to pa->scrub_demand and
pisa_alloc_coherent() schedules the background scrubber on the per-node
order-0 RW allocator, which zeroes dirty blocks and moves them to
meta->clean_bitmap.  However, unlike __pisa_alloc_bulk(),
pisa_alloc_coherent() unconditionally calls memset(page_address(page), 0,
size) at the end of the function even when blocks were taken from
meta->clean_bitmap (and pisa_pcp_alloc() on the same allocator also refills
with zero = false and zeroes inline), so background-scrubbed blocks are
always zeroed again on allocation.

> diff --git a/drivers/iommu/pisa-kunit.c b/drivers/iommu/pisa-kunit.c
> index b63cd6585b673..40bf7801183da 100644
> --- a/drivers/iommu/pisa-kunit.c
> +++ b/drivers/iommu/pisa-kunit.c
> @@ -572,8 +572,53 @@ static void test_gfp_pisa(struct kunit *test)
>  
>  static void test_reservoir_and_scrubber(struct kunit *test)
>  {
> -	KUNIT_ASSERT_EQ(test, pisa_meta_init(), 0);
> -	KUNIT_EXPECT_EQ(test, READ_ONCE(pisa_reservoir_max), 16U);
> +	unsigned int old_res = READ_ONCE(pisa_reservoir_max);
> +	unsigned int old_scrub = READ_ONCE(pisa_scrub_max);
> +	struct shrink_control sc = { .nr_to_scan = ULONG_MAX };
> +	struct pisa_meta *meta;
> +	struct pisa_alloc *pa;
> +	struct page *p;
> +	int nid;
> +
> +	pisa_shrink_scan(NULL, &sc);
> +
> +	/* Refill reservoir to 1 PISA page per memory node, then consume one. */
> +	WRITE_ONCE(pisa_reservoir_max, 1);
> +	pisa_schedule_reservoir();
> +	for_each_node_state(nid, N_MEMORY)
> +		flush_work(&pisa_nodes[nid].work);
> +	KUNIT_EXPECT_GE(test, pisa_reservoir_count(), 1UL);
> +
> +	pa = pisa_node_pa(first_memory_node, 2, true);
> +	KUNIT_ASSERT_NOT_NULL(test, pa);
> +	p = pisa_alloc(pa, GFP_KERNEL);
> +	if (p) {
> +		meta = pisa_meta_of_pfn(page_to_pfn(p));
> +		/* Reservoir pages come pre-zeroed (all remaining blocks clean). */
> +		KUNIT_EXPECT_EQ(test, meta->nr_dirty, 0U);
> +		__free_pages(p, 2);
> +		if (!want_init_on_free()) {
> +			KUNIT_EXPECT_EQ(test, meta->nr_dirty, 1U);
> +
> +			/* Scrub dirty block back into clean_bitmap. */
> +			pa->scrub_demand = 1;
> +			WRITE_ONCE(pisa_scrub_max, 512);
> +			pisa_scrub_node(pa->nid);
> +			KUNIT_EXPECT_EQ(test, meta->nr_dirty, 0U);
> +		}
> +	}
> +	pisa_shrink_scan(NULL, &sc);
> +
> +	/* Trim reservoirs back to 0. */
> +	WRITE_ONCE(pisa_reservoir_max, 0);
> +	pisa_schedule_reservoir();
> +	for_each_node_state(nid, N_MEMORY)
> +		flush_work(&pisa_nodes[nid].work);
> +	KUNIT_EXPECT_EQ(test, pisa_reservoir_count(), 0UL);

[Severity: Low]
Can an in-flight refill race with the trim step and cause
KUNIT_EXPECT_EQ(test, pisa_reservoir_count(), 0UL) to fail?

Popping a page from the reservoir in pisa_alloc() drops res->nr_pages from
1 to 0 while pisa_reservoir_max is still 1, which queues res->work to
refill the node:

CPU0 (kunit)                      CPU1 (kworker)
pisa_alloc(pa, GFP_KERNEL)
  pisa_reservoir_pop_node()
    res->nr_pages-- (0)
    pisa_schedule_reservoir_node()
      queue_work_node(..., &res->work)
                                  pisa_node_work_fn()
                                    pisa_reservoir_trim_node()
                                    pisa_reservoir_refill_node()
                                      /* reads pisa_reservoir_max == 1 */
                                      __pisa_page_alloc()
WRITE_ONCE(pisa_reservoir_max, 0)
pisa_schedule_reservoir()
  pisa_schedule_reservoir_node()
    /* nr_pages (0) == max (0),
     * does not re-queue res->work
     */
                                      spin_lock_irqsave(&res->lock, ...)
                                      list_add(&meta->list, &res->pages)
                                      res->nr_pages++ (1)
                                      spin_unlock_irqrestore(&res->lock, ...)
flush_work(&pisa_nodes[nid].work)
KUNIT_EXPECT_EQ(...,
  pisa_reservoir_count(), 0UL)  <-- fails (count is 1)

Flushing pisa_nodes[nid].work before setting pisa_reservoir_max to 0 (or
re-checking pisa_reservoir_max under res->lock in
pisa_reservoir_refill_node()) would avoid this race.

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