| Message ID | 20250107160846.2223263-7-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: make sure we retain NAPI ordering on netdev->napi_list | expand |
On Tue, Jan 7, 2025 at 8:11 AM Jakub Kicinski <kuba@kernel.org> wrote: > > Add queue management API support. We need a way to reset queues > to test NAPI reordering, the queue management API provides a > handy scaffolding for that. > > Reviewed-by: Willem de Bruijn <willemb@google.com> > Acked-by: Stanislav Fomichev <sdf@fomichev.me> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Mina Almasry <almasrymina@google.com> > --- > v2: > - don't null-check page pool before page_pool_destroy() > - controled -> controlled > --- > drivers/net/netdevsim/netdev.c | 134 +++++++++++++++++++++++++++--- > drivers/net/netdevsim/netdevsim.h | 2 + > 2 files changed, 124 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c > index 7b80796dbe26..cfb079a34532 100644 > --- a/drivers/net/netdevsim/netdev.c > +++ b/drivers/net/netdevsim/netdev.c > @@ -359,25 +359,24 @@ static int nsim_poll(struct napi_struct *napi, int budget) > return done; > } > > -static int nsim_create_page_pool(struct nsim_rq *rq) > +static int nsim_create_page_pool(struct page_pool **p, struct napi_struct *napi) > { > - struct page_pool_params p = { > + struct page_pool_params params = { > .order = 0, > .pool_size = NSIM_RING_SIZE, > .nid = NUMA_NO_NODE, > - .dev = &rq->napi.dev->dev, > - .napi = &rq->napi, > + .dev = &napi->dev->dev, > + .napi = napi, > .dma_dir = DMA_BIDIRECTIONAL, > - .netdev = rq->napi.dev, > + .netdev = napi->dev, > }; > + struct page_pool *pool; > > - rq->page_pool = page_pool_create(&p); > - if (IS_ERR(rq->page_pool)) { > - int err = PTR_ERR(rq->page_pool); > + pool = page_pool_create(¶ms); > + if (IS_ERR(pool)) > + return PTR_ERR(pool); > > - rq->page_pool = NULL; > - return err; > - } > + *p = pool; > return 0; > } > > @@ -396,7 +395,7 @@ static int nsim_init_napi(struct netdevsim *ns) > for (i = 0; i < dev->num_rx_queues; i++) { > rq = ns->rq[i]; > > - err = nsim_create_page_pool(rq); > + err = nsim_create_page_pool(&rq->page_pool, &rq->napi); > if (err) > goto err_pp_destroy; > } > @@ -613,6 +612,116 @@ static void nsim_queue_free(struct nsim_rq *rq) > kfree(rq); > } > > +/* Queue reset mode is controlled by ns->rq_reset_mode. > + * - normal - new NAPI new pool (old NAPI enabled when new added) Nit, probably not worth a respin: Normal seems to me to delete old napi after the new one is added and enabled. queue stop -> napi_disable(old) queue alloc -> netif_napi_add_config(new) queue start -> napi_enable(new) queue free -> netif_napi_del(old) > + * - mode 1 - allocate new pool (NAPI is only disabled / enabled) > + * - mode 2 - new NAPI new pool (old NAPI removed before new added) > + * - mode 3 - new NAPI new pool (old NAPI disabled when new added) > + */ Which modes are 'correct' for a driver to implement? 2/3 is for testing only, as you note in the code, the add/del functions should really be called from alloc/free and not from queue_start. I assume modes normal and 1 are both correct implementations of the queue API and the driver gets to pick whether to reuse the napi instance or not? Asking because IIRC GVE implements mode 1, not what you consider 'normal'. -- Thanks, Mina
On Tue, 7 Jan 2025 14:53:09 -0800 Mina Almasry wrote: > > +/* Queue reset mode is controlled by ns->rq_reset_mode. > > + * - normal - new NAPI new pool (old NAPI enabled when new added) > > Nit, probably not worth a respin: Normal seems to me to delete old > napi after the new one is added and enabled. Yes, the comment doesn't really focus on removal order, it's less important. I may be missing your point.. > queue stop -> napi_disable(old) > queue alloc -> netif_napi_add_config(new) > queue start -> napi_enable(new) > queue free -> netif_napi_del(old) I think you have stop and alloc swapped here. > > + * - mode 1 - allocate new pool (NAPI is only disabled / enabled) > > + * - mode 2 - new NAPI new pool (old NAPI removed before new added) > > + * - mode 3 - new NAPI new pool (old NAPI disabled when new added) > > + */ > > Which modes are 'correct' for a driver to implement? 2/3 is for > testing only, as you note in the code, the add/del functions should > really be called from alloc/free and not from queue_start. I assume > modes normal and 1 are both correct implementations of the queue API > and the driver gets to pick whether to reuse the napi instance or not? > Asking because IIRC GVE implements mode 1, not what you consider > 'normal'. I can't think why any of them would be "incorrect" per se, but 2 and 3 are likely weird and unnatural. I thought mode 0 would simply be most common, maybe I shouldn't have called it "normal". But because of all the "modes" netdevsim implementation seemed too messy to be considered a blueprint..
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c index 7b80796dbe26..cfb079a34532 100644 --- a/drivers/net/netdevsim/netdev.c +++ b/drivers/net/netdevsim/netdev.c @@ -359,25 +359,24 @@ static int nsim_poll(struct napi_struct *napi, int budget) return done; } -static int nsim_create_page_pool(struct nsim_rq *rq) +static int nsim_create_page_pool(struct page_pool **p, struct napi_struct *napi) { - struct page_pool_params p = { + struct page_pool_params params = { .order = 0, .pool_size = NSIM_RING_SIZE, .nid = NUMA_NO_NODE, - .dev = &rq->napi.dev->dev, - .napi = &rq->napi, + .dev = &napi->dev->dev, + .napi = napi, .dma_dir = DMA_BIDIRECTIONAL, - .netdev = rq->napi.dev, + .netdev = napi->dev, }; + struct page_pool *pool; - rq->page_pool = page_pool_create(&p); - if (IS_ERR(rq->page_pool)) { - int err = PTR_ERR(rq->page_pool); + pool = page_pool_create(¶ms); + if (IS_ERR(pool)) + return PTR_ERR(pool); - rq->page_pool = NULL; - return err; - } + *p = pool; return 0; } @@ -396,7 +395,7 @@ static int nsim_init_napi(struct netdevsim *ns) for (i = 0; i < dev->num_rx_queues; i++) { rq = ns->rq[i]; - err = nsim_create_page_pool(rq); + err = nsim_create_page_pool(&rq->page_pool, &rq->napi); if (err) goto err_pp_destroy; } @@ -613,6 +612,116 @@ static void nsim_queue_free(struct nsim_rq *rq) kfree(rq); } +/* Queue reset mode is controlled by ns->rq_reset_mode. + * - normal - new NAPI new pool (old NAPI enabled when new added) + * - mode 1 - allocate new pool (NAPI is only disabled / enabled) + * - mode 2 - new NAPI new pool (old NAPI removed before new added) + * - mode 3 - new NAPI new pool (old NAPI disabled when new added) + */ +struct nsim_queue_mem { + struct nsim_rq *rq; + struct page_pool *pp; +}; + +static int +nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx) +{ + struct nsim_queue_mem *qmem = per_queue_mem; + struct netdevsim *ns = netdev_priv(dev); + int err; + + if (ns->rq_reset_mode > 3) + return -EINVAL; + + if (ns->rq_reset_mode == 1) + return nsim_create_page_pool(&qmem->pp, &ns->rq[idx]->napi); + + qmem->rq = nsim_queue_alloc(); + if (!qmem->rq) + return -ENOMEM; + + err = nsim_create_page_pool(&qmem->rq->page_pool, &qmem->rq->napi); + if (err) + goto err_free; + + if (!ns->rq_reset_mode) + netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx); + + return 0; + +err_free: + nsim_queue_free(qmem->rq); + return err; +} + +static void nsim_queue_mem_free(struct net_device *dev, void *per_queue_mem) +{ + struct nsim_queue_mem *qmem = per_queue_mem; + struct netdevsim *ns = netdev_priv(dev); + + page_pool_destroy(qmem->pp); + if (qmem->rq) { + if (!ns->rq_reset_mode) + netif_napi_del(&qmem->rq->napi); + page_pool_destroy(qmem->rq->page_pool); + nsim_queue_free(qmem->rq); + } +} + +static int +nsim_queue_start(struct net_device *dev, void *per_queue_mem, int idx) +{ + struct nsim_queue_mem *qmem = per_queue_mem; + struct netdevsim *ns = netdev_priv(dev); + + if (ns->rq_reset_mode == 1) { + ns->rq[idx]->page_pool = qmem->pp; + napi_enable(&ns->rq[idx]->napi); + return 0; + } + + /* netif_napi_add()/_del() should normally be called from alloc/free, + * here we want to test various call orders. + */ + if (ns->rq_reset_mode == 2) { + netif_napi_del(&ns->rq[idx]->napi); + netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx); + } else if (ns->rq_reset_mode == 3) { + netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx); + netif_napi_del(&ns->rq[idx]->napi); + } + + ns->rq[idx] = qmem->rq; + napi_enable(&ns->rq[idx]->napi); + + return 0; +} + +static int nsim_queue_stop(struct net_device *dev, void *per_queue_mem, int idx) +{ + struct nsim_queue_mem *qmem = per_queue_mem; + struct netdevsim *ns = netdev_priv(dev); + + napi_disable(&ns->rq[idx]->napi); + + if (ns->rq_reset_mode == 1) { + qmem->pp = ns->rq[idx]->page_pool; + page_pool_disable_direct_recycling(qmem->pp); + } else { + qmem->rq = ns->rq[idx]; + } + + return 0; +} + +static const struct netdev_queue_mgmt_ops nsim_queue_mgmt_ops = { + .ndo_queue_mem_size = sizeof(struct nsim_queue_mem), + .ndo_queue_mem_alloc = nsim_queue_mem_alloc, + .ndo_queue_mem_free = nsim_queue_mem_free, + .ndo_queue_start = nsim_queue_start, + .ndo_queue_stop = nsim_queue_stop, +}; + static ssize_t nsim_pp_hold_read(struct file *file, char __user *data, size_t count, loff_t *ppos) @@ -739,6 +848,7 @@ static int nsim_init_netdevsim(struct netdevsim *ns) ns->phc = phc; ns->netdev->netdev_ops = &nsim_netdev_ops; ns->netdev->stat_ops = &nsim_stat_ops; + ns->netdev->queue_mgmt_ops = &nsim_queue_mgmt_ops; err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev); if (err) diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h index 80fde64f4a7a..8c50969b1240 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -103,6 +103,8 @@ struct netdevsim { struct mock_phc *phc; struct nsim_rq **rq; + int rq_reset_mode; + u64 tx_packets; u64 tx_bytes; u64 tx_dropped;