RE: [PATCH v2 net-next 07/11] net: ena: Add more information on TX timeouts

["Arinzon, David" <darinzon@amazon.com>]

> On Tue, Jan 30, 2024 at 09:53:49AM +0000, darinzon@amazon.com wrote:
> > From: David Arinzon <darinzon@amazon.com>
> >
> > The function responsible for polling TX completions might not receive
> > the CPU resources it needs due to higher priority tasks running on the
> > requested core.
> >
> > The driver might not be able to recognize such cases, but it can use
> > its state to suspect that they happened. If both conditions are met:
> >
> > - napi hasn't been executed more than the TX completion timeout value
> > - napi is scheduled (meaning that we've received an interrupt)
> >
> > Then it's more likely that the napi handler isn't scheduled because of
> > an overloaded CPU.
> > It was decided that for this case, the driver would wait twice as long
> > as the regular timeout before scheduling a reset.
> > The driver uses ENA_REGS_RESET_SUSPECTED_POLL_STARVATION reset
> reason
> > to indicate this case to the device.
> >
> > This patch also adds more information to the ena_tx_timeout() callback.
> > This function is called by the kernel when it detects that a specific
> > TX queue has been closed for too long.
> >
> > Signed-off-by: Shay Agroskin <shayagr@amazon.com>
> > Signed-off-by: David Arinzon <darinzon@amazon.com>
> > ---
> >  drivers/net/ethernet/amazon/ena/ena_netdev.c  | 77
> +++++++++++++++----
> >  .../net/ethernet/amazon/ena/ena_regs_defs.h   |  1 +
> >  2 files changed, 64 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > index 18acb76..ae9291b 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > @@ -47,19 +47,44 @@ static int ena_restore_device(struct ena_adapter
> > *adapter);
> >
> >  static void ena_tx_timeout(struct net_device *dev, unsigned int
> > txqueue)  {
> > +     enum ena_regs_reset_reason_types reset_reason =
> > + ENA_REGS_RESET_OS_NETDEV_WD;
> >       struct ena_adapter *adapter = netdev_priv(dev);
> > +     unsigned int time_since_last_napi, threshold;
> > +     struct ena_ring *tx_ring;
> > +     int napi_scheduled;
> > +
> > +     if (txqueue >= adapter->num_io_queues) {
> > +             netdev_err(dev, "TX timeout on invalid queue %u\n", txqueue);
> > +             goto schedule_reset;
> > +     }
> > +
> > +     threshold = jiffies_to_usecs(dev->watchdog_timeo);
> > +     tx_ring = &adapter->tx_ring[txqueue];
> > +
> > +     time_since_last_napi = jiffies_to_usecs(jiffies - tx_ring-
> >tx_stats.last_napi_jiffies);
> > +     napi_scheduled = !!(tx_ring->napi->state & NAPIF_STATE_SCHED);
> >
> > +     netdev_err(dev,
> > +                "TX q %d is paused for too long (threshold %u). Time since last
> napi %u usec. napi scheduled: %d\n",
> > +                txqueue,
> > +                threshold,
> > +                time_since_last_napi,
> > +                napi_scheduled);
> > +
> > +     if (threshold < time_since_last_napi && napi_scheduled) {
> > +             netdev_err(dev,
> > +                        "napi handler hasn't been called for a long time but is
> scheduled\n");
> > +                        reset_reason =
> > + ENA_REGS_RESET_SUSPECTED_POLL_STARVATION;
> 
> Hi David,
> 
> a nit from my side: the line above is indented one tab-stop too many.
> No need to respin just for this AFAIC.
> 

Hi Simon,

Thanks for pointing it out. Seems like I got carried away a bit with the
Indentation due to the print above it.

Thanks,
David

> > +     }
> > +schedule_reset:
> >       /* Change the state of the device to trigger reset
> >        * Check that we are not in the middle or a trigger already
> >        */
> > -
> >       if (test_and_set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))
> >               return;
> >
> > -     ena_reset_device(adapter, ENA_REGS_RESET_OS_NETDEV_WD);
> > +     ena_reset_device(adapter, reset_reason);
> >       ena_increase_stat(&adapter->dev_stats.tx_timeout, 1,
> > &adapter->syncp);
> > -
> > -     netif_err(adapter, tx_err, dev, "Transmit time out\n");
> >  }
> >
> 
> ...