[RFC net-next 0/2] igc: Link IRQs and queues to NAPIs

[RFC net-next 0/2] igc: Link IRQs and queues to NAPIs

[Joe Damato]

Greetings:

This is an RFC to get feedback before submitting an actual series and
because I have a question for igc maintainers, see below.

This series addss support for netdev-genl to igc so that userland apps
can query IRQ, queue, and NAPI instance relationships. This is useful
because developers who have igc NICs (for example, in their Intel NUCs)
who are working on epoll-based busy polling apps and using
SO_INCOMING_NAPI_ID, need access to this API to map NAPI IDs back to
queues.

See the commit messages of each patch for example output I got on my igc
hardware.

My question for maintainers:

In patch 2, the linking should be avoided for XDP queues. Is there a way
to test that somehow in the driver? I looked around a bit, but didn't
notice anything. Sorry if I'm missing something obvious.

Thanks,
Joe

Joe Damato (2):
  igc: Link IRQs to NAPI instances
  igc: Link queues to NAPI instances

 drivers/net/ethernet/intel/igc/igc.h      |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 33 ++++++++++++++++++++---
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.25.1

[RFC net-next 1/2] igc: Link IRQs to NAPI instances

[Joe Damato]

Link IRQs to NAPI instances via netdev-genl API so that users can query
this information with netlink.

Compare the output of /proc/interrupts (noting that IRQ 144 is the
"other" IRQ which does not appear to have a NAPI instance):

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

The output from netlink shows the mapping of NAPI IDs to IRQs (again
noting that 144 is absent as it is the "other" IRQ):

$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
                         --dump napi-get --json='{"ifindex": 2}'

[{'id': 8196, 'ifindex': 2, 'irq': 148},
 {'id': 8195, 'ifindex': 2, 'irq': 147},
 {'id': 8194, 'ifindex': 2, 'irq': 146},
 {'id': 8193, 'ifindex': 2, 'irq': 145}]

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/ethernet/intel/igc/igc.h      | 1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index eac0f966e0e4..e757ba53f165 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -593,6 +593,7 @@ struct igc_q_vector {
 
 	struct rcu_head rcu;    /* to avoid race with update stats on free */
 	char name[IFNAMSIZ + 9];
+	int irq;
 
 	/* for dynamic allocation of rings associated with this q_vector */
 	struct igc_ring ring[] ____cacheline_internodealigned_in_smp;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 6e70bca15db1..7964bbedb16c 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5576,6 +5576,9 @@ static int igc_request_msix(struct igc_adapter *adapter)
 				  q_vector);
 		if (err)
 			goto err_free;
+
+		netif_napi_set_irq(&q_vector->napi,
+				   adapter->msix_entries[vector].vector);
 	}
 
 	igc_configure_msix(adapter);
-- 
2.25.1

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

[Joe Damato]

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@fastly.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
+		 */
+		netif_queue_set_napi(adapter->netdev, i, NETDEV_QUEUE_TYPE_RX,
+				     napi);
+		netif_queue_set_napi(adapter->netdev, i, NETDEV_QUEUE_TYPE_TX,
+				     napi);
+	}
 
 	if (adapter->msix_entries)
 		igc_configure_msix(adapter);
@@ -5192,6 +5202,10 @@ void igc_down(struct igc_adapter *adapter)
 	for (i = 0; i < adapter->num_q_vectors; i++) {
 		if (adapter->q_vector[i]) {
 			napi_synchronize(&adapter->q_vector[i]->napi);
+			netif_queue_set_napi(netdev, i, NETDEV_QUEUE_TYPE_RX,
+					     NULL);
+			netif_queue_set_napi(netdev, i, NETDEV_QUEUE_TYPE_TX,
+					     NULL);
 			napi_disable(&adapter->q_vector[i]->napi);
 		}
 	}
@@ -6021,6 +6035,7 @@ static int __igc_open(struct net_device *netdev, bool resuming)
 	struct igc_adapter *adapter = netdev_priv(netdev);
 	struct pci_dev *pdev = adapter->pdev;
 	struct igc_hw *hw = &adapter->hw;
+	struct napi_struct *napi;
 	int err = 0;
 	int i = 0;
 
@@ -6056,8 +6071,15 @@ static int __igc_open(struct net_device *netdev, bool resuming)
 
 	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
+		 */
+		netif_queue_set_napi(netdev, i, NETDEV_QUEUE_TYPE_RX, napi);
+		netif_queue_set_napi(netdev, i, NETDEV_QUEUE_TYPE_TX, napi);
+	}
 
 	/* Clear any pending interrupts. */
 	rd32(IGC_ICR);
