Re: [RFC PATCH v2 25/25] iommu/dma: Add PISA statistics and debugfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] iommu/dma: missing failure and NUMA stats in
  pisa_alloc_coherent()
- [Low] iommu/dma: miscounted user page budget and skip stats in
  pisa-user.c
--

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

iommu/dma: Add PISA statistics and debugfs

This commit adds per-allocator, per-node reservoir, user-page migration,
and global shrinker/reclaim statistics for PISA, exposes them in
/sys/kernel/debug/pisa/allocs, and logs a one-shot warning on the first
2MB buddy allocation failure.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index cedb1a62d20e2..91b9f00d21985 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c

[ ... ]

> @@ -1310,15 +1369,31 @@ static unsigned long __pisa_alloc_bulk(struct pisa_alloc *pa, gfp_t gfp, unsigne
>  		spin_unlock_irqrestore(&pa->lock, flags);
>  		/* With @zero, fresh dirty blocks are zeroed below. */
>  		meta = pisa_alloc_split_page(pa->order, gfp, pa->nid);
> -		if (IS_ERR(meta))
> +		if (IS_ERR(meta)) {
> +			long err = PTR_ERR(meta);
> +
> +			if (err == -EAGAIN) {
> +				pisa_stat_atomic_inc(pa->stats.fail_backoff);
> +			} else if (err == -ENOSPC) {
> +				pisa_stat_atomic_inc(pa->stats.fail_budget);
> +			} else if (err == -ENOMEM) {
> +				pisa_stat_atomic_inc(pa->stats.fail_nomem);
> +				pr_warn_once("pisa: order-%u allocation failed for allocator %p; further failures are counted in debugfs only\n",
> +					     PISA_ORDER, pa);
> +			} else if (err == -EBUSY) {
> +				pisa_stat_atomic_inc(pa->stats.fail_split);
> +			}
> +			if (!n)
> +				pisa_stat_atomic_inc(pa->stats.block_alloc_fail);
>  			goto out;
> +		}
>  		spin_lock_irqsave(&pa->lock, flags);
>  		meta->pa = pa;
>  		meta->prot = pa->prot;
>  		WRITE_ONCE(meta->alloc_jiffies, jiffies);
>  		/* Set PISA_ACTIVE after @meta is fully set up. */
>  		WRITE_ONCE(meta->flags, meta->flags | PISA_ACTIVE);
> -		pa->stats.page_alloc_cnt++;
> +		pisa_stat_inc(pa->stats.page_alloc_cnt);
>  	}

[ ... ]

