[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 v2 follows from a PATCH submission which received some feedback
from broadcom on shortening the patch.

Patch 1 is a revised, shortened version of the original I submit a few
weeks ago [1].

Patch 2 is a new patch to link queues to NAPI instances. See the commit
message for more details and sample output.

I am sending them together as I plan to send them a series when net-next
reopens next week unless there is any feedback or changes requested.

I am hoping that broadcom would be able to take a look before then so
that these patches will be ready for official submission next week :)

Thanks,
Joe

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

rfv2:
  - Switched to RFC (net-next is closed).
  - Patch 1 incorporated the feedback from Michael Chan in the thread
    linked above to reduce the number of lines of code added.
  - Added patch 2, which implements a new feature.

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

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

-- 
2.25.1

[RFC net-next v2 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=':'
 331
 332
 333
 334
 335

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

[{'id': 149, 'ifindex': 2, 'irq': 335},
 {'id': 148, 'ifindex': 2, 'irq': 334},
 {'id': 147, 'ifindex': 2, 'irq': 333},
 {'id': 146, 'ifindex': 2, 'irq': 332},
 {'id': 145, 'ifindex': 2, 'irq': 331}]

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

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 378815917741..ddf0bb65c929 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7413,9 +7413,10 @@ 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

Re: [RFC net-next v2 1/2] tg3: Link IRQs to NAPI instances

[Pavan Chebbi]

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

On Wed, Sep 25, 2024 at 9:51 PM Joe Damato <jdamato@fastly.com> wrote:
>
> 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=':'
>  331
>  332
>  333
>  334
>  335
>
> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>                          --dump napi-get --json='{"ifindex": 2}'
>
> [{'id': 149, 'ifindex': 2, 'irq': 335},
>  {'id': 148, 'ifindex': 2, 'irq': 334},
>  {'id': 147, 'ifindex': 2, 'irq': 333},
>  {'id': 146, 'ifindex': 2, 'irq': 332},
>  {'id': 145, 'ifindex': 2, 'irq': 331}]
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  drivers/net/ethernet/broadcom/tg3.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 378815917741..ddf0bb65c929 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -7413,9 +7413,10 @@ 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
>

Thank you. LGTM. You can use Reviewed-by: Pavan Chebbi
<pavan.chebbi@broadcom.com>

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

[RFC net-next v2 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, 'type': 'rx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'},
 {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'},
 {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'},
 {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}]

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

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index ddf0bb65c929..f78d7e8c40b2 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget)
 
 static void tg3_napi_disable(struct tg3 *tp)
 {
+	struct tg3_napi *tnapi;
 	int i;
 
-	for (i = tp->irq_cnt - 1; i >= 0; i--)
-		napi_disable(&tp->napi[i].napi);
+	ASSERT_RTNL();
+	for (i = tp->irq_cnt - 1; i >= 0; i--) {
+		tnapi = &tp->napi[i];
+		if (tnapi->tx_buffers)
+			netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL);
+		else if (tnapi->rx_rcb)
+			netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_RX, NULL);
+		napi_disable(&tnapi->napi);
+	}
 }
 
 static void tg3_napi_enable(struct tg3 *tp)
 {
+	struct tg3_napi *tnapi;
 	int i;
 
-	for (i = 0; i < tp->irq_cnt; i++)
-		napi_enable(&tp->napi[i].napi);
+	ASSERT_RTNL();
+	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, i, NETDEV_QUEUE_TYPE_TX, &tnapi->napi);
+		else if (tnapi->rx_rcb)
+			netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_RX, &tnapi->napi);
+	}
 }
 
 static void tg3_napi_init(struct tg3 *tp)
-- 
2.25.1

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

[Joe Damato]

