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.xil...@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 <i...@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.xil...@gmail.com>
--
Martin Habets <habetsm.xil...@gmail.com>
