Re: [RFC PATCH v2 19/25] iommu/dma: Stop recycling old PISA pages when allocator waste is high

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] iommu/dma: unsafe list traversal in pisa_alloc_coherent() on
  list move
- [Low] iommu/dma: missing range check on wasted_pct in
  pisa_page_can_recycle()
- [Low] iommu/dma: unexplained EXPORT_SYMBOL change on
  pisa_page_can_recycle()
- [Low] iommu/dma: missing alloc_jiffies update in pisa_take_contig()
  error path
- [Low] iommu/dma: undocumented pisa.wasted_pct and pisa.old_ms boot
  parameters
--

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

iommu/dma: Stop recycling old PISA pages when allocator waste is high

This commit records an allocation timestamp in struct pisa_meta and adds
the pisa.wasted_pct and pisa.old_ms parameters so pisa_page_can_recycle()
stops recycling blocks from old partially used 2MB pages when allocator
waste exceeds the threshold.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index 1930135880d2b..5054a96f0f4e9 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c
> @@ -74,6 +74,14 @@ module_param_named(idle_ms, pisa_idle_ms, uint, 0644);
>  static unsigned int pisa_compact __read_mostly;
>  module_param_named(compact, pisa_compact, uint, 0644);
>  
> +/* Stop recycling partial pages older than @pisa_old_ms when wasted blocks > @pisa_wasted_pct%. */
> +#define PISA_OLD_MS_MAX		100000U
> +static unsigned int pisa_wasted_pct __read_mostly;
> +module_param_named(wasted_pct, pisa_wasted_pct, uint, 0644);
> +
> +static unsigned int pisa_old_ms __read_mostly;
> +module_param_named(old_ms, pisa_old_ms, uint, 0644);
> +

[Severity: Low]
Since CONFIG_PISA is a bool Kconfig symbol, pisa.wasted_pct and
pisa.old_ms are built-in kernel command-line parameters.  Should they be
documented in Documentation/admin-guide/kernel-parameters.txt (with their
format, valid ranges, behavior, and architecture constraints)?  The later
commit "Documentation: Add PISA architecture documentation" in this series
lists their names under pisa.*=, but neither kernel-parameters.txt nor
Documentation/core-api/pisa.rst describes what they control or their
expected ranges.

[ ... ]

