netdev - Re: [RFC net-next 2/2] igc: Link queues to NAPI instances

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zwa3sW-4s7oqktX3@LQ3V64L9R2>
Date: Wed, 9 Oct 2024 10:04:49 -0700
From: Joe Damato <jdamato@...tly.com>
To: Kurt Kanzenbach <kurt@...utronix.de>
Cc: netdev@...r.kernel.org, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"moderated list:INTEL ETHERNET DRIVERS" <intel-wired-lan@...ts.osuosl.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC net-next 2/2] igc: Link queues to NAPI instances

On Mon, Oct 07, 2024 at 11:14:51AM +0200, Kurt Kanzenbach wrote:
> Hi Joe,
> 
> On Thu Oct 03 2024, Joe Damato wrote:
> > Link queues to NAPI instances via netdev-genl API so that users can
> > query this information with netlink:
> >
> > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> >                          --dump queue-get --json='{"ifindex": 2}'
> >
> > [{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'},
> >  {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'},
> >  {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'},
> >  {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'},
> >  {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'},
> >  {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'tx'},
> >  {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'},
> >  {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}]
> >
> > Since igc uses only combined queues, you'll note that the same NAPI ID
> > is present for both rx and tx queues at the same index, for example
> > index 0:
> >
> > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'},
> > {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'},
> >
> > Signed-off-by: Joe Damato <jdamato@...tly.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_main.c | 30 ++++++++++++++++++++---
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 7964bbedb16c..b3bd5bf29fa7 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -4955,6 +4955,7 @@ static int igc_sw_init(struct igc_adapter *adapter)
> >  void igc_up(struct igc_adapter *adapter)
> >  {
> >  	struct igc_hw *hw = &adapter->hw;
> > +	struct napi_struct *napi;
> >  	int i = 0;
> >  
> >  	/* hardware has been reset, we need to reload some things */
> > @@ -4962,8 +4963,17 @@ void igc_up(struct igc_adapter *adapter)
> >  
> >  	clear_bit(__IGC_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);
> > +		/* igc only supports combined queues, so link each NAPI to both
> > +		 * TX and RX
> > +		 */
> 
> igc has IGC_FLAG_QUEUE_PAIRS. For example there may be 2 queues
> configured, but 4 vectors active (and 4 IRQs). Is your patch working
> with that?  Can be tested easily with `ethtool -L <inf> combined 2` or
> by booting with only 2 CPUs.

I tested what you asked, here's what it looks like on my system:

16 core Intel(R) Core(TM) i7-1360P

lspci:
Ethernet controller: Intel Corporation Device 125c (rev 04)
                     Subsystem: Intel Corporation Device 3037

ethtool -i:
firmware-version: 2017:888d

$ sudo ethtool -L enp86s0 combined 2
$ sudo ethtool -l enp86s0
Channel parameters for enp86s0:
Pre-set maximums:
RX:		n/a
TX:		n/a
Other:		1
Combined:	4
Current hardware settings:
RX:		n/a
TX:		n/a
Other:		1
Combined:	2

$ cat /proc/interrupts | grep enp86s0 | cut --delimiter=":" -f1
 144
 145
 146
 147
 148

Note that IRQ 144 is the "other" IRQ, so if we ignore that one...
/proc/interrupts shows 4 IRQs, despite there being only 2 queues.

Querying netlink to see which IRQs map to which NAPIs:

$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
                         --dump napi-get --json='{"ifindex": 2}'
[{'id': 8200, 'ifindex': 2, 'irq': 148},
 {'id': 8199, 'ifindex': 2, 'irq': 147},
 {'id': 8198, 'ifindex': 2, 'irq': 146},
 {'id': 8197, 'ifindex': 2, 'irq': 145}]

This suggests that all 4 IRQs are assigned to a NAPI (this mapping
happens due to netif_napi_set_irq in patch 1).

Now query the queues and which NAPIs they are associated with (which
is what patch 2 adds):

$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \ 
                         --dump queue-get --json='{"ifindex": 2}'
[{'id': 0, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8198, 'type': 'rx'},
 {'id': 0, 'ifindex': 2, 'napi-id': 8197, 'type': 'tx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8198, 'type': 'tx'}]

As you can see above, since the queues are combined and there are
only 2 of them, NAPI IDs 8197 and 8198 (which are triggered via IRQ
145 and 146) are displayed.

Does that cover the case you had in mind? If not let me know and I
am happy to test any other cases you like.

Thanks for taking a look at the code.

- Joe