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

[Kurt Kanzenbach]

On Wed Oct 09 2024, Joe Damato wrote:
> 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@xxxxxxxxxx>
>> > ---
>> >  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:

Thanks.

>
> 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.

Is that really correct? There are four NAPI IDs which are triggered by
the four IRQs. Let's say we have:

 - IRQ: 145 -> NAPI 8197 -> Tx for queue 0
 - IRQ: 146 -> NAPI 8198 -> Rx for queue 0
 - IRQ: 147 -> NAPI 8199 -> Tx for queue 1
 - IRQ: 148 -> NAPI 8200 -> Rx for queue 1

My understanding is that this scheme is used when <= 2 queues are
configured. See IGC_FLAG_QUEUE_PAIRS.

My expectation would be some output like:

[{'id': 0, 'ifindex': 2, 'napi-id': 8197, 'type': 'tx'},
 {'id': 0, 'ifindex': 2, 'napi-id': 8198, 'type': 'rx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8199, 'type': 'tx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8200, 'type': 'rx'}]

Thanks,
Kurt

Attachment: signature.asc
Description: PGP signature