On Tue, Dec 17, 2024 at 4:38 PM David Wei <dw@xxxxxxxxxxx> wrote:
>
> From: Pavel Begunkov <asml.silence@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: David Wei <dw@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
Thanks,
Mina
