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 <ma...@mellanox.com>; Wenzhuo Lu > >> <wenzhuo...@intel.com>; Jingjing Wu <jingjing...@intel.com> > >> Cc: dev@dpdk.org; sta...@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 <ma...@mellanox.com>; Wenzhuo Lu > >>>> <wenzhuo...@intel.com>; Jingjing Wu <jingjing...@intel.com> > >>>> Cc: dev@dpdk.org; sta...@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: sta...@dpdk.org > >>>>> > >>>>> Signed-off-by: Matan Azrad <ma...@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.