> @@ -1500,6 +1593,9 @@ static struct page *pisa_take_contig(struct pisa_meta *meta, unsigned int nblock
>  		page_ref_unfreeze(page + i, 1);
>  	}
>  	pa->blocks_in_use += nblocks;
> +	pisa_stat_add(pa->stats.block_alloc_cnt, nblocks);
> +	pisa_stat_add(pa->stats.alloc_kc, nblocks - dirty);
> +	pisa_stat_add(pa->stats.alloc_ks, dirty);
>  	pa->scrub_demand = min(pa->scrub_demand + dirty, PISA_SCRUB_DEMAND_MAX);
>  	pisa_place_meta(meta);
>  	return page;
> @@ -1577,7 +1673,7 @@ void *pisa_alloc_coherent(struct page **pagep, struct device *dev,
>  		meta->prot = pa->prot;
>  		WRITE_ONCE(meta->alloc_jiffies, jiffies);
>  		WRITE_ONCE(meta->flags, meta->flags | PISA_ACTIVE);
> -		pa->stats.page_alloc_cnt++;
> +		pisa_stat_inc(pa->stats.page_alloc_cnt);
>  		page = pisa_take_contig(meta, nblocks, align_mask);
>  		if (unlikely(!page))
>  			should_release = pisa_place_meta(meta);

[Severity: Low]
Should pisa_alloc_coherent() and pisa_take_contig() also update the
allocator failure and NUMA mismatch counters?

Because the failure accounting and pr_warn_once() live in
__pisa_alloc_bulk() rather than pisa_alloc_split_page(), a
pisa_alloc_split_page() failure in pisa_alloc_coherent() returns NULL
without updating pa->stats.fail_*, pa->stats.block_alloc_fail, or
emitting the one-shot warning on -ENOMEM, and leaves an allocator with
no prior allocations hidden from /sys/kernel/debug/pisa/allocs:

drivers/iommu/pisa-alloc.c:pisa_alloc_coherent() {
	...
		meta = pisa_alloc_split_page(0, gfp, pa->nid);
		if (IS_ERR(meta)) {
			if (should_scrub)
				pisa_schedule_scrub(pa->nid);
			return NULL;
		}
}

In addition, unlike take_blocks(), pisa_take_contig() does not add
nblocks to pa->stats.numa_mismatch_cnt when page_to_nid(page) != pa->nid.

> diff --git a/drivers/iommu/pisa-user.c b/drivers/iommu/pisa-user.c
> index 7dca3bf066f0d..5cf199943f82c 100644
> --- a/drivers/iommu/pisa-user.c
> +++ b/drivers/iommu/pisa-user.c

[ ... ]

> @@ -264,9 +288,17 @@ static unsigned int pisa_user_migrate_dir(struct list_head *list, bool to_pisa,
>  		if (!gfpflags_allow_blocking(gfp))
>  			mode = MIGRATE_ASYNC;
>  	}
> +	nr = list_count_nodes(list);	/* all order-0 */
>  	if (migrate_pages(list, to_pisa ? pisa_user_new_folio : pisa_user_old_folio,
>  			  NULL, (unsigned long)&mtc, mode, MR_SYSCALL, &done))
>  		putback_movable_pages(list);
> +	if (to_pisa) {
> +		pisa_stat_atomic_add(pisa_user_stats.in, done);
> +		pisa_stat_atomic_add(pisa_user_stats.in_fail, nr - done);
> +	} else {
> +		pisa_stat_atomic_add(pisa_user_stats.out, done);
> +		pisa_stat_atomic_add(pisa_user_stats.out_fail, nr - done);
> +	}
>  	return done;
>  }

[ ... ]

> @@ -583,9 +623,14 @@ int pisa_madvise(struct vm_area_struct *vma, unsigned long start, unsigned long
>  		struct folio *folio = folio_walk_start(&fw, vma, addr, 0);
>  
>  		if (folio) {
> -			if (pisa_user_want_dir(folio, to_pisa, DMA_TO_DEVICE, false) &&
> -			    !pisa_user_busy(folio, 0) && isolate_folio_to_list(folio, &list))
> -				n++;
> +			if (to_pisa && folio_test_large(folio) && folio_test_anon(folio)) {
> +				pisa_stat_atomic_inc(pisa_user_stats.large);
> +			} else if (pisa_user_want_dir(folio, to_pisa, DMA_TO_DEVICE, false)) {
> +				if (pisa_user_busy(folio, 0))
> +					pisa_stat_atomic_inc(pisa_user_stats.busy);
> +				else if (isolate_folio_to_list(folio, &list))
> +					n++;
> +			}
>  			folio_walk_end(&fw, vma);
>  		}
>  		if (n == PISA_USER_BATCH) {

[Severity: Low]
Does pisa_madvise() count budget exhaustion as pisa_user_stats.in_fail
instead of pisa_user_stats.no_budget?

Unlike pisa_upgrade_user_pages(), pisa_madvise() isolates folios without
checking pisa_user_budget().  When the user page budget is exhausted (or
runs out mid-batch in pisa_user_new_folio()), migrate_pages() aborts and
pisa_user_migrate_dir() charges the unmigrated folios (nr - done) to
pisa_user_stats.in_fail rather than pisa_user_stats.no_budget.

Similarly, pisa_folio_upgrade() returns early when pisa_user_budget() <= 0
or pisa_user_busy(folio, 1) is true without incrementing
pisa_user_stats.no_budget or pisa_user_stats.busy.

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