-- 
2.25.1

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

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

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

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [RFC net-next 0/2] igc: Link IRQs and queues to NAPIs

[Vinicius Costa Gomes]

Joe Damato <jdamato@fastly.com> writes:

> Greetings:
>
> This is an RFC to get feedback before submitting an actual series and
> because I have a question for igc maintainers, see below.
>
> This series addss support for netdev-genl to igc so that userland apps
> can query IRQ, queue, and NAPI instance relationships. This is useful
> because developers who have igc NICs (for example, in their Intel NUCs)
> who are working on epoll-based busy polling apps and using
> SO_INCOMING_NAPI_ID, need access to this API to map NAPI IDs back to
> queues.
>
> See the commit messages of each patch for example output I got on my igc
> hardware.
>
> My question for maintainers:
>
> In patch 2, the linking should be avoided for XDP queues. Is there a way
> to test that somehow in the driver? I looked around a bit, but didn't
> notice anything. Sorry if I'm missing something obvious.
>

From a quick look, it seems that you could "unlink" the XDP queues in
igc_xdp_enable_pool() and (re-)link them in igc_xdp_disable_poll().

Or just the existence of the flag IGC_RING_FLAG_AF_XDP_ZC in the rings
associated with the queue is enough?

I still have to take a better look at your work to help more, sorry.

> Thanks,
> Joe
>
> Joe Damato (2):
>   igc: Link IRQs to NAPI instances
>   igc: Link queues to NAPI instances
>
>  drivers/net/ethernet/intel/igc/igc.h      |  1 +
>  drivers/net/ethernet/intel/igc/igc_main.c | 33 ++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 4 deletions(-)
>
> -- 
> 2.25.1
>
>

Cheers,
-- 
Vinicius

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

[Joe Damato]

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

Re: [RFC net-next 0/2] igc: Link IRQs and queues to NAPIs

[Joe Damato]

On Mon, Oct 07, 2024 at 04:03:00PM -0700, Vinicius Costa Gomes wrote:
> Joe Damato <jdamato@fastly.com> writes:
> 
> > Greetings:
> >
> > This is an RFC to get feedback before submitting an actual series and
> > because I have a question for igc maintainers, see below.
> >
> > This series addss support for netdev-genl to igc so that userland apps
> > can query IRQ, queue, and NAPI instance relationships. This is useful
> > because developers who have igc NICs (for example, in their Intel NUCs)
> > who are working on epoll-based busy polling apps and using
> > SO_INCOMING_NAPI_ID, need access to this API to map NAPI IDs back to
> > queues.
> >
> > See the commit messages of each patch for example output I got on my igc
> > hardware.
> >
> > My question for maintainers:
> >
> > In patch 2, the linking should be avoided for XDP queues. Is there a way
> > to test that somehow in the driver? I looked around a bit, but didn't
> > notice anything. Sorry if I'm missing something obvious.
> >
> 
> From a quick look, it seems that you could "unlink" the XDP queues in
> igc_xdp_enable_pool() and (re-)link them in igc_xdp_disable_poll().

That approach seems reasonable to me, but I am not an igc expert by
any means :)

I checked and it seems that igc_xdp_enable_pool and
igc_xdp_disable_poll are only called while RTNL is held, which is
good because netif_queue_set_napi uses ASSERT_RTNL.
 
> Or just the existence of the flag IGC_RING_FLAG_AF_XDP_ZC in the rings
> associated with the queue is enough?

I didn't notice that flag, thanks for pointing that out.

It might be better to go the link/unlink route as you described
above, though.

> I still have to take a better look at your work to help more, sorry.

No worries, thanks for taking a look.

I'll implement what you suggested above and send another RFC.

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

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 5450 bytes --]

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@fastly.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:

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

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

[Joe Damato]

On Thu, Oct 10, 2024 at 09:08:20AM +0200, Kurt Kanzenbach wrote:
> 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@fastly.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:
> 
> 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?

So I definitely think the case where IGC_FLAG_QUEUE_PAIRS is enabled is
correct, that case is highlighted by the original commit message.

I think IGC_FLAG_QUEUE_PAIRS disabled was buggy, as you pointed out, and I've
made a change I'll include in the next RFC, which I believe fixes it.

Please see below for the case where IGC_FLAG_QUEUE_PAIRS is disabled and a
walk-through.

> There are four NAPI IDs which are triggered by
> the four IRQs.

I'm not an IGC expert and I appreciate your review/comments very much, so thank
you!

