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

[Mina Almasry <almasrymina@google.com>]

On Tue, Dec 17, 2024 at 4:38 PM David Wei <dw@davidwei.uk> wrote:
>
> From: Pavel Begunkov <asml.silence@gmail.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@gmail.com>
> Signed-off-by: David Wei <dw@davidwei.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. */

>  };
>
>  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:

Anyhow:

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

-- 
Thanks,
Mina