Re: [dpdk-dev] [PATCH] net/ena: enable multi segment in Tx offload flags

["Michał Krawczyk" <mk@semihalf.com>]

czw., 29 lip 2021 o 09:26 Olivier Matz <olivier.matz@6wind.com> napisał(a):
>
> Hi Michał,
>
> On Thu, Jul 29, 2021 at 08:40:00AM +0200, Michał Krawczyk wrote:
> > śr., 28 lip 2021 o 16:27 Olivier Matz <olivier.matz@6wind.com> napisał(a):
> > >
> > > Hi Michał,
> > >
> > > On Wed, Jul 14, 2021 at 12:02:32PM +0200, Michał Krawczyk wrote:
> > > > pon., 12 lip 2021 o 19:03 Ghalem Boudour <ghalem.boudour@6wind.com> napisał(a):
> > > > >
> > > > > The DPDK ENA driver does not provide multi-segment tx offload capability.
> > > > > Let's add DEV_TX_OFFLOAD_MULTI_SEGS to ports offload capability by default.
> > > > >
> > > >
> > > > Hi Ghalem,
> > > >
> > > > This patch enables announcement of the DEV_TX_OFFLOAD_MULTI_SEGS
> > > > capability, but still the application may not request this offload.
> > > >
> > > > As ENA PMD currently assumes all the mbufs may have multiple segments
> > > > (and we don't have fast-path for the other cases), I suggest
> > > > overwriting this flag in the ena_dev_configure(), similar to what
> > > > we're doing with the DEV_RX_OFFLOAD_RSS_HASH flag.
> > >
> > > To give some more context, our application currently checks if the
> > > driver supports multi-segments by checking its capabilities, and asks
> > > for the feature if it is advertised.
> > >
> > > When dealing with drivers that do not advertise this capability, our app
> > > linearizes the segmented mbufs before sending them to the driver.
> > >
> > > I think this is the proper way to use the API: if the driver supports to
> > > handle multisegmented mbufs, it should advertise the capability.
> > >
> > > Regards,
> > > Olivier
> > >
> >
> > Hi Olivier,
> >
> > I agree we should advertise it. However, after advertising this
> > option, the application should ask PMD to use this feature if I
> > understand the offload API correctly.
>
> Yes
>
> > I was thinking about the PMD behavior if this option won't be
> > requested by the application - it would be the same as for the
> > multisegment setup, that's why I suggested to also override this
> > feature at the configuration step.
> >
> > With the RSS hash feature we're doing exactly the same - first we're
> > advertising that it's available, and then (if RSS was enabled), we're
> > setting this offload as enabled, even though the application didn't
> > request it explicitly.
>
> I have no strong opinion here. If there is no behavior change when the
> option is set or unset by the application, I think both options are
> acceptable: either return what the application asked for, or return the
> option always set (as you do for hash).
>
> > Anyway, I won't be holding push of this patch as we're close to rc3.
> > @Ghalem, please add the fixline and Cc the dpdk@stable.org in the
> > commit log, so we will have this commit backported.
>
> Ghalem is currently off, I'll take care of it.
>
> The multiseg Tx feature is there since the beginning, so the Fixes
> line will be:
> Fixes: 1173fca25af9 ("ena: add polling-mode driver")
>

Ack

> I can also add the flag in ena_dev_configure(), if you feel it is more
> consistent. Please let me know.
>

Please add it, so it will be more consistent and verbose.

Thanks,
Michal