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

[Joe Damato <jdamato@fastly.com>]

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.