Re: [dpdk-stable] [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad <matan@mellanox.com>]

From: Ferruh Yigit 
> On 7/30/2019 4:56 PM, Matan Azrad wrote:
> > Hi Ferruh
> >
> >  From: Ferruh Yigit
> >> Sent: Tuesday, July 30, 2019 6:22 PM
> >> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
> >> configuration
> >>
> >> On 7/30/2019 2:17 PM, Matan Azrad wrote:
> >>> Hi Ferruh
> >>>
> >>> From: Ferruh Yigit
> >>>> Sent: Tuesday, July 30, 2019 4:09 PM
> >>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
> >>>> offload configuration
> >>>>
> >>>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
> >>>>> When the mbuf data size cannot contain the maximum Rx packet
> >>>>> length with the mbuf headroom, a packet should be scattered in
> >>>>> more than one
> >>>> mbuf.
> >>>>>
> >>>>> The application did not configure scatter offload in the above case.
> >>>>>
> >>>>> Enable the Rx scatter offload in the above case.
> >>>>>
> >>>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
> >>>>> supported
> >>>>> segments")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> >>>>
> >>>> Deferring the patchset to next release, they were late anyway and
> >>>> not actually fixing a defect, safer to defer than getting them in rc3.
> >>>
> >>> Yes this patch came late for RC3 but it is a fix.
> >>>
> >>> What are you concerns here?
> >>> Why don't you think defect found?
> >>
> >> First patch changes the behavior, when mbuf size is larger than
> >> configured size and user didn't provided the scatter offload, should
> >> test application automatically enable it?
> >
> > No, only when the mbuf size is smaller than the max_rx_pkt_len with
> headroom.
> > If scatter is not enabled in the above case, how can the PMD provide a
> packet with max_rx_pkt_len size?
> >

Answer here?

> > I think not enabling scatter in this case it is a user conflict in configuration
> and should raise an error in the PMD.  Maybe even in ethdev layer.
> >
> >> It may or not, but this is the change of the behavior, I think not a
> >> fix.
> >>
> >> And second patch adds more detail into the statistics, so I believe
> >> it is clear that it is not a fix.
> >
> >  Agree, this can wait.
> >
> >> The concern is getting changes very close to release, to balance
> >> between risk and benefit of the feature. I don't see any reason why
> >> these changes can't wait next release, so I don't see any reason to get the
> risk.
> >
> > When  I changed the default max_rx_pkt_len and mbuf size in LRO testing I
> met this issue.
> >
> > By default scatter will not be enabled.
> 
> I think it is still arguable if scatter should be enabled by default,

I meant that with this patch it will not be enabled by default due to the default values of mbuf size and max_rx_pkt_len.

> but isn't there a way in testpmd to enable scatter explicitly? If so you have a way to test LRO.

Yes there is a way.

This patch is just the right way to do it.