Re: [PATCH net-next] sfc: backport XDP EV queue sharing from the out-of-tree driver

[Martin Habets <habetsm.xilinx@gmail.com>]

On Sun, Dec 13, 2020 at 10:44:56AM -0800, Ivan Babrou wrote:
> On Sun, Dec 13, 2020 at 4:23 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
> >
> > On Thu, Dec 10, 2020 at 04:18:53PM -0800, Ivan Babrou wrote:
> > > Queue sharing behaviour already exists in the out-of-tree sfc driver,
> > > available under xdp_alloc_tx_resources module parameter.
> >
> > This comment is not relevant for in-tree patches. I'd also like to
> > make clear that we never intend to upstream any module parameters.
> 
> Would the following commit message be acceptable?
> 
> sfc: reduce the number of requested xdp ev queues
> 
> Without this change the driver tries to allocate too many queues,
> breaching the number of available msi-x interrupts on machines
> with many logical cpus and default adapter settings:
> 
> Insufficient resources for 12 XDP event queues (24 other channels, max 32)
> 
> Which in turn triggers EINVAL on XDP processing:
> 
> sfc 0000:86:00.0 ext0: XDP TX failed (-22)

Yes, that looks fine to me.

> > > This avoids the following issue on machines with many cpus:
> > >
> > > Insufficient resources for 12 XDP event queues (24 other channels, max 32)
> > >
> > > Which in turn triggers EINVAL on XDP processing:
> > >
> > > sfc 0000:86:00.0 ext0: XDP TX failed (-22)
> >
> > The code changes themselves are good.
> > The real limit that is hit here is with the number of MSI-X interrupts.
> > Reducing the number of event queues needed also reduces the number of
> > interrupts required, so this is a good thing.
> > Another way to get around this issue is to increase the number of
> > MSI-X interrupts allowed bu the NIC using the sfboot tool.
> 
> I've tried that, but on 5.10-rc7 with the in-tree driver both ethtool -l
> and sfboot are unable to work for some reason with sfc adapter.
> 
> The docs about the setting itself says you need to contact support
> to figure out the right values to use to make sure it works properly.

Indeed, our support may be better placed to help with this.

> What is your overall verdict on the patch? Should it be in the kernel
> or should users change msix-limit configuration? The configuration
> change requires breaking pcie lockdown measures as well, which is
> why I'd prefer for things to work out of the box.

The patch itself is good, as it saves on resources.

Thanks,
Martin

> Thanks!
> 
> >
> > Best regards,
> > Martin
> >
> > > Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> > > ---
> > >  drivers/net/ethernet/sfc/efx_channels.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/sfc/efx_channels.c
> > > b/drivers/net/ethernet/sfc/efx_channels.c
> > > index a4a626e9cd9a..1bfeee283ea9 100644
> > > --- a/drivers/net/ethernet/sfc/efx_channels.c
> > > +++ b/drivers/net/ethernet/sfc/efx_channels.c
> > > @@ -17,6 +17,7 @@
> > >  #include "rx_common.h"
> > >  #include "nic.h"
> > >  #include "sriov.h"
> > > +#include "workarounds.h"
> > >
> > >  /* This is the first interrupt mode to try out of:
> > >   * 0 => MSI-X
> > > @@ -137,6 +138,7 @@ static int efx_allocate_msix_channels(struct efx_nic *efx,
> > >  {
> > >   unsigned int n_channels = parallelism;
> > >   int vec_count;
> > > + int tx_per_ev;
> > >   int n_xdp_tx;
> > >   int n_xdp_ev;
> > >
> > > @@ -149,9 +151,9 @@ static int efx_allocate_msix_channels(struct efx_nic *efx,
> > >   * multiple tx queues, assuming tx and ev queues are both
> > >   * maximum size.
> > >   */
> > > -
> > > + tx_per_ev = EFX_MAX_EVQ_SIZE / EFX_TXQ_MAX_ENT(efx);
> > >   n_xdp_tx = num_possible_cpus();
> > > - n_xdp_ev = DIV_ROUND_UP(n_xdp_tx, EFX_MAX_TXQ_PER_CHANNEL);
> > > + n_xdp_ev = DIV_ROUND_UP(n_xdp_tx, tx_per_ev);
> > >
> > >   vec_count = pci_msix_vec_count(efx->pci_dev);
> > >   if (vec_count < 0)
> > > --
> > > 2.29.2
> >
> > --
> > Martin Habets <habetsm.xilinx@gmail.com>

-- 
Martin Habets <habetsm.xilinx@gmail.com>