Re: [PATCH net] net: page_pool: don't try to stash the napi id

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Mina Almasry <almasrymina@google.com> writes:

> On Thu, Jan 23, 2025 at 3:16 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> Page ppol tried to cache the NAPI ID in page pool info to avoid
>
> Page pool
>
>> having a dependency on the life cycle of the NAPI instance.
>> Since commit under Fixes the NAPI ID is not populated until
>> napi_enable() and there's a good chance that page pool is
>> created before NAPI gets enabled.
>>
>> Protect the NAPI pointer with the existing page pool mutex,
>> the reading path already holds it. napi_id itself we need
>
> The reading paths in page_pool.c don't hold the lock, no? Only the
> reading paths in page_pool_user.c seem to do.
>
> I could not immediately wrap my head around why pool->p.napi can be
> accessed in page_pool_napi_local with no lock, but needs to be
> protected in the code in page_pool_user.c. It seems
> READ_ONCE/WRITE_ONCE protection is good enough to make sure
> page_pool_napi_local doesn't race with
> page_pool_disable_direct_recycling in a way that can crash (the
> reading code either sees a valid pointer or NULL). Why is that not
> good enough to also synchronize the accesses between
> page_pool_disable_direct_recycling and page_pool_nl_fill? I.e., drop
> the locking?

It actually seems that this is *not* currently the case. See the
discussion here:

https://lore.kernel.org/all/8734h8qgmz.fsf@toke.dk/

IMO (as indicated in the message linked above), we should require users
to destroy the page pool before freeing the NAPI memory, rather than add
additional synchronisation.

-Toke