netdev - Re: [PATCH net-next v9 06/20] net: page_pool: add a mp hook to unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0698763f-0a98-4f0b-b287-6e131ef6b99f@davidwei.uk>
Date: Mon, 6 Jan 2025 15:34:49 -0800
From: David Wei <dw@...idwei.uk>
To: Mina Almasry <almasrymina@...gle.com>
Cc: io-uring@...r.kernel.org, netdev@...r.kernel.org,
 Jens Axboe <axboe@...nel.dk>, Pavel Begunkov <asml.silence@...il.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jesper Dangaard Brouer <hawk@...nel.org>, David Ahern <dsahern@...nel.org>,
 Stanislav Fomichev <stfomichev@...il.com>, Joe Damato <jdamato@...tly.com>,
 Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH net-next v9 06/20] net: page_pool: add a mp hook to
 unregister_netdevice*

On 2025-01-06 13:44, Mina Almasry wrote:
> On Tue, Dec 17, 2024 at 4:38 PM David Wei <dw@...idwei.uk> wrote:
>>
>> From: Pavel Begunkov <asml.silence@...il.com>
>>
>> Devmem TCP needs a hook in unregister_netdevice_many_notify() to upkeep
>> the set tracking queues it's bound to, i.e. ->bound_rxqs. Instead of
>> devmem sticking directly out of the genetic path, add a mp function.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
>> Signed-off-by: David Wei <dw@...idwei.uk>
>> ---
>>  include/net/page_pool/types.h |  3 +++
>>  net/core/dev.c                | 15 ++++++++++++++-
>>  net/core/devmem.c             | 36 ++++++++++++++++-------------------
>>  net/core/devmem.h             |  5 -----
>>  4 files changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
>> index a473ea0c48c4..140fec6857c6 100644
>> --- a/include/net/page_pool/types.h
>> +++ b/include/net/page_pool/types.h
>> @@ -152,12 +152,15 @@ struct page_pool_stats {
>>   */
>>  #define PAGE_POOL_FRAG_GROUP_ALIGN     (4 * sizeof(long))
>>
>> +struct netdev_rx_queue;
>> +
>>  struct memory_provider_ops {
>>         netmem_ref (*alloc_netmems)(struct page_pool *pool, gfp_t gfp);
>>         bool (*release_netmem)(struct page_pool *pool, netmem_ref netmem);
>>         int (*init)(struct page_pool *pool);
>>         void (*destroy)(struct page_pool *pool);
>>         int (*nl_report)(const struct page_pool *pool, struct sk_buff *rsp);
>> +       void (*uninstall)(void *mp_priv, struct netdev_rx_queue *rxq);
> 
> nit: the other params take struct page_pool *pool as input, and find
> the mp_priv in there if they need, it may be nice for consistency to
> continue to pass the entire pool to these ops.
> 
> AFAIU this is the first added non-mandatory op, right? Please specify
> it as so, maybe something like:
> 
> /* optional: called when the memory provider is uninstalled from the
> netdev_rx_queue due to the interface going down or otherwise. The
> memory provider may perform whatever cleanup necessary here if it
> needs to. */

Sounds good, I'll make the change in the next version.

> 
>>  };
>>
>>  struct pp_memory_provider_params {
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index c7f3dea3e0eb..aa082770ab1c 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -11464,6 +11464,19 @@ void unregister_netdevice_queue(struct net_device *dev, struct list_head *head)
>>  }
>>  EXPORT_SYMBOL(unregister_netdevice_queue);
>>
>> +static void dev_memory_provider_uninstall(struct net_device *dev)
>> +{
>> +       unsigned int i;
>> +
>> +       for (i = 0; i < dev->real_num_rx_queues; i++) {
>> +               struct netdev_rx_queue *rxq = &dev->_rx[i];
>> +               struct pp_memory_provider_params *p = &rxq->mp_params;
>> +
>> +               if (p->mp_ops && p->mp_ops->uninstall)
> 
> Previous versions of the code checked p->mp_priv to check if the queue
> is mp enabled or not, I guess you check mp_ops and that is set/cleared
> along with p->mp_priv. I guess that is fine. It would have been nicer,
> maybe, to have all the mp stuff behind one pointer/struct. I would
> dread some code path setting one of mp_[ops|priv] but forgetting to
> set the other... :shrug:

We can follow up with helpers that do the set/unset/check to make sure
it is consistent.

> 
> Anyhow:
> 
> Reviewed-by: Mina Almasry <almasrymina@...gle.com>
>