[RFC net-next v2 0/2] tg3: Link IRQs, NAPIs, and queues

[RFC net-next v2 0/2] tg3: Link IRQs, NAPIs, and queues

[Joe Damato]

Greetings:

This RFC v3 follows from a previous RFC [1] submission which I noticed
had an issue in patch 2.

Patch 1 is changed to wrap a long line at 80 characters. No functional
changes. As such, I retained Pavan Chebbi's Reviewed-by.

Patch 2 in RFC v2 had an issue where it used the index into tp->irq_cnt
as the rxq or txq index; this is incorrect. It does not need seem that
tg3 assigns explicit queue index to struct tg3_napi, so the least
invasive change I could think of included two running counters in
tg3_napi_enable and tg3_napi_disable.

This is required because netif_queue_set_napi expected the queue index
(0 to real_num_[rt]x_queues) to be passed in to associate queues IDs
with NAPI IDs. tg2_napi_disable is modified in the reverse order;
counting down queue indices.

I am open to other suggestions on implementation from broadcom, but
thought that this was the least disruptive change.

I've tested this change on my tg3 hardware and it seems to work, see
commit message for examples of how to test.

Thanks,
Joe

[1]: https://lore.kernel.org/netdev/20240925162048.16208-1-jdamato@fastly.com/


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

 drivers/net/ethernet/broadcom/tg3.c | 45 ++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 7 deletions(-)

-- 
2.25.1

[net-next v3 1/2] tg3: Link IRQs to NAPI instances

[Joe Damato]

Link IRQs to NAPI instances with netif_napi_set_irq. This information
can be queried with the netdev-genl API.

Compare the output of /proc/interrupts for my tg3 device with the output of
netdev-genl after applying this patch:

$ cat /proc/interrupts | grep eth0 | cut -f1 --delimiter=':'
 343
 344
 345
 346
 347

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

[{'id': 8197, 'ifindex': 2, 'irq': 347},
 {'id': 8196, 'ifindex': 2, 'irq': 346},
 {'id': 8195, 'ifindex': 2, 'irq': 345},
 {'id': 8194, 'ifindex': 2, 'irq': 344},
 {'id': 8193, 'ifindex': 2, 'irq': 343}]

Signed-off-by: Joe Damato <jdamato@fastly.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
---
 rfcv3:
   - wrapped the netif_napi_add call to 80 characters

 drivers/net/ethernet/broadcom/tg3.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 378815917741..6564072b47ba 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7413,9 +7413,11 @@ static void tg3_napi_init(struct tg3 *tp)
 {
 	int i;
 
-	netif_napi_add(tp->dev, &tp->napi[0].napi, tg3_poll);
-	for (i = 1; i < tp->irq_cnt; i++)
-		netif_napi_add(tp->dev, &tp->napi[i].napi, tg3_poll_msix);
+	for (i = 0; i < tp->irq_cnt; i++) {
+		netif_napi_add(tp->dev, &tp->napi[i].napi,
+			       i ? tg3_poll_msix : tg3_poll);
+		netif_napi_set_irq(&tp->napi[i].napi, tp->napi[i].irq_vec);
+	}
 }
 
 static void tg3_napi_fini(struct tg3 *tp)
-- 
2.25.1

[net-next v3 2/2] tg3: Link queues to NAPIs

[Joe Damato]

Link queues to NAPIs using the netdev-genl API so this information is
queryable.

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

