Re: 22.11.3 patches review and test

[Kevin Traynor <ktraynor@redhat.com>]

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>

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


<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 ?

> 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
>>>>
>>>
>