Re: [PATCH net 2/2] page_pool: fix IOMMU crash when driver has already unbound

On 2024/9/19 18:54, Yunsheng Lin wrote:

On 2024/9/19 1:06, Ilias Apalodimas wrote:

quoted

Hi Yunsheng,

Thanks for looking into this!

On Wed, 18 Sept 2024 at 14:24, Yunsheng Lin [off-list ref] wrote:

quoted

Networking driver with page_pool support may hand over page
still with dma mapping to network stack and try to reuse that
page after network stack is done with it and passes it back
to page_pool to avoid the penalty of dma mapping/unmapping.

I think you can shorten this to "If recycling and DMA mapping are
enabled during the pool creation"

I am not sure if I understand the 'recycling' part here. Is the
'recycling' part referring to whether skb_mark_for_recycle() is
called to enable recycling for the skb? Is there still any driver
with page_pool support but doesn't call skb_mark_for_recycle()
when handing over page to network stack?

For the 'DMA mapping' part, as there is no space in 'struct
page' to track the inflight pages, so 'pp' in 'struct page'
is renamed to 'pp_item' to enable the tracking of inflight
page. I tried shortening this for 'pool->dma_map being false'
when coding, but it seems differentiating the same field in
'struct page' doesn't make much sense according to 'pool->dma_map'
as it means we might need to add an union in 'struct page' for
that to work and add additional checking to decide if it is 'pp'
or 'pp_item'.

quoted

quoted

With all the caching in the network stack, some pages may be
held in the network stack without returning to the page_pool
soon enough, and with VF disable causing the driver unbound,
the page_pool does not stop the driver from doing it's
unbounding work, instead page_pool uses workqueue to check
if there is some pages coming back from the network stack
periodically, if there is any, it will do the dma unmmapping
related cleanup work.

As mentioned in [1], attempting DMA unmaps after the driver
has already unbound may leak resources or at worst corrupt
memory. Fundamentally, the page pool code cannot allow DMA
mappings to outlive the driver they belong to.

Currently it seems there are at least two cases that the page
is not released fast enough causing dma unmmapping done after
driver has already unbound:
1. ipv4 packet defragmentation timeout: this seems to cause
   delay up to 30 secs:

2. skb_defer_free_flush(): this may cause infinite delay if
   there is no triggering for net_rx_action().

In order not to do the dma unmmapping after driver has already
unbound and stall the unloading of the networking driver, add
the pool->items array to record all the pages including the ones
which are handed over to network stack, so the page_pool can
do the dma unmmapping for those pages when page_pool_destroy()
is called.

So, I was thinking of a very similar idea. But what do you mean by
"all"? The pages that are still in caches (slow or fast) of the pool
will be unmapped during page_pool_destroy().

Yes, it includes the one in pool->alloc and pool->ring.

It worths mentioning that there is a semantics changing here:
Before this patch, there can be almost unlimited inflight pages used by
driver and network stack, as page_pool doesn't really track those pages.
After this patch, as we use a fixed-size pool->items array to track the
inflight pages, the inflight pages is limited by the pool->items, currently
the size of pool->items array is calculated as below in this patch:

+#define PAGE_POOL_MIN_ITEM_CNT	512
+	unsigned int item_cnt = (params->pool_size ? : 1024) +
+				PP_ALLOC_CACHE_SIZE + PAGE_POOL_MIN_ITEM_CNT;

Personally I would consider it is an advantage to limit how many pages which
are used by the driver and network stack, the problem seems to how to decide
the limited number of page used by network stack so that performance is not
impacted.

quoted

Don't we 'just' need a list of the inflight packets and their pages or
fragments? What we could do is go through that list and unmap these
pages during page_pool_destroy().

The main reason for that is to avoid the overhead of page_pool_item_del()
and page_pool_item_add() when allocing/freeing page from/to pool->alloc
and pool->ring.

Yes, including the pages in pool->ring seems to make the pool->ring
somewhat duplicated, maybe we can remove pool->ring if we can make
and prove 'pool->items' is performing better than pool->ring in the
future?

quoted

I'll have a closer look at the patch tomorrow

Thanks for the reviewing.

quoted

Thanks!
/Ilias