| Message ID | 20250331194308.2026940-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: make memory provider install / close paths more common | expand |
On 03/31, Jakub Kicinski wrote: > Commit under Fixes solved the problem of spurious warnings when we > uninstall an MP from a device while its down. The __net_mp_close_rxq() > which is used by io_uring was not fixed. Move the fix over and reuse > __net_mp_close_rxq() in the devmem path. > > Fixes: a70f891e0fa0 ("net: devmem: do not WARN conditionally after netdev_rx_queue_restart()") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Acked-by: Stanislav Fomichev <sdf@fomichev.me>
On Mon, Mar 31, 2025 at 12:43 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Commit under Fixes solved the problem of spurious warnings when we > uninstall an MP from a device while its down. The __net_mp_close_rxq() > which is used by io_uring was not fixed. Move the fix over and reuse > __net_mp_close_rxq() in the devmem path. > > Fixes: a70f891e0fa0 ("net: devmem: do not WARN conditionally after netdev_rx_queue_restart()") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > net/core/devmem.c | 12 +++++------- > net/core/netdev_rx_queue.c | 17 +++++++++-------- > 2 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/net/core/devmem.c b/net/core/devmem.c > index f2ce3c2ebc97..6e27a47d0493 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c > @@ -116,21 +116,19 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding) > struct netdev_rx_queue *rxq; > unsigned long xa_idx; > unsigned int rxq_idx; > - int err; > > if (binding->list.next) > list_del(&binding->list); > > xa_for_each(&binding->bound_rxqs, xa_idx, rxq) { > - WARN_ON(rxq->mp_params.mp_priv != binding); > - > - rxq->mp_params.mp_priv = NULL; > - rxq->mp_params.mp_ops = NULL; > + const struct pp_memory_provider_params mp_params = { > + .mp_priv = binding, > + .mp_ops = &dmabuf_devmem_ops, > + }; > > rxq_idx = get_netdev_rx_queue_index(rxq); > > - err = netdev_rx_queue_restart(binding->dev, rxq_idx); > - WARN_ON(err && err != -ENETDOWN); > + __net_mp_close_rxq(binding->dev, rxq_idx, &mp_params); > } > > xa_erase(&net_devmem_dmabuf_bindings, binding->id); > diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c > index 556b5393ec9f..3e906c2950bd 100644 > --- a/net/core/netdev_rx_queue.c > +++ b/net/core/netdev_rx_queue.c > @@ -154,17 +154,13 @@ void __net_mp_close_rxq(struct net_device *dev, unsigned int ifq_idx, > const struct pp_memory_provider_params *old_p) > { > struct netdev_rx_queue *rxq; > + int err; > > if (WARN_ON_ONCE(ifq_idx >= dev->real_num_rx_queues)) > return; > > rxq = __netif_get_rx_queue(dev, ifq_idx); > - > - /* Callers holding a netdev ref may get here after we already > - * went thru shutdown via dev_memory_provider_uninstall(). > - */ > - if (dev->reg_state > NETREG_REGISTERED && > - !rxq->mp_params.mp_ops) > + if (!rxq->mp_params.mp_ops) > return; > > if (WARN_ON_ONCE(rxq->mp_params.mp_ops != old_p->mp_ops || > @@ -173,13 +169,18 @@ void __net_mp_close_rxq(struct net_device *dev, unsigned int ifq_idx, > > rxq->mp_params.mp_ops = NULL; > rxq->mp_params.mp_priv = NULL; > - WARN_ON(netdev_rx_queue_restart(dev, ifq_idx)); > + err = netdev_rx_queue_restart(dev, ifq_idx); > + WARN_ON(err && err != -ENETDOWN); > } > > void net_mp_close_rxq(struct net_device *dev, unsigned ifq_idx, > struct pp_memory_provider_params *old_p) > { > netdev_lock(dev); > - __net_mp_close_rxq(dev, ifq_idx, old_p); > + /* Callers holding a netdev ref may get here after we already > + * went thru shutdown via dev_memory_provider_uninstall(). > + */ > + if (dev->reg_state <= NETREG_REGISTERED) > + __net_mp_close_rxq(dev, ifq_idx, old_p); Not obvious to me why this check was moved. Do you expect to call __net_mp_close_rxq on an unregistered netdev and expect it to succeed in io_uring binding or something?
diff --git a/net/core/devmem.c b/net/core/devmem.c index f2ce3c2ebc97..6e27a47d0493 100644 --- a/net/core/devmem.c +++ b/net/core/devmem.c @@ -116,21 +116,19 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding) struct netdev_rx_queue *rxq; unsigned long xa_idx; unsigned int rxq_idx; - int err; if (binding->list.next) list_del(&binding->list); xa_for_each(&binding->bound_rxqs, xa_idx, rxq) { - WARN_ON(rxq->mp_params.mp_priv != binding); - - rxq->mp_params.mp_priv = NULL; - rxq->mp_params.mp_ops = NULL; + const struct pp_memory_provider_params mp_params = { + .mp_priv = binding, + .mp_ops = &dmabuf_devmem_ops, + }; rxq_idx = get_netdev_rx_queue_index(rxq); - err = netdev_rx_queue_restart(binding->dev, rxq_idx); - WARN_ON(err && err != -ENETDOWN); + __net_mp_close_rxq(binding->dev, rxq_idx, &mp_params); } xa_erase(&net_devmem_dmabuf_bindings, binding->id); diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c index 556b5393ec9f..3e906c2950bd 100644 --- a/net/core/netdev_rx_queue.c +++ b/net/core/netdev_rx_queue.c @@ -154,17 +154,13 @@ void __net_mp_close_rxq(struct net_device *dev, unsigned int ifq_idx, const struct pp_memory_provider_params *old_p) { struct netdev_rx_queue *rxq; + int err; if (WARN_ON_ONCE(ifq_idx >= dev->real_num_rx_queues)) return; rxq = __netif_get_rx_queue(dev, ifq_idx); - - /* Callers holding a netdev ref may get here after we already - * went thru shutdown via dev_memory_provider_uninstall(). - */ - if (dev->reg_state > NETREG_REGISTERED && - !rxq->mp_params.mp_ops) + if (!rxq->mp_params.mp_ops) return; if (WARN_ON_ONCE(rxq->mp_params.mp_ops != old_p->mp_ops || @@ -173,13 +169,18 @@ void __net_mp_close_rxq(struct net_device *dev, unsigned int ifq_idx, rxq->mp_params.mp_ops = NULL; rxq->mp_params.mp_priv = NULL; - WARN_ON(netdev_rx_queue_restart(dev, ifq_idx)); + err = netdev_rx_queue_restart(dev, ifq_idx); + WARN_ON(err && err != -ENETDOWN); } void net_mp_close_rxq(struct net_device *dev, unsigned ifq_idx, struct pp_memory_provider_params *old_p) { netdev_lock(dev); - __net_mp_close_rxq(dev, ifq_idx, old_p); + /* Callers holding a netdev ref may get here after we already + * went thru shutdown via dev_memory_provider_uninstall(). + */ + if (dev->reg_state <= NETREG_REGISTERED) + __net_mp_close_rxq(dev, ifq_idx, old_p); netdev_unlock(dev); }
Commit under Fixes solved the problem of spurious warnings when we uninstall an MP from a device while its down. The __net_mp_close_rxq() which is used by io_uring was not fixed. Move the fix over and reuse __net_mp_close_rxq() in the devmem path. Fixes: a70f891e0fa0 ("net: devmem: do not WARN conditionally after netdev_rx_queue_restart()") Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- net/core/devmem.c | 12 +++++------- net/core/netdev_rx_queue.c | 17 +++++++++-------- 2 files changed, 14 insertions(+), 15 deletions(-)