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 Fri, Jan 24, 2025 at 2:18 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> 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.
>>
>
> Ah, I see. I wonder if we should make this part of the API via comment
> and/or add DEBUG_NET_WARN_ON to catch misuse, something like:
>
> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
> index ed4cd114180a..3919ca302e95 100644
> --- a/include/net/page_pool/types.h
> +++ b/include/net/page_pool/types.h
> @@ -257,6 +257,10 @@ struct xdp_mem_info;
>
>  #ifdef CONFIG_PAGE_POOL
>  void page_pool_disable_direct_recycling(struct page_pool *pool);
> +
> +/* page_pool_destroy or page_pool_disable_direct_recycling must be
> called before
> + * netif_napi_del if pool->p.napi is set.
> + */
>  void page_pool_destroy(struct page_pool *pool);
>  void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *),
>                            const struct xdp_mem_info *mem);
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 5c4b788b811b..dc82767b2516 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -1161,6 +1161,8 @@ void page_pool_destroy(struct page_pool *pool)
>         if (!page_pool_put(pool))
>                 return;
>
> +       DEBUG_NET_WARN_ON(pool->p.napi && !napi_is_valid(pool->p.napi));
> +
>         page_pool_disable_direct_recycling(pool);
>         page_pool_free_frag(pool);

Yeah, good idea; care to send a proper patch? :)

> I also took a quick spot check - which could be wrong - but it seems
> to me both gve and bnxt free the napi before destroying the pool :(

Right, that fits with what Yunsheng found over in that other thread, at
least (for bnxt).

> But I think this entire discussion is unrelated to this patch, so and
> the mutex sync in this patch seems necessary for the page_pool_user.c
> code which runs outside of softirq context:
>
> Reviewed-by: Mina Almasry <almasrymina@google.com>

Yeah, didn't really mean this comment to have anything to do with this
patch, just mentioned it since you were talking about the data path :)

For this patch, I agree, the mutex seems fine:

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>