| 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
| ||
|
Date: Tue, 21 Nov 2023 13:26:27 -0800
From: "Nambiar, Amritha" <amritha.nambiar@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <pabeni@...hat.com>,
<sridhar.samudrala@...el.com>
Subject: Re: [net-next PATCH v8 02/10] net: Add queue and napi association
On 11/20/2023 3:54 PM, Jakub Kicinski wrote:
> On Thu, 16 Nov 2023 17:16:48 -0800 Amritha Nambiar wrote:
>> Add the napi pointer in netdev queue for tracking the napi
>> instance for each queue. This achieves the queue<->napi mapping.
>
> I took the series for a spin. I'll send a bnxt patch in a separate
> reply, please add it to the series.
>
Sure, will add the bnxt patch to v9.
> Three high level comments:
>
> - the netif_queue_set_napi() vs __netif_queue_set_napi() gave me pause;
> developers may be used to calling netif_*() functions from open/stop
> handlers, without worrying about locking.
> I think that netif_queue_set_napi() should assume rtnl_lock was
> already taken, as it will be in 90% of cases. A rare driver which
> does not hold it should take it locally for now.
>
Okay, will make these changes in v9.
> - drivers don't set real_num_*_queues to 0 when they go down,
> currently. So even tho we don't list any disabled queues when
> device is UP, we list queues when device is down.
> I mean:
>
> $ ifup eth0
> $ ethtool -L eth0 combined 4
> $ ./get-queues my-device
> ... will list 4 rx and 4 rx queues ...
> $ ethtool -L eth0 combined 2
> $ ./get-queues my-device
> ... will list 2 rx and 2 rx queues ...
> $ ifdown eth0
> $ ./get-queues my-device
> ... will list 2 rx and 2 rx queues ...
> ... even tho practically speaking there are no active queues ...
>
> I think we should skip listing queue and NAPI info of devices which
> are DOWN. Do you have any use case which would need looking at those?
>
So, currently, 'ethtool --show-channels' and 'ps -aef | grep napi' would
list all the queues and NAPIs if the device is DOWN. I think what you
are pointing at is:
<ifdown and ./get-queues> should show something similar to <ethtool -L
eth0 combined 0 (0 is not valid... but almost to that effect) and
./get-queues>.
But, 'ethtool -L' actually deletes the queues vs 'device DOWN' which
only disables or makes the queues inactive.
Maybe as a follow-up patch, would it be better to have an additional
parameter called 'state' for 'queues-get' and 'napi-get' that indicates
the queues or NAPIs as active/inactive. The queue/NAPI state would be
inherited from the device state. This way we can still list the
queues/NAPIs when the device is down, set/update parameter
configurations and then bring UP the device (in case where we stop
traffic and tune parameters).
Also, if in future, we have the interface to tune parameters per-queue
without full reset (of all queues or the device itself, as the hardware
supports this), the 'state' would report this for specific queue as
active/inactive. Maybe:
'queue-set' can set 'state = active' for a single queue '{"ifindex": 12,
"id": 0, "type": 0}' and start a queue.
> - We need a way to detach queues form NAPI. This is sort-of related to
> the above, maybe its not as crucial once we skip DOWN devices but
> nonetheless for symmetry we should let the driver clear the NAPI
> pointer. NAPIs may be allocated dynamically, the queue::napi pointer
> may get stale.
Okay, will handle this in v9.
>
> I hacked together the following for my local testing, feel free to fold
> appropriate parts into your patches:
>
Sure, thank you!
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 1a0603b3529d..2ed7a3aeec40 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2948,10 +2948,11 @@ static void
> ice_queue_set_napi(unsigned int queue_index, enum netdev_queue_type type,
> struct napi_struct *napi, bool locked)
> {
> - if (locked)
> - __netif_queue_set_napi(queue_index, type, napi);
> - else
> - netif_queue_set_napi(queue_index, type, napi);
> + if (!locked)
> + rtnl_lock();
> + netif_queue_set_napi(napi->dev, queue_index, type, napi);
> + if (!locked)
> + rtnl_unlock();
> }
>
> /**
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index dbc4ea74b8d6..e09a039a092a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2644,13 +2644,10 @@ static inline void *netdev_priv(const struct net_device *dev)
> */
> #define SET_NETDEV_DEVTYPE(net, devtype) ((net)->dev.type = (devtype))
>
> -void netif_queue_set_napi(unsigned int queue_index, enum netdev_queue_type type,
> +void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
> + enum netdev_queue_type type,
> struct napi_struct *napi);
>
> -void __netif_queue_set_napi(unsigned int queue_index,
> - enum netdev_queue_type type,
> - struct napi_struct *napi);
> -
> static inline void netif_napi_set_irq(struct napi_struct *napi, int irq)
> {
> napi->irq = irq;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 99ca59e18abf..bb93240c69b9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6400,25 +6400,27 @@ int dev_set_threaded(struct net_device *dev, bool threaded)
> EXPORT_SYMBOL(dev_set_threaded);
>
> /**
> - * __netif_queue_set_napi - Associate queue with the napi
> + * netif_queue_set_napi - Associate queue with the napi
> + * @dev: device to which NAPI and queue belong
> * @queue_index: Index of queue
> * @type: queue type as RX or TX
> - * @napi: NAPI context
> + * @napi: NAPI context, pass NULL to clear previously set NAPI
> *
> * Set queue with its corresponding napi context. This should be done after
> * registering the NAPI handler for the queue-vector and the queues have been
> * mapped to the corresponding interrupt vector.
> */
> -void __netif_queue_set_napi(unsigned int queue_index,
> - enum netdev_queue_type type,
> - struct napi_struct *napi)
> +void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
> + enum netdev_queue_type type,
> + struct napi_struct *napi)
> {
> - struct net_device *dev = napi->dev;
> struct netdev_rx_queue *rxq;
> struct netdev_queue *txq;
>
> - if (WARN_ON_ONCE(!dev))
> + if (WARN_ON_ONCE(napi && !napi->dev))
> return;
> + if (dev->reg_state >= NETREG_REGISTERED)
> + ASSERT_RTNL();
>
> switch (type) {
> case NETDEV_QUEUE_TYPE_RX:
> @@ -6433,15 +6435,6 @@ void __netif_queue_set_napi(unsigned int queue_index,
> return;
> }
> }
> -EXPORT_SYMBOL(__netif_queue_set_napi);
> -
> -void netif_queue_set_napi(unsigned int queue_index, enum netdev_queue_type type,
> - struct napi_struct *napi)
> -{
> - rtnl_lock();
> - __netif_queue_set_napi(queue_index, type, napi);
> - rtnl_unlock();
> -}
> EXPORT_SYMBOL(netif_queue_set_napi);
>
> void netif_napi_add_weight(struct net_device *dev, struct napi_struct *napi,
Powered by blists - more mailing lists