I don't think the number of queues I create with ethtool factors into whether
or not IGC_FLAG_QUEUE_PAIRS is enabled or not. Please forgive me for the length
of my message, but let me walk through the code to see if I've gotten it right,
including some debug output I added:

In igc_init_queue_configuration:

max_rss_queues = IGC_MAX_RX_QUEUES (4)

and

adapter->rss_queues = min of 4 or num_online_cpus

which I presume is 16 on my 16 core machine, so:

adapter->rss_queues = 4 (see below for debug output which verifies this)

In igc_set_flag_queue_pairs, the flag IGC_FLAG_QUEUE_PAIRS is set only if:

(adapter->rss_queues (4) > max_rss_queues(4) / 2) which simplifies
to (4 > 2), meaning the flag would be enabled regardless of the
number of queues I create with ethtool, as long as I boot my machine
with 16 cores available.

I verified this by adding debug output to igc_set_flag_queue_pairs and
igc_init_queue_configuration, which outputs:

igc 0000:56:00.0: IGC_FLAG_QUEUE_PAIRS on
igc 0000:56:00.0: max_rss_queues: 4, rss_queues: 4

That's at boot with the default number of combined queues of 4 (which is also
the hardware max).

The result of IGC_FLAG_QUEUE_PAIRS on was the result posted in the
original commit message of this patch and I believe that to be
correct.

The only place I can see that IGC_FLAG_QUEUE_PAIRS has any impact
(aside from ethtool IRQ coalescing, which we can ignore) is
igc_set_interrupt_capability:

  /* start with one vector for every Rx queue */
  numvecs = adapter->num_rx_queues;
  
  /* if Tx handler is separate add 1 for every Tx queue */
  if (!(adapter->flags & IGC_FLAG_QUEUE_PAIRS))
    numvecs += adapter->num_tx_queues;

In this case, the flag only has impact if it is _off_.

It impacts the number of vectors allocated, so I made a small change
to the driver, which I'll include in the next RFC to deal with the
IGC_FLAG_QUEUE_PAIRS off case.

In order to get IGC_FLAG_QUEUE_PAIRS off, I boot my machine with the grub
command line option "maxcpus=2", which should force the flag off.

Checking my debug output at boot to make sure:

igc 0000:56:00.0: IGC_FLAG_QUEUE_PAIRS off
igc 0000:56:00.0: max_rss_queues: 4, rss_queues: 2

So, now IGC_FLAG_QUEUE_PAIRS is off which should impact
igc_set_interrupt_capability and the vector calculation.

Let's check how things look at boot:

$ ethtool -l enp86s0 | tail -5
Current hardware settings:
RX:		n/a
TX:		n/a
Other:		1
Combined:	2

2 combined queues by default when I have 2 CPUs.

$ cat /proc/interrupts  | grep enp
 127:  enp86s0
 128:  enp86s0-rx-0
 129:  enp86s0-rx-1
 130:  enp86s0-tx-0
 131:  enp86s0-tx-1

1 other IRQ, and 2 IRQs for each of RX and TX.

Compare to netlink:

$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
                       --dump napi-get --json='{"ifindex": 2}'
[{'id': 8196, 'ifindex': 2, 'irq': 131},
 {'id': 8195, 'ifindex': 2, 'irq': 130},
 {'id': 8194, 'ifindex': 2, 'irq': 129},
 {'id': 8193, 'ifindex': 2, 'irq': 128}]

So the driver has 4 IRQs linked to 4 different NAPIs, let's check queues:

$ ./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': 0, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}]

In this case you can see that each RX and TX queue has a unique NAPI.

I think this is correct, but slightly confusing :) as ethtool
reports n/a for RX and TX and only reports a combined queue count,
but you were correct that there was a bug for this case in the code
I proposed in this RFC.

I think this new output looks correct and will include the adjusted
patch and a detailed commit message in the next RFC.

Let me know if you think the output looks right to you now?

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

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 7154 bytes --]

On Fri Oct 11 2024, Joe Damato wrote:
>> > 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?
>
> So I definitely think the case where IGC_FLAG_QUEUE_PAIRS is enabled is
> correct, that case is highlighted by the original commit message.

Yes.

>
> I think IGC_FLAG_QUEUE_PAIRS disabled was buggy, as you pointed out, and I've
> made a change I'll include in the next RFC, which I believe fixes it.

Great, thanks :).

>
> Please see below for the case where IGC_FLAG_QUEUE_PAIRS is disabled and a
> walk-through.
>
>> There are four NAPI IDs which are triggered by
>> the four IRQs.
>
> I'm not an IGC expert and I appreciate your review/comments very much, so thank
> you!
>
> I don't think the number of queues I create with ethtool factors into whether
> or not IGC_FLAG_QUEUE_PAIRS is enabled or not.