On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato 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, 'type': 'rx'},
>  {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'},
>  {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'},
>  {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'},
>  {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}]
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index ddf0bb65c929..f78d7e8c40b2 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget)
>  
>  static void tg3_napi_disable(struct tg3 *tp)
>  {
> +	struct tg3_napi *tnapi;
>  	int i;
>  
> -	for (i = tp->irq_cnt - 1; i >= 0; i--)
> -		napi_disable(&tp->napi[i].napi);
> +	ASSERT_RTNL();
> +	for (i = tp->irq_cnt - 1; i >= 0; i--) {
> +		tnapi = &tp->napi[i];
> +		if (tnapi->tx_buffers)
> +			netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL);

It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi
will call it internally, so I'll remove it before sending this to
the list (barring any other feedback).

>  static void tg3_napi_enable(struct tg3 *tp)
>  {
> +	struct tg3_napi *tnapi;
>  	int i;
>  
> -	for (i = 0; i < tp->irq_cnt; i++)
> -		napi_enable(&tp->napi[i].napi);
> +	ASSERT_RTNL();
> +	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, i, NETDEV_QUEUE_TYPE_TX, &tnapi->napi);

Same as above.

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

[Pavan Chebbi]

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

On Fri, Sep 27, 2024 at 4:47 AM Joe Damato <jdamato@fastly.com> wrote:
>
> On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato 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, 'type': 'rx'},
> >  {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'},
> >  {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'},
> >  {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'},
> >  {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}]
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >  drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> > index ddf0bb65c929..f78d7e8c40b2 100644
> > --- a/drivers/net/ethernet/broadcom/tg3.c
> > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget)
> >
> >  static void tg3_napi_disable(struct tg3 *tp)
> >  {
> > +     struct tg3_napi *tnapi;
> >       int i;
> >
> > -     for (i = tp->irq_cnt - 1; i >= 0; i--)
> > -             napi_disable(&tp->napi[i].napi);
> > +     ASSERT_RTNL();
> > +     for (i = tp->irq_cnt - 1; i >= 0; i--) {
> > +             tnapi = &tp->napi[i];
> > +             if (tnapi->tx_buffers)
> > +                     netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL);
>
> It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi
> will call it internally, so I'll remove it before sending this to
> the list (barring any other feedback).

Thanks LGTM. You can use Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
>
> >  static void tg3_napi_enable(struct tg3 *tp)
> >  {
> > +     struct tg3_napi *tnapi;
> >       int i;
> >
> > -     for (i = 0; i < tp->irq_cnt; i++)
> > -             napi_enable(&tp->napi[i].napi);
> > +     ASSERT_RTNL();
> > +     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, i, NETDEV_QUEUE_TYPE_TX, &tnapi->napi);
>
> Same as above.

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

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

[Joe Damato]

On Fri, Sep 27, 2024 at 09:33:51AM +0530, Pavan Chebbi wrote:
> On Fri, Sep 27, 2024 at 4:47 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato 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, 'type': 'rx'},
> > >  {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'},
> > >  {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'},
> > >  {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'},
> > >  {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}]
> > >
> > > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > > ---
> > >  drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++----
> > >  1 file changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> > > index ddf0bb65c929..f78d7e8c40b2 100644
> > > --- a/drivers/net/ethernet/broadcom/tg3.c
> > > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > > @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget)
> > >
> > >  static void tg3_napi_disable(struct tg3 *tp)
> > >  {
> > > +     struct tg3_napi *tnapi;
> > >       int i;
> > >
> > > -     for (i = tp->irq_cnt - 1; i >= 0; i--)
> > > -             napi_disable(&tp->napi[i].napi);
> > > +     ASSERT_RTNL();
> > > +     for (i = tp->irq_cnt - 1; i >= 0; i--) {
> > > +             tnapi = &tp->napi[i];
> > > +             if (tnapi->tx_buffers)
> > > +                     netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL);
> >
> > It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi
> > will call it internally, so I'll remove it before sending this to
> > the list (barring any other feedback).
> 
> Thanks LGTM. You can use Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

Thanks, I've added your Reviewed-by for the official submission.

I'll mention this in the cover letter when net-next is open but the
only changes I've made to the patch I posted are:
  - Removal of ASSERT_RTNL (mentioned above, as it seems to be unnecessary)
  - Wrapped lines at 80 characters (cosmetic change only)

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

[Joe Damato]

On Fri, Sep 27, 2024 at 09:33:51AM +0530, Pavan Chebbi wrote:
> On Fri, Sep 27, 2024 at 4:47 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > On Wed, Sep 25, 2024 at 04:20:48PM +0000, Joe Damato 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, 'type': 'rx'},
> > >  {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'},
> > >  {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'},
> > >  {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'},
> > >  {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}]
> > >
> > > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > > ---
> > >  drivers/net/ethernet/broadcom/tg3.c | 24 ++++++++++++++++++++----
> > >  1 file changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> > > index ddf0bb65c929..f78d7e8c40b2 100644
> > > --- a/drivers/net/ethernet/broadcom/tg3.c
> > > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > > @@ -7395,18 +7395,34 @@ static int tg3_poll(struct napi_struct *napi, int budget)
> > >
> > >  static void tg3_napi_disable(struct tg3 *tp)
> > >  {
> > > +     struct tg3_napi *tnapi;
> > >       int i;
> > >
> > > -     for (i = tp->irq_cnt - 1; i >= 0; i--)
> > > -             napi_disable(&tp->napi[i].napi);
> > > +     ASSERT_RTNL();
> > > +     for (i = tp->irq_cnt - 1; i >= 0; i--) {
> > > +             tnapi = &tp->napi[i];
> > > +             if (tnapi->tx_buffers)
> > > +                     netif_queue_set_napi(tp->dev, i, NETDEV_QUEUE_TYPE_TX, NULL);
> >
> > It looks like the ASSERT_RTNL is unnecessary; netif_queue_set_napi
> > will call it internally, so I'll remove it before sending this to
> > the list (barring any other feedback).
> 
> Thanks LGTM. You can use Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

I noticed there's a misnumbering issue in the code.

Note the output from the first patch:

[{'id': 149, 'ifindex': 2, 'irq': 335},
 {'id': 148, 'ifindex': 2, 'irq': 334},
 {'id': 147, 'ifindex': 2, 'irq': 333},
 {'id': 146, 'ifindex': 2, 'irq': 332},
 {'id': 145, 'ifindex': 2, 'irq': 331}]

Note the output in the commit message above:

 [{'id': 0, 'ifindex': 2, 'type': 'rx'},
  {'id': 1, 'ifindex': 2, 'napi-id': 146, 'type': 'rx'},
  {'id': 2, 'ifindex': 2, 'napi-id': 147, 'type': 'rx'},
  {'id': 3, 'ifindex': 2, 'napi-id': 148, 'type': 'rx'},
  {'id': 0, 'ifindex': 2, 'napi-id': 145, 'type': 'tx'}]

Note that id 0 type: 'rx' has no napi-id associated with it, and in
the second block, NAPI ID 149 is nowhere to be found.

This is happening because the code in the driver does this:

  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, i, NETDEV_QUEUE_TYPE_TX,
                                     &tnapi->napi);

The code I added assumed that i is the txq or rxq index, but it's
not - it's the index into the array of struct tg3_napi.

Corrected, the code looks like something like this:

  int txq_idx = 0, rxq_idx = 0;
  [...]

  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++;
          [...]

I tested that and the output looks correct to me. However, what to
do about tg3_napi_disable ?

Probably something like this (txq only for brevity):

  int txq_idx = tp->txq_cnt - 1;
  [...]

  for (i = tp->irq_cnt - 1; i >= 0; i--) {
    [...]
    if (tnapi->tx_buffers) {
        netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX,
                             NULL);
        txq_idx--;
    }
    [...]

Does that seem correct to you? I wanted to ask before sending
another revision, since I am not a tg3 expert.

I will of course remove your Reviewed-by from this patch (but leave
it on patch 1 which is unmodified) when I resend it.

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

[Pavan Chebbi]

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

On Thu, Oct 3, 2024 at 4:51 AM Joe Damato <jdamato@fastly.com> wrote:
>

> This is happening because the code in the driver does this:
>
>   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, i, NETDEV_QUEUE_TYPE_TX,
>                                      &tnapi->napi);
>
> The code I added assumed that i is the txq or rxq index, but it's
> not - it's the index into the array of struct tg3_napi.

Yes, you are right..
>
> Corrected, the code looks like something like this:
>
>   int txq_idx = 0, rxq_idx = 0;
>   [...]
>
>   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++;
>           [...]
>
> I tested that and the output looks correct to me. However, what to
> do about tg3_napi_disable ?
>
> Probably something like this (txq only for brevity):
>
>   int txq_idx = tp->txq_cnt - 1;
>   [...]
>
>   for (i = tp->irq_cnt - 1; i >= 0; i--) {
>     [...]
>     if (tnapi->tx_buffers) {
>         netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX,
>                              NULL);
>         txq_idx--;
>     }
>     [...]
>
> Does that seem correct to you? I wanted to ask before sending
> another revision, since I am not a tg3 expert.
>

The local counter variable for the ring ids might work because irqs
are requested sequentially.
Thinking out loud, a better way would be to save the tx/rx id inside
their struct tg3_napi in the tg3_request_irq() function.
And have a separate new function (I know you did something similar for
v1 of irq-napi linking) to link queues and napi.
I think it should work, and should help during de-linking also. Let me
know what you think.

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

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

[Joe Damato]

On Thu, Oct 03, 2024 at 09:56:40AM +0530, Pavan Chebbi wrote:
> On Thu, Oct 3, 2024 at 4:51 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> 
> > This is happening because the code in the driver does this:
> >
> >   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, i, NETDEV_QUEUE_TYPE_TX,
> >                                      &tnapi->napi);
> >
> > The code I added assumed that i is the txq or rxq index, but it's
> > not - it's the index into the array of struct tg3_napi.
> 
> Yes, you are right..
> >
> > Corrected, the code looks like something like this:
> >
> >   int txq_idx = 0, rxq_idx = 0;
> >   [...]
> >
> >   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++;
> >           [...]
> >
> > I tested that and the output looks correct to me. However, what to
> > do about tg3_napi_disable ?
> >
> > Probably something like this (txq only for brevity):
> >
> >   int txq_idx = tp->txq_cnt - 1;
> >   [...]
> >
> >   for (i = tp->irq_cnt - 1; i >= 0; i--) {
> >     [...]
> >     if (tnapi->tx_buffers) {
> >         netif_queue_set_napi(tp->dev, txq_idx, NETDEV_QUEUE_TYPE_TX,
> >                              NULL);
> >         txq_idx--;
> >     }
> >     [...]
> >
> > Does that seem correct to you? I wanted to ask before sending
> > another revision, since I am not a tg3 expert.
> >
> 
> The local counter variable for the ring ids might work because irqs
> are requested sequentially.

Yea, my proposal relies on the sequential ordering.

> Thinking out loud, a better way would be to save the tx/rx id inside
> their struct tg3_napi in the tg3_request_irq() function.

I think that could work, yes. I wasn't sure if you'd be open to such
a change.

It seems like in that case, though, we'd need to add some state
somewhere.

It's not super clear to me where the appropriate place for the state
would be because tg3_request_irq is called in a couple places (like
tg3_test_interrupt).

Another option would be to modify tg3_enable_msix and modify:

  for (i = 0; i < tp->irq_max; i++)
          tp->napi[i].irq_vec = msix_ent[i].vector;

But, all of that is still a bit invasive compared to the running
rxq_idx txq_idx counters I proposed in my previous message.

I am open to doing whatever you suggest/prefer, though, since it is
your driver after all :)

> And have a separate new function (I know you did something similar for
> v1 of irq-napi linking) to link queues and napi.
> I think it should work, and should help during de-linking also. Let me
> know what you think.

I think it's possible, it's just disruptive and it's not clear if
it's worth it? Some other code path might break and it might be fine
to just rely on the sequential indexing? Not sure.

Let me know what you think; thanks for taking the time to review and
respond.

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

[Pavan Chebbi]

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

> > The local counter variable for the ring ids might work because irqs
> > are requested sequentially.
>
> Yea, my proposal relies on the sequential ordering.
>
> > Thinking out loud, a better way would be to save the tx/rx id inside
> > their struct tg3_napi in the tg3_request_irq() function.
>
> I think that could work, yes. I wasn't sure if you'd be open to such
> a change.
>
> It seems like in that case, though, we'd need to add some state
> somewhere.
>
> It's not super clear to me where the appropriate place for the state
> would be because tg3_request_irq is called in a couple places (like
> tg3_test_interrupt).
>
> Another option would be to modify tg3_enable_msix and modify:
>
>   for (i = 0; i < tp->irq_max; i++)
>           tp->napi[i].irq_vec = msix_ent[i].vector;
Hi Joe, not in favor of this change.
>
> But, all of that is still a bit invasive compared to the running
> rxq_idx txq_idx counters I proposed in my previous message.
>
> I am open to doing whatever you suggest/prefer, though, since it is
> your driver after all :)
>
> > And have a separate new function (I know you did something similar for
> > v1 of irq-napi linking) to link queues and napi.
> > I think it should work, and should help during de-linking also. Let me
> > know what you think.
>
> I think it's possible, it's just disruptive and it's not clear if
> it's worth it? Some other code path might break and it might be fine
> to just rely on the sequential indexing? Not sure.
>
I don't have strong opposition to your proposal of using local counters.
Just that an alternate solution like what I suggested may look less
arbitrary, imo.
So if you want to use the local counters you may go ahead unless
Michael has any other suggestions.

> Let me know what you think; thanks for taking the time to review and
> respond.

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

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

[Joe Damato]

On Fri, Oct 04, 2024 at 09:03:58PM +0530, Pavan Chebbi wrote:

[...]

> > > Thinking out loud, a better way would be to save the tx/rx id inside
> > > their struct tg3_napi in the tg3_request_irq() function.
> >
> > I think that could work, yes. I wasn't sure if you'd be open to such
> > a change.
> >
> > It seems like in that case, though, we'd need to add some state
> > somewhere.
> >
> > It's not super clear to me where the appropriate place for the state
> > would be because tg3_request_irq is called in a couple places (like
> > tg3_test_interrupt).
> >
> > Another option would be to modify tg3_enable_msix and modify:
> >
> >   for (i = 0; i < tp->irq_max; i++)
> >           tp->napi[i].irq_vec = msix_ent[i].vector;
> Hi Joe, not in favor of this change.

OK

[...]

> > I think it's possible, it's just disruptive and it's not clear if
> > it's worth it? Some other code path might break and it might be fine
> > to just rely on the sequential indexing? Not sure.
> >
> I don't have strong opposition to your proposal of using local counters.
> Just that an alternate solution like what I suggested may look less
> arbitrary, imo.

I don't see where the state would be added for tracking the current
rxq_idx and txq_idx, though. And I don't necessarily agree that the
counters are arbitrary?

Unless tg3 is currently rx and tx index somewhere, then just
assuming they are linear seems fine ?

> So if you want to use the local counters you may go ahead unless
> Michael has any other suggestions.

I'll send another RFC and you can see what it looks like before
deciding.

[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

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.