| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f81eccd-bc14-47a5-bc65-b159c79ce422@davidwei.uk>
Date: Wed, 1 May 2024 18:04:21 -0700
From: David Wei <dw@...idwei.uk>
To: Mina Almasry <almasrymina@...gle.com>
Cc: netdev@...r.kernel.org, Michael Chan <michael.chan@...adcom.com>,
Pavan Chebbi <pavan.chebbi@...adcom.com>,
Andy Gospodarek <andrew.gospodarek@...adcom.com>,
Shailend Chand <shailend@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [RFC PATCH net-next v1 3/3] netdev: add netdev_rx_queue_restart()
On 2024-04-30 11:15 am, Mina Almasry wrote:
> On Mon, Apr 29, 2024 at 6:07 PM David Wei <dw@...idwei.uk> wrote:
>>
>> From: Mina Almasry <almasrymina@...gle.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@...idwei.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.
Powered by blists - more mailing lists