| 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: <Z6pJrRRqcHYhZWss@LQ3V64L9R2>
Date: Mon, 10 Feb 2025 10:47:09 -0800
From: Joe Damato <jdamato@...tly.com>
To: Kurt Kanzenbach <kurt@...utronix.de>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>,
Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [PATCH 2/3] igb: Link queues to NAPI instances
On Mon, Feb 10, 2025 at 10:19:36AM +0100, Kurt Kanzenbach wrote:
> Link queues to NAPI instances via netdev-genl API. This is required to use
> XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
> support") for details.
>
> This also allows users to query the info with netlink:
>
> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> | --dump queue-get --json='{"ifindex": 2}'
> |[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
> | {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]
>
> While at __igb_open() use RCT coding style.
>
> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.de>
> ---
> drivers/net/ethernet/intel/igb/igb.h | 2 ++
> drivers/net/ethernet/intel/igb/igb_main.c | 35 ++++++++++++++++++++++++++-----
> drivers/net/ethernet/intel/igb/igb_xsk.c | 2 ++
> 3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 02f340280d20a6f7e32bbd3dfcbb9c1c7b4c6662..79eca385a751bfdafdf384928b6cc1b350b22560 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -722,6 +722,8 @@ enum igb_boards {
>
> extern char igb_driver_name[];
>
> +void igb_set_queue_napi(struct igb_adapter *adapter, int q_idx,
> + struct napi_struct *napi);
> int igb_xmit_xdp_ring(struct igb_adapter *adapter,
> struct igb_ring *ring,
> struct xdp_frame *xdpf);
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index d4128d19cc08f62f95682069bb5ed9b8bbbf10cb..8e964484f4c9854e4e3e0b4f3e8785fe93bd1207 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2099,6 +2099,22 @@ static void igb_check_swap_media(struct igb_adapter *adapter)
> wr32(E1000_CTRL_EXT, ctrl_ext);
> }
>
> +void igb_set_queue_napi(struct igb_adapter *adapter, int vector,
> + struct napi_struct *napi)
> +{
> + struct igb_q_vector *q_vector = adapter->q_vector[vector];
> +
> + if (q_vector->rx.ring)
> + netif_queue_set_napi(adapter->netdev,
> + q_vector->rx.ring->queue_index,
> + NETDEV_QUEUE_TYPE_RX, napi);
> +
> + if (q_vector->tx.ring)
> + netif_queue_set_napi(adapter->netdev,
> + q_vector->tx.ring->queue_index,
> + NETDEV_QUEUE_TYPE_TX, napi);
> +}
> +
> /**
> * igb_up - Open the interface and prepare it to handle traffic
> * @adapter: board private structure
> @@ -2106,6 +2122,7 @@ static void igb_check_swap_media(struct igb_adapter *adapter)
> int igb_up(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> + struct napi_struct *napi;
> int i;
>
> /* hardware has been reset, we need to reload some things */
> @@ -2113,8 +2130,11 @@ int igb_up(struct igb_adapter *adapter)
>
> clear_bit(__IGB_DOWN, &adapter->state);
>
> - for (i = 0; i < adapter->num_q_vectors; i++)
> - napi_enable(&(adapter->q_vector[i]->napi));
> + for (i = 0; i < adapter->num_q_vectors; i++) {
> + napi = &adapter->q_vector[i]->napi;
> + napi_enable(napi);
> + igb_set_queue_napi(adapter, i, napi);
> + }
It looks like igb_ub is called from igb_io_resume (struct
pci_error_handlers). I don't know if RTNL is held in that path. If
its not, this could trip the ASSERT_RTNL in netif_queue_set_napi.
Can you check and see if this is an issue for that path?
igb_reinit_locked looks OK (as the name implies).
>
> if (adapter->flags & IGB_FLAG_HAS_MSIX)
> igb_configure_msix(adapter);
> @@ -2184,6 +2204,7 @@ void igb_down(struct igb_adapter *adapter)
> for (i = 0; i < adapter->num_q_vectors; i++) {
> if (adapter->q_vector[i]) {
> napi_synchronize(&adapter->q_vector[i]->napi);
> + igb_set_queue_napi(adapter, i, NULL);
> napi_disable(&adapter->q_vector[i]->napi);
Same question as above. It looks like igb_down is called from
igb_io_error_detected. I don't know if RTNL is held in that path. If
its not, it'll trip the ASSERT_RTNL in netif_queue_set_napi.
Can you check if that's an issue for this path, as well?
> }
> }
> @@ -4116,8 +4137,9 @@ static int igb_sw_init(struct igb_adapter *adapter)
> static int __igb_open(struct net_device *netdev, bool resuming)
> {
> struct igb_adapter *adapter = netdev_priv(netdev);
> - struct e1000_hw *hw = &adapter->hw;
> struct pci_dev *pdev = adapter->pdev;
> + struct e1000_hw *hw = &adapter->hw;
> + struct napi_struct *napi;
> int err;
> int i;
>
> @@ -4169,8 +4191,11 @@ static int __igb_open(struct net_device *netdev, bool resuming)
> /* From here on the code is the same as igb_up() */
> clear_bit(__IGB_DOWN, &adapter->state);
>
> - for (i = 0; i < adapter->num_q_vectors; i++)
> - napi_enable(&(adapter->q_vector[i]->napi));
> + for (i = 0; i < adapter->num_q_vectors; i++) {
> + napi = &adapter->q_vector[i]->napi;
> + napi_enable(napi);
> + igb_set_queue_napi(adapter, i, napi);
> + }
The above looks fine. __igb_open is called from __igb_resume which
takes care of RTNL. So, I think this part is fine.
> rd32(E1000_TSICR);
> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index 157d43787fa0b55a74714f69e9e7903b695fcf0a..a5ad090dfe94b6afc8194fe39d28cdd51c7067b0 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -45,6 +45,7 @@ static void igb_txrx_ring_disable(struct igb_adapter *adapter, u16 qid)
> synchronize_net();
>
> /* Rx/Tx share the same napi context. */
> + igb_set_queue_napi(adapter, qid, NULL);
> napi_disable(&rx_ring->q_vector->napi);
>
> igb_clean_tx_ring(tx_ring);
> @@ -78,6 +79,7 @@ static void igb_txrx_ring_enable(struct igb_adapter *adapter, u16 qid)
>
> /* Rx/Tx share the same napi context. */
> napi_enable(&rx_ring->q_vector->napi);
> + igb_set_queue_napi(adapter, qid, &rx_ring->q_vector->napi);
> }
These seem fine to me.
Powered by blists - more mailing lists