[{'id': 0, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'},
 {'id': 2, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'},
 {'id': 3, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'},
 {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}]

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 37 +++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 6564072b47ba..12172783b9b6 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7395,18 +7395,47 @@ static int tg3_poll(struct napi_struct *napi, int budget)
 
 static void tg3_napi_disable(struct tg3 *tp)
 {
+	int txq_idx = tp->txq_cnt - 1;
+	int rxq_idx = tp->rxq_cnt - 1;
+	struct tg3_napi *tnapi;
 	int i;
 
-	for (i = tp->irq_cnt - 1; i >= 0; i--)
-		napi_disable(&tp->napi[i].napi);
+	for (i = tp->irq_cnt - 1; i >= 0; i--) {
+		tnapi = &tp->napi[i];
+		if (tnapi->tx_buffers) {
+			netif_queue_set_napi(tp->dev, txq_idx,
+					     NETDEV_QUEUE_TYPE_TX, NULL);
+			txq_idx--;
+		} else if (tnapi->rx_rcb) {
+			netif_queue_set_napi(tp->dev, rxq_idx,
+					     NETDEV_QUEUE_TYPE_RX, NULL);
+			rxq_idx--;
+		}
+		napi_disable(&tnapi->napi);
+	}
 }
 
 static void tg3_napi_enable(struct tg3 *tp)
 {
+	int txq_idx = 0, rxq_idx = 0;
+	struct tg3_napi *tnapi;
 	int i;
 
-	for (i = 0; i < tp->irq_cnt; i++)
-		napi_enable(&tp->napi[i].napi);
+	for (i = 0; i < tp->irq_cnt; i++) {
+		tnapi = &tp->napi[i];
+		napi_enable(&tnapi->napi);
+		if (tnapi->tx_buffers) {
+			netif_queue_set_napi(tp->dev, txq_idx,
+					     NETDEV_QUEUE_TYPE_TX,
+					     &tnapi->napi);
+			txq_idx++;
+		} else if (tnapi->rx_rcb) {
+			netif_queue_set_napi(tp->dev, rxq_idx,
+					     NETDEV_QUEUE_TYPE_RX,
+					     &tnapi->napi);
+			rxq_idx++;
+		}
+	}
 }
 
 static void tg3_napi_init(struct tg3 *tp)
-- 
2.25.1

Re: [net-next v3 2/2] tg3: Link queues to NAPIs

[Michael Chan]

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

On Sat, Oct 5, 2024 at 7:57 AM Joe Damato <jdamato@fastly.com> wrote:
>
> Link queues to NAPIs using the netdev-genl API so this information is
> queryable.
>
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --dump queue-get --json='{"ifindex": 2}'
>
> [{'id': 0, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'},
>  {'id': 1, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'},
>  {'id': 2, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'},
>  {'id': 3, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'},
>  {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}]
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>

>  static void tg3_napi_enable(struct tg3 *tp)
>  {
> +       int txq_idx = 0, rxq_idx = 0;
> +       struct tg3_napi *tnapi;
>         int i;
>
> -       for (i = 0; i < tp->irq_cnt; i++)
> -               napi_enable(&tp->napi[i].napi);
> +       for (i = 0; i < tp->irq_cnt; i++) {
> +               tnapi = &tp->napi[i];
> +               napi_enable(&tnapi->napi);
> +               if (tnapi->tx_buffers) {
> +                       netif_queue_set_napi(tp->dev, txq_idx,
> +                                            NETDEV_QUEUE_TYPE_TX,
> +                                            &tnapi->napi);
> +                       txq_idx++;
> +               } else if (tnapi->rx_rcb) {

Shouldn't this be "if" instead of "else if" ?  A napi can be for both
a TX ring and an RX ring in some cases.
Thanks.

> +                       netif_queue_set_napi(tp->dev, rxq_idx,
> +                                            NETDEV_QUEUE_TYPE_RX,
> +                                            &tnapi->napi);
> +                       rxq_idx++;
> +               }
> +       }
>  }

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [net-next v3 2/2] tg3: Link queues to NAPIs

[Joe Damato]

On Mon, Oct 07, 2024 at 12:30:09AM -0700, Michael Chan wrote:
> On Sat, Oct 5, 2024 at 7:57 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > Link queues to NAPIs using the netdev-genl API so this information is
> > queryable.
> >
> > $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
> >                          --dump queue-get --json='{"ifindex": 2}'
> >
> > [{'id': 0, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'},
> >  {'id': 1, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'},
> >  {'id': 2, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'},
> >  {'id': 3, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'},
> >  {'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'tx'}]
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> 
> >  static void tg3_napi_enable(struct tg3 *tp)
> >  {
> > +       int txq_idx = 0, rxq_idx = 0;
> > +       struct tg3_napi *tnapi;
> >         int i;
> >
> > -       for (i = 0; i < tp->irq_cnt; i++)
> > -               napi_enable(&tp->napi[i].napi);
> > +       for (i = 0; i < tp->irq_cnt; i++) {
> > +               tnapi = &tp->napi[i];
> > +               napi_enable(&tnapi->napi);
> > +               if (tnapi->tx_buffers) {
> > +                       netif_queue_set_napi(tp->dev, txq_idx,
> > +                                            NETDEV_QUEUE_TYPE_TX,
> > +                                            &tnapi->napi);
> > +                       txq_idx++;
> > +               } else if (tnapi->rx_rcb) {
> 
> Shouldn't this be "if" instead of "else if" ?  A napi can be for both
> a TX ring and an RX ring in some cases.
> Thanks.

OK, but is this approach (with the running counters) acceptable for
rxq and txq numbering? If not, can you suggest an alternative?

Re: [net-next v3 2/2] tg3: Link queues to NAPIs

[Joe Damato]

On Mon, Oct 07, 2024 at 12:30:09AM -0700, Michael Chan wrote:
> On Sat, Oct 5, 2024 at 7:57 AM Joe Damato <jdamato@fastly.com> wrote:
> > +               if (tnapi->tx_buffers) {
> > +                       netif_queue_set_napi(tp->dev, txq_idx,
> > +                                            NETDEV_QUEUE_TYPE_TX,
> > +                                            &tnapi->napi);
> > +                       txq_idx++;
> > +               } else if (tnapi->rx_rcb) {
> 
> Shouldn't this be "if" instead of "else if" ?  A napi can be for both
> a TX ring and an RX ring in some cases.
> Thanks.

BTW: tg3 set_channels doesn't seem to support combined queues;
combined_count is not even examined in set_channels. But maybe
the link queue can be a combined queue, I don't know.

Regardless, I'll still make the change you requested as there is
similar code in tg3_request_irq.

But what I really would like to get feedback on is the rxq and txq
indexing with the running counters, please. That was called out
explicitly in the cover letter.

Thanks.

Re: [net-next v3 2/2] tg3: Link queues to NAPIs

[Michael Chan]

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

On Mon, Oct 7, 2024 at 7:23 AM Joe Damato <jdamato@fastly.com> wrote:
>
> On Mon, Oct 07, 2024 at 12:30:09AM -0700, Michael Chan wrote:
> > On Sat, Oct 5, 2024 at 7:57 AM Joe Damato <jdamato@fastly.com> wrote:
> > > +               if (tnapi->tx_buffers) {
> > > +                       netif_queue_set_napi(tp->dev, txq_idx,
> > > +                                            NETDEV_QUEUE_TYPE_TX,
> > > +                                            &tnapi->napi);
> > > +                       txq_idx++;
> > > +               } else if (tnapi->rx_rcb) {
> >
> > Shouldn't this be "if" instead of "else if" ?  A napi can be for both
> > a TX ring and an RX ring in some cases.
> > Thanks.
>
> BTW: tg3 set_channels doesn't seem to support combined queues;
> combined_count is not even examined in set_channels. But maybe
> the link queue can be a combined queue, I don't know.

tg3 is a little odd.  When there are more than 1 MSIX, TX starts from
vector 0 but RX starts at vector 1.  If there are more than 1 TX ring,
the 2nd TX ring is combined with the 1st RX ring and so on.

>
> Regardless, I'll still make the change you requested as there is
> similar code in tg3_request_irq.
>
> But what I really would like to get feedback on is the rxq and txq
> indexing with the running counters, please. That was called out
> explicitly in the cover letter.
>

It looks reasonable to me.  Thanks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [RFC net-next v2 0/2] tg3: Link IRQs, NAPIs, and queues

[Joe Damato]

On Sat, Oct 05, 2024 at 02:57:15PM +0000, Joe Damato wrote:
> Greetings:
> 
> This RFC v3 follows from a previous RFC [1] submission which I noticed
> had an issue in patch 2.

Apologies; looks like I botched the subject lines on this series
somehow.

This should be RFC v3.