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

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/9/27 17:58, Ilias Apalodimas wrote:

...

>>
>>> importantly, though, why does struct page need to know about this?
>>> Can't we have the same information in page pool?
>>> When the driver allocates pages it does via page_pool_dev_alloc_XXXXX
>>> or something similar. Cant we do what you suggest here ? IOW when we
>>> allocate a page we put it in a list, and when that page returns to
>>> page_pool (and it's mapped) we remove it.
>>
>> Yes, that is the basic idea, but the important part is how to do that
>> with less performance impact.
> 
> Yes, but do you think that keeping that list of allocated pages in
> struct page_pool will end up being more costly somehow compared to
> struct page?

I am not sure if I understand your above question here.
I am supposing the question is about what's the cost between using
single/doubly linked list for the inflight pages or using a array
for the inflight pages like this patch does using pool->items?
If I understand question correctly, the single/doubly linked list
is more costly than array as the page_pool case as my understanding.

For single linked list, it doesn't allow deleting a specific entry but
only support deleting the first entry and all the entries. It does support
lockless operation using llist, but have limitation as below:
https://elixir.bootlin.com/linux/v6.7-rc8/source/include/linux/llist.h#L13

For doubly linked list, it needs two pointer to support deleting a specific
entry and it does not support lockless operation.

For pool->items, as the alloc side is protected by NAPI context, and the
free side use item->pp_idx to ensure there is only one producer for each
item, which means for each item in pool->items, there is only one consumer
and one producer, which seems much like the case when the page is not
recyclable in __page_pool_put_page, we don't need a lock protection when
calling page_pool_return_page(), the 'struct page' is also one consumer
and one producer as the pool->items[item->pp_idx] does:
https://elixir.bootlin.com/linux/v6.7-rc8/source/net/core/page_pool.c#L645

We only need a lock protection when page_pool_destroy() is called to
check if there is inflight page to be unmapped as a consumer, and the
__page_pool_put_page() may also called to unmapped the inflight page as
another consumer, there is why the 'destroy_lock' is added for protection
when pool->destroy_cnt > 0.

> 
> Thanks
> /Ilias