Re: [RFC PATCH v2 03/25] iommu/dma: Add per-NUMA-node PISA page reservoir

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] iommu/dma: pisa_reservoir_pop() schedules refill worker
  before backoff check
- [Medium] iommu/dma: fs_reclaim lock inversion on pisa_meta_mutex in
  __pisa_page_alloc()
--

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

iommu/dma: Add per-NUMA-node PISA page reservoir

This commit adds a per-NUMA-node reservoir of pre-allocated PISA pages to
serve non-blocking allocations, refilled and trimmed asynchronously by
per-node work items.

> diff --git a/drivers/iommu/pisa-node.c b/drivers/iommu/pisa-node.c
> new file mode 100644
> index 0000000000000..524f0d4457ad7
> --- /dev/null
> +++ b/drivers/iommu/pisa-node.c
[ ... ]
> +/* Allocate a PISA page from the buddy allocator and initialize its metadata. */
> +static struct pisa_meta *__pisa_page_alloc(gfp_t gfp, int nid, bool can_block,
> +					   bool *clean)
> +{
> +	struct pisa_meta *meta;
> +	unsigned int memflags;
> +	struct page *page;
> +
> +	if (unlikely(!static_branch_unlikely(&pisa_key))) {
> +		if (!can_block || (gfp & (__GFP_FS | __GFP_IO)) != (__GFP_FS | __GFP_IO) ||
> +		    pisa_meta_init())
> +			return ERR_PTR(-EAGAIN);
> +	}
> +
> +	/*
> +	 * Long-lived active pages must be unmovable, lowmem, non-compound, and
> +	 * must not dip into emergency reserves or trigger OOM/warnings.
> +	 */
> +	gfp &= ~(__GFP_COMP | __GFP_HIGHMEM | __GFP_MOVABLE);
> +	gfp |= __GFP_NOWARN | __GFP_NOMEMALLOC | (can_block ? __GFP_RETRY_MAYFAIL : __GFP_NORETRY);
> +
> +	if (can_block)
> +		memflags = pisa_memalloc_save(gfp);
> +	page = alloc_pages_node(nid, gfp, PISA_ORDER);
> +	if (!page) {
> +		if (can_block)
> +			memalloc_flags_restore(memflags);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Ensure the metadata chunk for this PFN is populated. */
> +	if (unlikely(pisa_meta_ensure_pfn(page_to_pfn(page), can_block))) {

[Severity: Medium]
Can this invert the lock order between pisa_meta_mutex and fs_reclaim when
called under GFP_NOFS, GFP_NOIO, or a memalloc_nofs_save() /
memalloc_noio_save() scope?

pisa_meta_init(), pisa_meta_memory_cb(), and GFP_KERNEL calls to
pisa_meta_ensure_pfn() perform GFP_KERNEL allocations while holding
pisa_meta_mutex, establishing pisa_meta_mutex -> fs_reclaim.

Here, can_block is gfpflags_allow_blocking(gfp), which is true for GFP_NOFS
and GFP_NOIO.  Although pisa_memalloc_save(gfp) sets PF_MEMALLOC_NOFS or
PF_MEMALLOC_NOIO, __pisa_page_alloc() still passes can_block = true to
pisa_meta_ensure_pfn(), which acquires pisa_meta_mutex when the metadata
page table is not yet populated:

drivers/iommu/pisa-meta.c:pisa_meta_ensure_pfn() {
    ...
	if (can_block && !pisa_meta_pte((unsigned long)pisa_meta_array + (idx << PAGE_SHIFT))) {
		int err;

		mutex_lock(&pisa_meta_mutex);
		err = pisa_meta_prealloc(pisa_meta_array, pisa_meta_nframes, pfn, pfn + 1);
		mutex_unlock(&pisa_meta_mutex);
    ...
}

In addition, the pisa_meta_init() check above tests raw gfp rather than
current_gfp_context(gfp), so a caller inside a memalloc_nofs_save() or
memalloc_noio_save() scope passing GFP_KERNEL will also take pisa_meta_mutex:

CPU0                              CPU1
----                              ----
lock(fs_reclaim)
                                  mutex_lock(&pisa_meta_mutex)
mutex_lock(&pisa_meta_mutex)
                                  lock(fs_reclaim)

 *** DEADLOCK ***

CPU0: fs/io path (holding lock waited on by reclaim) -> pisa_reservoir_pop()
      -> __pisa_page_alloc() -> pisa_meta_ensure_pfn() / pisa_meta_init()
CPU1: pisa_node_work_fn() -> pisa_meta_init() -> kvcalloc(..., GFP_KERNEL)

Passing current_gfp_context(gfp) into gfp at entry and only allowing the
mutex-taking paths when (gfp & (__GFP_FS | __GFP_IO)) == (__GFP_FS | __GFP_IO)
would avoid this lock inversion.

[ ... ]
> +/* Refill node @nid's reservoir up to pisa_reservoir_max pre-zeroed PISA pages. */
> +static void pisa_reservoir_refill_node(int nid)
> +{
> +	struct pisa_node *res = &pisa_nodes[nid];
> +
> +	while (res->nr_pages < READ_ONCE(pisa_reservoir_max)) {
> +		struct pisa_meta *meta;
> +		unsigned long flags;
> +		bool clean;
> +
> +		meta = __pisa_page_alloc(GFP_KERNEL | __GFP_THISNODE, nid, true, &clean);
> +		if (IS_ERR(meta))
> +			break;
[ ... ]
> +struct pisa_meta *pisa_reservoir_pop(int nid, gfp_t gfp, bool *clean)
> +{
> +	bool can_block = gfpflags_allow_blocking(gfp);
> +	unsigned int max = READ_ONCE(pisa_reservoir_max);
> +	struct pisa_meta *meta;
> +
> +	nid = pisa_sanitize_nid(nid);
> +	if (max) {
> +		meta = pisa_reservoir_pop_node(nid, max);
> +		if (meta) {
> +			*clean = true;
> +			return meta;
> +		}
> +		/* Empty: pop_node() only schedules after a successful pop. */
> +		pisa_schedule_reservoir_node(nid);
> +	}
> +
> +	/* Back off briefly on GFP_ATOMIC if previous allocation on @nid failed. */
> +	if (!can_block && time_before(jiffies, READ_ONCE(pisa_nodes[nid].next_alloc)))
> +		return ERR_PTR(-EAGAIN);

[Severity: Medium]
Does calling pisa_schedule_reservoir_node(nid) before checking
pisa_nodes[nid].next_alloc defeat the backoff for the refill work item when
the reservoir is empty?

When order-9 allocations on nid are failing under memory pressure or
fragmentation, a GFP_ATOMIC miss sets pisa_nodes[nid].next_alloc to back off
for 10ms.  However, every subsequent GFP_ATOMIC call to pisa_reservoir_pop()
during that backoff window still queues pisa_nodes[nid].work before hitting
the next_alloc check, and pisa_reservoir_refill_node() immediately breaks
out on __pisa_page_alloc() failure, allowing softirq callers to continuously
re-queue the worker and trigger repeated GFP_KERNEL | __GFP_RETRY_MAYFAIL
compaction attempts.

Checking next_alloc before pisa_schedule_reservoir_node(nid) (or having
refill failure also update next_alloc) would keep the worker from thrashing
during backoff.

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