igc_ethtool_set_channels() sets adapter->rss_queues and calls
igc_set_flag_queue_pairs(). So, ethtool should influence it.

> Please forgive me for the length of my message, but let me walk
> through the code to see if I've gotten it right, including some debug
> output I added:
>
> In igc_init_queue_configuration:
>
> max_rss_queues = IGC_MAX_RX_QUEUES (4)
>
> and
>
> adapter->rss_queues = min of 4 or num_online_cpus
>
> which I presume is 16 on my 16 core machine, so:
>
> adapter->rss_queues = 4 (see below for debug output which verifies this)
>
> In igc_set_flag_queue_pairs, the flag IGC_FLAG_QUEUE_PAIRS is set only if:
>
> (adapter->rss_queues (4) > max_rss_queues(4) / 2) which simplifies
> to (4 > 2), meaning the flag would be enabled regardless of the
> number of queues I create with ethtool, as long as I boot my machine
> with 16 cores available.
>
> I verified this by adding debug output to igc_set_flag_queue_pairs and
> igc_init_queue_configuration, which outputs:
>
> igc 0000:56:00.0: IGC_FLAG_QUEUE_PAIRS on
> igc 0000:56:00.0: max_rss_queues: 4, rss_queues: 4
>
> That's at boot with the default number of combined queues of 4 (which is also
> the hardware max).
>
> The result of IGC_FLAG_QUEUE_PAIRS on was the result posted in the
> original commit message of this patch and I believe that to be
> correct.
>
> The only place I can see that IGC_FLAG_QUEUE_PAIRS has any impact
> (aside from ethtool IRQ coalescing, which we can ignore) is
> igc_set_interrupt_capability:
>
>   /* start with one vector for every Rx queue */
>   numvecs = adapter->num_rx_queues;
>   
>   /* if Tx handler is separate add 1 for every Tx queue */
>   if (!(adapter->flags & IGC_FLAG_QUEUE_PAIRS))
>     numvecs += adapter->num_tx_queues;
>
> In this case, the flag only has impact if it is _off_.
>
> It impacts the number of vectors allocated, so I made a small change
> to the driver, which I'll include in the next RFC to deal with the
> IGC_FLAG_QUEUE_PAIRS off case.
>
> In order to get IGC_FLAG_QUEUE_PAIRS off, I boot my machine with the grub
> command line option "maxcpus=2", which should force the flag off.
>
> Checking my debug output at boot to make sure:
>
> igc 0000:56:00.0: IGC_FLAG_QUEUE_PAIRS off
> igc 0000:56:00.0: max_rss_queues: 4, rss_queues: 2
>
> So, now IGC_FLAG_QUEUE_PAIRS is off which should impact
> igc_set_interrupt_capability and the vector calculation.
>
> Let's check how things look at boot:
>
> $ ethtool -l enp86s0 | tail -5
> Current hardware settings:
> RX:		n/a
> TX:		n/a
> Other:		1
> Combined:	2
>
> 2 combined queues by default when I have 2 CPUs.
>
> $ cat /proc/interrupts  | grep enp
>  127:  enp86s0
>  128:  enp86s0-rx-0
>  129:  enp86s0-rx-1
>  130:  enp86s0-tx-0
>  131:  enp86s0-tx-1
>
> 1 other IRQ, and 2 IRQs for each of RX and TX.
>
> Compare to netlink:
>
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                        --dump napi-get --json='{"ifindex": 2}'
> [{'id': 8196, 'ifindex': 2, 'irq': 131},
>  {'id': 8195, 'ifindex': 2, 'irq': 130},
>  {'id': 8194, 'ifindex': 2, 'irq': 129},
>  {'id': 8193, 'ifindex': 2, 'irq': 128}]
>
> So the driver has 4 IRQs linked to 4 different NAPIs, let's check queues:
>
> $ ./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': 0, 'ifindex': 2, 'napi-id': 8195, 'type': 'tx'},
>  {'id': 1, 'ifindex': 2, 'napi-id': 8196, 'type': 'tx'}]
>
> In this case you can see that each RX and TX queue has a unique NAPI.
>
> I think this is correct, but slightly confusing :) as ethtool
> reports n/a for RX and TX and only reports a combined queue count,
> but you were correct that there was a bug for this case in the code
> I proposed in this RFC.
>
> I think this new output looks correct and will include the adjusted
> patch and a detailed commit message in the next RFC.
>
> Let me know if you think the output looks right to you now?

Looks good to me now.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]