Re: [BUG] Possible unsafe page_pool usage in octeontx2

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 24/08/2023 17.26, Alexander Lobakin wrote:
> From: Jesper Dangaard Brouer<hawk@kernel.org>
> Date: Wed, 23 Aug 2023 21:45:04 +0200
> 
>> (Cc Olek as he have changes in this code path)
> Thanks! I was reading the thread a bit on LKML, but being in the CC list
> is more convenient :D
> 

:D

>> On 23/08/2023 11.47, Sebastian Andrzej Siewior wrote:
>>> Hi,
>>>
>>> I've been looking at the page_pool locking.
>>>
>>> page_pool_alloc_frag() -> page_pool_alloc_pages() ->
>>> __page_pool_get_cached():
>>>
>>> There core of the allocation is:
>>> |         /* Caller MUST guarantee safe non-concurrent access, e.g.
>>> softirq */
>>> |         if (likely(pool->alloc.count)) {
>>> |                 /* Fast-path */
>>> |                 page = pool->alloc.cache[--pool->alloc.count];
>>>
>>> The access to the `cache' array and the `count' variable is not locked.
>>> This is fine as long as there only one consumer per pool. In my
>>> understanding the intention is to have one page_pool per NAPI callback
>>> to ensure this.
>>>
>> Yes, the intention is a single PP instance is "bound" to one RX-NAPI.
>
> Isn't that also a misuse of page_pool->p.napi? I thought it can be set
> only when page allocation and cache refill happen both inside the same
> NAPI polling function. Otx2 uses workqueues to refill the queues,
> meaning that consumer and producer can happen in different contexts or
> even threads and it shouldn't set p.napi.
> 

As Jakub wrote this otx2 driver doesn't set p.napi (so that part of the
problem cannot happen).

That said using workqueues to refill the queues is not compatible with
using page_pool_alloc APIs.  This need to be fixed in driver!

--Jesper