> @@ -603,7 +612,7 @@ static void pisa_return_block_locked(struct pisa_alloc *pa, struct page *block,
>   * pisa_page_can_recycle - Check if a block can stay cached in a caller pool
>   * @page: Block head page
>   *
> - * Called on the recycle fast path (per-CPU cache, TX page_frag, page_pool).
> + * Called on the recycle fast path (per-CPU cache, TX page_frag).
>   * Fast-path cost is a static-key branch plus a READ_ONCE() of module parameters
>   * (~1-2 ns when compaction is off); when enabled, it does a lockless RCU +
>   * metadata array lookup (~5-10 ns).
> @@ -613,10 +622,11 @@ static void pisa_return_block_locked(struct pisa_alloc *pa, struct page *block,
>   */
>  bool pisa_page_can_recycle(const struct page *page)
>  {
> -	unsigned long bpp, in_use, free_blocks, pfn;
> +	unsigned int compact, wasted_pct, old_ms;
> +	unsigned long bpp, in_use, free_blocks;
>  	const struct pisa_meta *meta;
>  	const struct pisa_alloc *pa;
> -	unsigned int compact;
> +	unsigned long pfn;
>  	bool ret = true;
>  	u8 on_list;
>  	long held;
> @@ -625,7 +635,9 @@ bool pisa_page_can_recycle(const struct page *page)
>  		return true;
>  
>  	compact = READ_ONCE(pisa_compact);
> -	if (!compact)
> +	wasted_pct = READ_ONCE(pisa_wasted_pct);
> +	old_ms = READ_ONCE(pisa_old_ms);
> +	if (!compact && (!wasted_pct || !old_ms))
>  		return true;
>  
>  	pfn = page_to_pfn(page);
> @@ -638,6 +650,13 @@ bool pisa_page_can_recycle(const struct page *page)
>  	if (on_list == PISA_LIST_FULL || on_list == PISA_LIST_NONE)
>  		goto out;
>  
> +	if (!compact) {
> +		unsigned long max_age = msecs_to_jiffies(min(old_ms, PISA_OLD_MS_MAX));
> +
> +		if (!time_after(jiffies, READ_ONCE(meta->alloc_jiffies) + max_age))
> +			goto out;
> +	}
> +
>  	pa = meta->pa;
>  	if (!pa)
>  		goto out;
> @@ -657,12 +676,21 @@ bool pisa_page_can_recycle(const struct page *page)
>  	free_blocks = held > 0 && (unsigned long)held * bpp > in_use ?
>  		      (unsigned long)held * bpp - in_use : 0;
>  
> -	ret = free_blocks <= (unsigned long)compact * bpp;
> +	if (compact && free_blocks > (unsigned long)compact * bpp) {
> +		ret = false;
> +		goto out;
> +	}
> +	if (wasted_pct && old_ms && held > 0 &&
> +	    free_blocks * 100 > (unsigned long)held * bpp * wasted_pct) {
> +		unsigned long max_age = msecs_to_jiffies(min(old_ms, PISA_OLD_MS_MAX));
> +
> +		ret = !time_after(jiffies, READ_ONCE(meta->alloc_jiffies) + max_age);
> +	}

[Severity: Low]
What happens if pisa_wasted_pct is set to 100 or higher?

Unlike pisa_old_ms, which is clamped to PISA_OLD_MS_MAX, pisa_wasted_pct
is registered as an unconstrained uint and used unclamped.  Setting
wasted_pct >= 100 bypasses the early return in pisa_page_can_recycle() and
runs the RCU lookup and utilization calculation on every recycle check,
even though free_blocks * 100 > held * bpp * wasted_pct can never hold (or
can overflow unsigned long for large uint values when held * bpp is large,
spuriously forcing compaction).  Also, when compact == 0, the earlier
!compact block has already verified time_after(jiffies, meta->alloc_jiffies
+ max_age), so recomputing max_age and time_after() here is redundant on
that path.

>  out:
>  	rcu_read_unlock();
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(pisa_page_can_recycle);
> +EXPORT_SYMBOL(pisa_page_can_recycle);

[Severity: Low]
Was changing EXPORT_SYMBOL_GPL() to EXPORT_SYMBOL() on
pisa_page_can_recycle() intentional?  The commit message does not mention
dropping the GPL restriction, and it still lists page_pool as a caller
governed by pisa_page_can_recycle() even though the kernel-doc update
above removes page_pool.

[ ... ]

> @@ -1454,8 +1485,10 @@ static struct page *pisa_take_contig(struct pisa_meta *meta, unsigned int nblock
>  	}
>  
>  	if (meta->on_list == PISA_LIST_IDLE_CLEAN ||
> -	    meta->on_list == PISA_LIST_IDLE_DIRTY)
> +	    meta->on_list == PISA_LIST_IDLE_DIRTY) {
>  		pa->num_idle_pages--;
> +		WRITE_ONCE(meta->alloc_jiffies, jiffies);
> +	}

[Severity: Low]
Should the poisoned fallback path earlier in pisa_take_contig() also
update meta->alloc_jiffies when moving an idle page onto a partial list?

drivers/iommu/pisa-alloc.c:pisa_take_contig() {
    ...
		if (off >= bpp) {
			if (poisoned) {
				if (meta->on_list == PISA_LIST_IDLE_CLEAN ||
				    meta->on_list == PISA_LIST_IDLE_DIRTY)
					pa->num_idle_pages--;
				pisa_place_meta(meta);
			}
			return NULL;
		}
    ...
}

If pisa_take_contig() isolates a poisoned block on an idle page and then
fails to find nblocks contiguous blocks, pisa_place_meta() moves meta from
pa->idle onto pa->partial or pa->partial_dirty without setting
meta->alloc_jiffies.  Later allocations from that partial page will not
update meta->alloc_jiffies, leaving a stale timestamp from the page's
previous use and causing pisa_page_can_recycle() to treat blocks allocated
from it as already expired.

[ ... ]

> @@ -1544,6 +1577,7 @@ void *pisa_alloc_coherent(struct page **pagep, struct device *dev,
>  		spin_lock_irqsave(&pa->lock, flags);
>  		meta->pa = pa;
>  		meta->prot = pa->prot;
> +		WRITE_ONCE(meta->alloc_jiffies, jiffies);
>  		WRITE_ONCE(meta->flags, meta->flags | PISA_ACTIVE);
>  		pa->stats.page_alloc_cnt++;
>  		page = pisa_take_contig(meta, nblocks, align_mask);

[Severity: Medium]
Can the list_for_each_entry() loop just above this in
pisa_alloc_coherent() walk off into the wrong list if pisa_take_contig()
encounters a poisoned block and returns NULL?

drivers/iommu/pisa-alloc.c:pisa_alloc_coherent() {
    ...
	for (i = 0; i < ARRAY_SIZE(lists) && !page; i++) {
		list_for_each_entry(meta, lists[i], list) {
			page = pisa_take_contig(meta, nblocks, align_mask);
			if (page)
				break;
		}
	}
    ...
}

When pisa_take_contig() hits a poisoned block and fails to find nblocks
contiguous blocks (off >= bpp), it calls pisa_place_meta(meta) and returns
NULL.  pisa_place_meta() relinks &meta->list onto a different list head in
struct pisa_alloc (for example from pa->idle to pa->partial, or from
pa->partial to pa->partial_dirty).  Because list_for_each_entry() only
breaks when page is non-NULL, it then follows meta->list.next along the
new list while checking for termination against lists[i], walking past the
new list's head in struct pisa_alloc and dereferencing struct pisa_alloc
as a struct pisa_meta.  This was introduced earlier in this series in
commit "iommu/dma: Add PISA block allocation"; using
list_for_each_entry_safe() fixes the traversal.

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