RE: 22.11.3 patches review and test

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: 9/5/2023 16:50
> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>; Xu, HailinX
> <hailinx.xu@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Xu, Ke1 <ke1.xu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: xuemingl@nvdia.com; dev@dpdk.org; Stokes, Ian <ian.stokes@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>; Luca Boccassi
> <bluca@debian.org>; Xu, Qian Q <qian.q.xu@intel.com>; NBU-Contact-
> Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Peng, Yuan
> <yuan.peng@intel.com>; Chen, Zhaoyan <zhaoyan.chen@intel.com>; Yang,
> Qiming <qiming.yang@intel.com>; Zhou, YidingX <yidingx.zhou@intel.com>;
> Cui, KaixinX <kaixinx.cui@intel.com>
> Subject: Re: 22.11.3 patches review and test
> 
> On 05/09/2023 02:51, Zeng, ZhichaoX wrote:
> >
> >
> > Regards
> > Zhichao
> >> -----Original Message-----
> >> From: Kevin Traynor <ktraynor@redhat.com>
> >> Sent: Monday, September 4, 2023 10:15 PM
> >> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>; Xu, HailinX
> >> <hailinx.xu@intel.com>; Xueming Li <xuemingl@nvidia.com>;
> >> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> >> <beilei.xing@intel.com>; Xu, Ke1 <ke1.xu@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>
> >> Cc: xuemingl@nvdia.com; dev@dpdk.org; Stokes, Ian
> >> <ian.stokes@intel.com>; Mcnamara, John <john.mcnamara@intel.com>;
> >> Luca Boccassi <bluca@debian.org>; Xu, Qian Q <qian.q.xu@intel.com>;
> >> Thomas Monjalon <thomas@monjalon.net>; Peng, Yuan
> >> <yuan.peng@intel.com>; Chen, Zhaoyan <zhaoyan.chen@intel.com>
> >> Subject: Re: 22.11.3 patches review and test
> >>
> >> On 04/09/2023 10:32, Kevin Traynor wrote:
> >>> On 01/09/2023 04:23, Zeng, ZhichaoX wrote:
> >>>>> -----Original Message-----
> >>>>> From: Kevin Traynor <ktraynor@redhat.com>
> >>>>> Sent: Thursday, August 31, 2023 8:18 PM
> >>>>> To: Xu, HailinX <hailinx.xu@intel.com>; Xueming Li
> >>>>> <xuemingl@nvidia.com>; stable@dpdk.org; Wu, Jingjing
> >>>>> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Xu,
> >>>>> Ke1 <ke1.xu@intel.com>; Zeng, ZhichaoX <zhichaox.zeng@intel.com>;
> >>>>> Zhang, Qi Z <qi.z.zhang@intel.com>
> >>>>> Cc: xuemingl@nvdia.com; dev@dpdk.org; Stokes, Ian
> >>>>> <ian.stokes@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>;
> >>>>> Luca Boccassi <bluca@debian.org>; Xu, Qian Q
> >>>>> <qian.q.xu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> >>>>> Peng, Yuan <yuan.peng@intel.com>; Chen, Zhaoyan
> >>>>> <zhaoyan.chen@intel.com>
> >>>>> Subject: Re: 22.11.3 patches review and test
> >>>>>
> >>>>> On 30/08/2023 17:25, Kevin Traynor wrote:
> >>>>>> On 29/08/2023 09:22, Xu, HailinX wrote:
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Xueming Li <xuemingl@nvidia.com>
> >>>>>>>> Sent: Thursday, August 17, 2023 2:14 PM
> >>>>>>>> To: stable@dpdk.org
> >>>>>>>> Cc: xuemingl@nvdia.com; dev@dpdk.org; Abhishek Marathe
> >>>>>>>> <Abhishek.Marathe@microsoft.com>; Ali Alnubani
> >>>>>>>> <alialnu@nvidia.com>; Walker, Benjamin
> >>>>>>>> <benjamin.walker@intel.com>; David Christensen
> >>>>>>>> <drc@linux.vnet.ibm.com>; Hemant Agrawal
> >>>>> <hemant.agrawal@nxp.com>;
> >>>>>>>> Stokes, Ian <ian.stokes@intel.com>; Jerin Jacob
> >>>>>>>> <jerinj@marvell.com>; Mcnamara, John
> >> <john.mcnamara@intel.com>;
> >>>>>>>> Ju-Hyoung Lee <juhlee@microsoft.com>; Kevin Traynor
> >>>>>>>> <ktraynor@redhat.com>; Luca Boccassi <bluca@debian.org>; Pei
> >>>>>>>> Zhang <pezhang@redhat.com>; Xu, Qian Q <qian.q.xu@intel.com>;
> >>>>>>>> Raslan Darawsheh <rasland@nvidia.com>; Thomas Monjalon
> >>>>>>>> <thomas@monjalon.net>; Yanghang Liu <yanghliu@redhat.com>;
> >> Peng,
> >>>>>>>> Yuan <yuan.peng@intel.com>; Chen, Zhaoyan
> >>>>> <zhaoyan.chen@intel.com>
> >>>>>>>> Subject: 22.11.3 patches review and test
> >>>>>>>>
> >>>>>>>> Hi all,
> >>>>>>>>
> >>>>>>>> Here is a list of patches targeted for stable release 22.11.3.
> >>>>>>>>
> >>>>>>>> The planned date for the final release is 31th August.
> >>>>>>>>
> >>>>>>>> Please help with testing and validation of your use cases and
> >>>>>>>> report any issues/results with reply-all to this mail. For the
> >>>>>>>> final release the fixes and reported validations will be added
> >>>>>>>> to the release
> >>>>> notes.
> >>>>>>>>
> >>>>>>>> A release candidate tarball can be found at:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> https://dpdk.org/browse/dpdk-stable/tag/?id=v22.11.3-rc1
> >>>>>>>>
> >>>>>>>> These patches are located at branch 22.11 of dpdk-stable repo:
> >>>>>>>>          https://dpdk.org/browse/dpdk-stable/
> >>>>>>>>
> >>>>>>>> Thanks.
> >>>>>>>
> >>>>>>> We are conducting DPDK testing and have found two issues.
> >>>>>>>
> >>>>>>> 1. The startup speed of testpmd is significantly slower in the os of
> SUSE
> >>>>>>>        This issue fix patch has been merged into main, But it
> >>>>>>> has not backported
> >>>>> to 22.11.3.
> >>>>>>>        Fix patch commit id on DPDK main:
> >>>>>>> 7e7b6762eac292e78c77ad37ec0973c0c944b845
> >>>>>>>
> >>>>>>> 2. The SCTP tunnel packet of iavf cannot be forwarded in avx512
> >>>>>>> mode
> >>>>>
> >>>>> Need to clarify this sentence. It looks like it is not a
> >>>>> functional bug where
> >>>>> avx512 mode is selected and then an SCTP tunnel packet cannot be
> sent.
> >>>>> Instead, it is a possible performance issue that avx512 mode will
> >>>>> not be selected where it might have been due to unneeded additions
> >>>>> (RTE_ETH_TX_OFFLOAD_*_TNL_TSO) to IAVF_TX_NO_VECTOR_FLAGS.
> >>>>>
> >>>>> @IAVF maintainers - please confirm my analysis is correct ?
> >>>>>
> >>>>> In that case, as it is a possible performance issue in a specific
> >>>>> case for a single driver I think it is non-critical for 21.11 and
> >>>>> we can just revert the patch on the branch and wait for 21.11.6
> >>>>> release in
> >> December.
> >>>>
> >>>> Hi Kevin,
> >>>>
> >>>> Since the LTS version of the IAVF driver does not support avx512
> >>>> checksum offload, the scalar path should be selected, but this
> >>>> patch makes it incorrectly select the
> >>>> avx512 path, and the SCTP tunnel packets can't be forwarded properly.
> >>>>
> >>>
> >>> ok, let's take a look at the patch and usage.
> >>>
> >>> diff --git a/drivers/net/iavf/iavf_rxtx.h
> >>> b/drivers/net/iavf/iavf_rxtx.h index 8d4a77271a..605ea3f824 100644
> >>> --- a/drivers/net/iavf/iavf_rxtx.h
> >>> +++ b/drivers/net/iavf/iavf_rxtx.h
> >>> @@ -32,4 +32,8 @@
> >>>                    RTE_ETH_TX_OFFLOAD_MULTI_SEGS |          \
> >>>                    RTE_ETH_TX_OFFLOAD_TCP_TSO |             \
> >>> +               RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |       \
> >>> +               RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |         \
> >>> +               RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |        \
> >>> +               RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |      \
> >>>                    RTE_ETH_TX_OFFLOAD_SECURITY)
> >>>
> >>> <snip>
> >>>
> >>> So we now have:
> >>> #define IAVF_TX_NO_VECTOR_FLAGS (				 \
> >>> 		RTE_ETH_TX_OFFLOAD_VLAN_INSERT |		 \
> >>> 		RTE_ETH_TX_OFFLOAD_QINQ_INSERT |		 \
> >>> 		RTE_ETH_TX_OFFLOAD_MULTI_SEGS |		 \
> >>> 		RTE_ETH_TX_OFFLOAD_TCP_TSO |		 \
> >>> 		RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |	 \
> >>> 		RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |	 \
> >>> 		RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |	 \
> >>> 		RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |	 \
> >>> 		RTE_ETH_TX_OFFLOAD_SECURITY)
> >>>
> >>> <snip>
> >>>
> >
> > Hi Kevin,
> >
> > This patch also removes two flags from IAVF_TX_NO_VECTOR_FLAGS,
> > RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM
> > and RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM.
> >
> >>> static inline int
> >>> iavf_tx_vec_queue_default(struct iavf_tx_queue *txq) {
> >>> 	if (!txq)
> >>> 		return -1;
> >>>
> >>> 	if (txq->rs_thresh < IAVF_VPMD_TX_MAX_BURST ||
> >>> 	    txq->rs_thresh > IAVF_VPMD_TX_MAX_FREE_BUF)
> >>> 		return -1;
> >>>
> >>> 	if (txq->offloads & IAVF_TX_NO_VECTOR_FLAGS)
> >>> 		return -1;
> >>>                ^^^ Adding the extra bits to IAVF_TX_NO_VECTOR_FLAGS
> >>> gives
> >>> *more* reasons for why this statement might not be true, so
> >>> returning
> >>> -1 indicating that vector cannot be used for tx queue
> >>>
> >>
> >> typo here - just to clarify, the new flags give more reasons for this
> >> statement to be true, so returning -1.
> >>
> >>>
> >>> <snip>
> >>>
> >>> static inline bool
> >>> check_tx_vec_allow(struct iavf_tx_queue *txq) {
> >>> 	if (!(txq->offloads & IAVF_TX_NO_VECTOR_FLAGS) &&
> >>>
> >>>                ^^^ Adding the extra bits to IAVF_TX_NO_VECTOR_FLAGS
> >>> gives
> >>> *more* reason for this statement will be false and then false
> >>> returned indicating that vector cannot be used.
> >>>
> >>> 	    txq->rs_thresh >= IAVF_VPMD_TX_MAX_BURST &&
> >>> 	    txq->rs_thresh <= IAVF_VPMD_TX_MAX_FREE_BUF) {
> >>> 		PMD_INIT_LOG(DEBUG, "Vector tx can be enabled on this
> >> txq.");
> >>> 		return true;
> >>> 	}
> >>> 	PMD_INIT_LOG(DEBUG, "Vector Tx cannot be enabled on this txq.");
> >>> 	return false;
> >>> }
> >>>
> >>> --
> >>>
> >>> It looks like that adding the extra bits gives it less reasons to
> >>> select vector mode. However, you are saying that this patch means
> >>> there is a case where it now selects vector where it should not,
> >>> meaning additional reason to select vector mode. So maybe I miss
> >> something ?
> >
> > Originally, the vector path would not be selected after configuring
> > outer checksum offload, but it will be selected after removing the two flags.
> > But the vector path doesn't support outer checksum offload on stable DPDK,
> so there will be a problem.
> >
> > The key issue is that these two flags are removed,
> > RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM
> > and RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM.
> >
> 
> ok, i got it now. In the main branch (ca34627be5d6) and the 21.11 backport
> (3eb4ad8ed694) those flags are not removed. Those flags are only removed in
> the 22.11 branch (9b7215f150d0).
> 
> So this is not an issue for 21.11. Thanks for helping clear it up.

Hi Guys, thanks for the clarification, the patch has been reverted in 22.11 candidate branch.

Thanks,
Xueming

> 
> Kevin.
> 
> > Regards
> > Zhichao
> >
> >>>
> >>>> Yes, we can revert this commit for 21.11.6 release, thanks.
> >>>>
> >>>> Regards
> >>>> Zhichao
> >>>>
> >>>>> thanks,
> >>>>> Kevin.
> >>>>>
> >>>>>>>        commit 9b7215f150d0bfc5cb00fce68ff08e5217c7f2b3 on
> >> v22.11.3-
> >>>>> rc1.
> >>>>>>>        This commit is for the new feature (avx512 checksum
> >>>>>>> offload) in DPDK
> >>>>> 23.03, which should not be backported to the LTS version since
> >>>>> avx512 checksum offload is not supported in v22.11.3 LTS.
> >>>>>>>
> >>>>>>
> >>>>>> Thanks for flagging Xueming.
> >>>>>>
> >>>>>> The issue is that it was listed as fixing 059f18ae2aec ("net/iavf:
> >>>>>> add offload path for Tx AVX512") which goes back to 21.05.
> >>>>>>
> >>>>>> This could have been avoided if the 'Fixes:' tag was correct, or
> >>>>>> if the authors replied to the email about queued backports :/
> >>>>>>
> >>>>>> Requesting iavf/next-net-intel maintainers to check Fixes: tags
> >>>>>> are correct before merging.
> >>>>>>
> >>>>>> DPDK 21.11.5 is already released with this patch. Any idea why it
> >>>>>> did not show up in validation for 21.11.5 ? Is it an issue for 21.11.5 ?
> >>>>>> How critical is it ?
> >>>>>>
> >>>>>> I can revert it on the 21.11 branch, but it will need to wait
> >>>>>> until
> >>>>>> 21.11.6 in December before it will be reverted in a released version.
> >>>>>>
> >>>>>> thanks,
> >>>>>> Kevin.
> >>>>>>
> >>>>>>> Regards,
> >>>>>>> Xu, Hailin
> >>>>>>>
> >>>>>>
> >>>>
> >>>
> >