| Message ID | 20240430010732.666512-4-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt: implement queue api | expand |
On Mon, Apr 29, 2024 at 6:07 PM David Wei <dw@davidwei.uk> wrote: > > From: Mina Almasry <almasrymina@google.com> > > Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is > taken from Mina's work in [1] with a slight modification of taking > rtnl_lock() during the queue stop and start ops. > > For bnxt specifically, if the firmware doesn't support > BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and > the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no > attempt to reset the whole device. > > [1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t > > Signed-off-by: David Wei <dw@davidwei.uk> > --- > include/net/netdev_rx_queue.h | 3 ++ > net/core/Makefile | 1 + > net/core/netdev_rx_queue.c | 58 +++++++++++++++++++++++++++++++++++ > 3 files changed, 62 insertions(+) > create mode 100644 net/core/netdev_rx_queue.c > > diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h > index aa1716fb0e53..e78ca52d67fb 100644 > --- a/include/net/netdev_rx_queue.h > +++ b/include/net/netdev_rx_queue.h > @@ -54,4 +54,7 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue) > return index; > } > #endif > + > +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq); > + > #endif > diff --git a/net/core/Makefile b/net/core/Makefile > index 21d6fbc7e884..f2aa63c167a3 100644 > --- a/net/core/Makefile > +++ b/net/core/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o > > obj-y += net-sysfs.o > obj-y += hotdata.o > +obj-y += netdev_rx_queue.o > obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o > obj-$(CONFIG_PROC_FS) += net-procfs.o > obj-$(CONFIG_NET_PKTGEN) += pktgen.o > diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c > new file mode 100644 > index 000000000000..9633fb36f6d1 > --- /dev/null > +++ b/net/core/netdev_rx_queue.c > @@ -0,0 +1,58 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include <linux/netdevice.h> > +#include <net/netdev_queues.h> > +#include <net/netdev_rx_queue.h> > + > +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq) > +{ > + void *new_mem; > + void *old_mem; > + int err; > + > + if (!dev->queue_mgmt_ops->ndo_queue_stop || > + !dev->queue_mgmt_ops->ndo_queue_mem_free || > + !dev->queue_mgmt_ops->ndo_queue_mem_alloc || > + !dev->queue_mgmt_ops->ndo_queue_start) > + return -EOPNOTSUPP; > + > + new_mem = dev->queue_mgmt_ops->ndo_queue_mem_alloc(dev, rxq); > + if (!new_mem) > + return -ENOMEM; > + > + rtnl_lock(); FWIW in my case this function is called from a netlink API which already has rtnl_lock, so maybe a check of rtnl_is_locked is good here rather than a relock. > + err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq, &old_mem); > + if (err) > + goto err_free_new_mem; > + > + err = dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, new_mem); > + if (err) > + goto err_start_queue; > + rtnl_unlock(); > + > + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem); > + > + return 0; > + > +err_start_queue: > + /* Restarting the queue with old_mem should be successful as we haven't > + * changed any of the queue configuration, and there is not much we can > + * do to recover from a failure here. > + * > + * WARN if the we fail to recover the old rx queue, and at least free > + * old_mem so we don't also leak that. > + */ > + if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) { > + WARN(1, > + "Failed to restart old queue in error path. RX queue %d may be unhealthy.", > + rxq); > + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem); > + } > + > +err_free_new_mem: > + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem); > + rtnl_unlock(); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(netdev_rx_queue_restart); Does stuff outside of core need this? I don't think so, right? I think you can drop EXPORT_SYMBOL_GPL. At that point the compiler may complain about an unused function, I think, so maybe __attribute__((unused)) would help there. I also think it's fine for this patch series to only add the ndos and to leave it to the devmem series to introduce this function, but I'm fine either way.
On 2024-04-30 11:15 am, Mina Almasry wrote: > On Mon, Apr 29, 2024 at 6:07 PM David Wei <dw@davidwei.uk> wrote: >> >> From: Mina Almasry <almasrymina@google.com> >> >> Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is >> taken from Mina's work in [1] with a slight modification of taking >> rtnl_lock() during the queue stop and start ops. >> >> For bnxt specifically, if the firmware doesn't support >> BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and >> the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no >> attempt to reset the whole device. >> >> [1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t >> >> Signed-off-by: David Wei <dw@davidwei.uk> >> --- >> include/net/netdev_rx_queue.h | 3 ++ >> net/core/Makefile | 1 + >> net/core/netdev_rx_queue.c | 58 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 62 insertions(+) >> create mode 100644 net/core/netdev_rx_queue.c >> >> diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h >> index aa1716fb0e53..e78ca52d67fb 100644 >> --- a/include/net/netdev_rx_queue.h >> +++ b/include/net/netdev_rx_queue.h >> @@ -54,4 +54,7 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue) >> return index; >> } >> #endif >> + >> +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq); >> + >> #endif >> diff --git a/net/core/Makefile b/net/core/Makefile >> index 21d6fbc7e884..f2aa63c167a3 100644 >> --- a/net/core/Makefile >> +++ b/net/core/Makefile >> @@ -19,6 +19,7 @@ obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o >> >> obj-y += net-sysfs.o >> obj-y += hotdata.o >> +obj-y += netdev_rx_queue.o >> obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o >> obj-$(CONFIG_PROC_FS) += net-procfs.o >> obj-$(CONFIG_NET_PKTGEN) += pktgen.o >> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c >> new file mode 100644 >> index 000000000000..9633fb36f6d1 >> --- /dev/null >> +++ b/net/core/netdev_rx_queue.c >> @@ -0,0 +1,58 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#include <linux/netdevice.h> >> +#include <net/netdev_queues.h> >> +#include <net/netdev_rx_queue.h> >> + >> +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq) >> +{ >> + void *new_mem; >> + void *old_mem; >> + int err; >> + >> + if (!dev->queue_mgmt_ops->ndo_queue_stop || >> + !dev->queue_mgmt_ops->ndo_queue_mem_free || >> + !dev->queue_mgmt_ops->ndo_queue_mem_alloc || >> + !dev->queue_mgmt_ops->ndo_queue_start) >> + return -EOPNOTSUPP; >> + >> + new_mem = dev->queue_mgmt_ops->ndo_queue_mem_alloc(dev, rxq); >> + if (!new_mem) >> + return -ENOMEM; >> + >> + rtnl_lock(); > > FWIW in my case this function is called from a netlink API which > already has rtnl_lock, so maybe a check of rtnl_is_locked is good here > rather than a relock. > >> + err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq, &old_mem); >> + if (err) >> + goto err_free_new_mem; >> + >> + err = dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, new_mem); >> + if (err) >> + goto err_start_queue; >> + rtnl_unlock(); >> + >> + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem); >> + >> + return 0; >> + >> +err_start_queue: >> + /* Restarting the queue with old_mem should be successful as we haven't >> + * changed any of the queue configuration, and there is not much we can >> + * do to recover from a failure here. >> + * >> + * WARN if the we fail to recover the old rx queue, and at least free >> + * old_mem so we don't also leak that. >> + */ >> + if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) { >> + WARN(1, >> + "Failed to restart old queue in error path. RX queue %d may be unhealthy.", >> + rxq); >> + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem); >> + } >> + >> +err_free_new_mem: >> + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem); >> + rtnl_unlock(); >> + >> + return err; >> +} >> +EXPORT_SYMBOL_GPL(netdev_rx_queue_restart); > > Does stuff outside of core need this? I don't think so, right? I think > you can drop EXPORT_SYMBOL_GPL. Not sure, we intend to call this from within io_uring. Does that require exporting or not? Later on I'll want to add something like netdev_rx_queue_set_memory_provider() which then calls netdev_rx_queue_restart(). When that happens I can remove the EXPORT_SYMBOL_GPL. > > At that point the compiler may complain about an unused function, I > think, so maybe __attribute__((unused)) would help there. > > I also think it's fine for this patch series to only add the ndos and > to leave it to the devmem series to introduce this function, but I'm > fine either way. > I'd like to agree on the netdev public API and merge alongside the queue api changes, separate to TCP devmem. Then that's one fewer deps between our main patchsets.
On Wed, May 1, 2024 at 6:04 PM David Wei <dw@davidwei.uk> wrote: > > On 2024-04-30 11:15 am, Mina Almasry wrote: > > On Mon, Apr 29, 2024 at 6:07 PM David Wei <dw@davidwei.uk> wrote: > >> > >> +err_start_queue: > >> + /* Restarting the queue with old_mem should be successful as we haven't > >> + * changed any of the queue configuration, and there is not much we can > >> + * do to recover from a failure here. > >> + * > >> + * WARN if the we fail to recover the old rx queue, and at least free > >> + * old_mem so we don't also leak that. > >> + */ > >> + if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) { > >> + WARN(1, > >> + "Failed to restart old queue in error path. RX queue %d may be unhealthy.", > >> + rxq); > >> + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem); > >> + } > >> + > >> +err_free_new_mem: > >> + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem); > >> + rtnl_unlock(); > >> + > >> + return err; > >> +} > >> +EXPORT_SYMBOL_GPL(netdev_rx_queue_restart); > > > > Does stuff outside of core need this? I don't think so, right? I think > > you can drop EXPORT_SYMBOL_GPL. > > Not sure, we intend to call this from within io_uring. Does that require > exporting or not? > I don't this this requires exporting, no. > Later on I'll want to add something like > netdev_rx_queue_set_memory_provider() which then calls > netdev_rx_queue_restart(). When that happens I can remove the > EXPORT_SYMBOL_GPL. > Sorry, I think if we don't need the EXPORT, then I think don't export in the first place. Removing an EXPORT is, AFAIU, tricky. Because if something is exported and then you unexport it could break an out of tree module/driver that developed a dependency on it. Not sure how much of a concern it really is. > > > > At that point the compiler may complain about an unused function, I > > think, so maybe __attribute__((unused)) would help there. > > > > I also think it's fine for this patch series to only add the ndos and > > to leave it to the devmem series to introduce this function, but I'm > > fine either way. > > > > I'd like to agree on the netdev public API and merge alongside the queue > api changes, separate to TCP devmem. Then that's one fewer deps between > our main patchsets. Ah, OK, sounds good. I was thinking both efforts would use the ndos, but I think you're thinking both efforts would use the netdev_rx_queue_restart. Yes, that sounds reasonable. -- Thanks, Mina
On Thu, 2 May 2024 09:46:46 -0700 Mina Almasry wrote: > Sorry, I think if we don't need the EXPORT, then I think don't export > in the first place. Removing an EXPORT is, AFAIU, tricky. Because if > something is exported and then you unexport it could break an out of > tree module/driver that developed a dependency on it. Not sure how > much of a concern it really is. FWIW don't worry about out of tree code, it's not a concern.
On Thu, 2 May 2024 17:22:41 -0700 Jakub Kicinski wrote: > On Thu, 2 May 2024 09:46:46 -0700 Mina Almasry wrote: > > Sorry, I think if we don't need the EXPORT, then I think don't export > > in the first place. Removing an EXPORT is, AFAIU, tricky. Because if > > something is exported and then you unexport it could break an out of > > tree module/driver that developed a dependency on it. Not sure how > > much of a concern it really is. > > FWIW don't worry about out of tree code, it's not a concern. That said (looking at the other thread), if there's no in-tree user, it's actually incorrect to add an export.
On 2024-05-02 17:49, Jakub Kicinski wrote: > On Thu, 2 May 2024 17:22:41 -0700 Jakub Kicinski wrote: >> On Thu, 2 May 2024 09:46:46 -0700 Mina Almasry wrote: >>> Sorry, I think if we don't need the EXPORT, then I think don't export >>> in the first place. Removing an EXPORT is, AFAIU, tricky. Because if >>> something is exported and then you unexport it could break an out of >>> tree module/driver that developed a dependency on it. Not sure how >>> much of a concern it really is. >> >> FWIW don't worry about out of tree code, it's not a concern. > > That said (looking at the other thread), if there's no in-tree > user, it's actually incorrect to add an export. Sorry, I forgot to remove it here, but I will do.
diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h index aa1716fb0e53..e78ca52d67fb 100644 --- a/include/net/netdev_rx_queue.h +++ b/include/net/netdev_rx_queue.h @@ -54,4 +54,7 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue) return index; } #endif + +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq); + #endif diff --git a/net/core/Makefile b/net/core/Makefile index 21d6fbc7e884..f2aa63c167a3 100644 --- a/net/core/Makefile +++ b/net/core/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o obj-y += net-sysfs.o obj-y += hotdata.o +obj-y += netdev_rx_queue.o obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o obj-$(CONFIG_PROC_FS) += net-procfs.o obj-$(CONFIG_NET_PKTGEN) += pktgen.o diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c new file mode 100644 index 000000000000..9633fb36f6d1 --- /dev/null +++ b/net/core/netdev_rx_queue.c @@ -0,0 +1,58 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <linux/netdevice.h> +#include <net/netdev_queues.h> +#include <net/netdev_rx_queue.h> + +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq) +{ + void *new_mem; + void *old_mem; + int err; + + if (!dev->queue_mgmt_ops->ndo_queue_stop || + !dev->queue_mgmt_ops->ndo_queue_mem_free || + !dev->queue_mgmt_ops->ndo_queue_mem_alloc || + !dev->queue_mgmt_ops->ndo_queue_start) + return -EOPNOTSUPP; + + new_mem = dev->queue_mgmt_ops->ndo_queue_mem_alloc(dev, rxq); + if (!new_mem) + return -ENOMEM; + + rtnl_lock(); + err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq, &old_mem); + if (err) + goto err_free_new_mem; + + err = dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, new_mem); + if (err) + goto err_start_queue; + rtnl_unlock(); + + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem); + + return 0; + +err_start_queue: + /* Restarting the queue with old_mem should be successful as we haven't + * changed any of the queue configuration, and there is not much we can + * do to recover from a failure here. + * + * WARN if the we fail to recover the old rx queue, and at least free + * old_mem so we don't also leak that. + */ + if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) { + WARN(1, + "Failed to restart old queue in error path. RX queue %d may be unhealthy.", + rxq); + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem); + } + +err_free_new_mem: + dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem); + rtnl_unlock(); + + return err; +} +EXPORT_SYMBOL_GPL(netdev_rx_queue_restart);