diff mbox series

net/iavf: fix performance drop

Message ID 1619414983-131070-1-git-send-email-wenzhuo.lu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series net/iavf: fix performance drop |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/github-robot success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Wenzhuo Lu April 26, 2021, 5:29 a.m. UTC 
  AVX2 and SSE don't have the offload path.
Not necessary doing any check. Or the scalar path
will be chosen.

Fixes: eff56a7b9f97 ("net/iavf: add offload path for Rx AVX512")

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 drivers/net/iavf/iavf_rxtx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

Qi Zhang April 26, 2021, 9:04 a.m. UTC | #1 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wenzhuo Lu
> Sent: Monday, April 26, 2021 1:30 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: [dpdk-dev] [PATCH] net/iavf: fix performance drop
> 
> AVX2 and SSE don't have the offload path.
> Not necessary doing any check. Or the scalar path will be chosen.
> 
> Fixes: eff56a7b9f97 ("net/iavf: add offload path for Rx AVX512")
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  drivers/net/iavf/iavf_rxtx.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c index
> 3f3cf63..0ba19dbf 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -2401,13 +2401,11 @@
>  	check_ret = iavf_rx_vec_dev_check(dev);
>  	if (check_ret >= 0 &&
>  	    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_128) {
> -		if (check_ret == IAVF_VECTOR_PATH) {
> -			use_sse = true;
> -			if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> -			     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1)
> &&
> -			    rte_vect_get_max_simd_bitwidth() >=
> RTE_VECT_SIMD_256)
> -				use_avx2 = true;
> -		}
> +		use_sse = true;
> +		if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> +		     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
> +		    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
> +			use_avx2 = true;


Not sure if the right path will be selected when avx512 is not true, (CC_AVX512_SUPPORT is disable) while check_ret is IAVF_VECTOR_OFFLOAD_PATH?

Currently we have 

if (!use_sse && !use_avx2 && !use_avx512)
	goto normal;

Should we also add below check?

if (!use_avx512 && check_ret == IAVF_VECTOR_OFFLOAD_PATH)
   goto normal;

> 
>  #ifdef CC_AVX512_SUPPORT
>  		if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1 &&
> --
> 1.9.3
Wenzhuo Lu April 27, 2021, 1:51 a.m. UTC | #2 
> > +use_sse = true;
> > +if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> > +     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
> > +    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
> > +use_avx2 = true;
> 
> 
> Not sure if the right path will be selected when avx512 is not true,
> (CC_AVX512_SUPPORT is disable) while check_ret is
> IAVF_VECTOR_OFFLOAD_PATH?
> 
> Currently we have
> 
> if (!use_sse && !use_avx2 && !use_avx512) goto normal;
> 
> Should we also add below check?
> 
> if (!use_avx512 && check_ret == IAVF_VECTOR_OFFLOAD_PATH)
>    goto normal;
Not necessary. As explained in the commit log, AVX2 and SSE support the offload features. The purpose of this patch is to let AVX2 be chosen when offload needed and AVX512 disable.
Qi Zhang April 27, 2021, 2:57 a.m. UTC | #3 
> -----Original Message-----
> From: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Sent: 2021年4月27日 9:51
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix performance drop
> 
> > > +use_sse = true;
> > > +if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> > > +     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
> > > +    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
> > > +use_avx2 = true;
> >
> >
> > Not sure if the right path will be selected when avx512 is not true,
> > (CC_AVX512_SUPPORT is disable) while check_ret is
> > IAVF_VECTOR_OFFLOAD_PATH?
> >
> > Currently we have
> >
> > if (!use_sse && !use_avx2 && !use_avx512) goto normal;
> >
> > Should we also add below check?
> >
> > if (!use_avx512 && check_ret == IAVF_VECTOR_OFFLOAD_PATH)
> >    goto normal;
> Not necessary. As explained in the commit log, AVX2 and SSE support the
> offload features. The purpose of this patch is to let AVX2 be chosen when
> offload needed and AVX512 disable.

OK, now I understand it, I will suggest to change the commit log.

From 
AVX2 and SSE don't have the offload path.

To 
AVX2 and SSE Tx path already support offloads but can't selected.

If you agree, will apply above during change during patch merge.
Wenzhuo Lu April 27, 2021, 5:09 a.m. UTC | #4 
> > > > +use_sse = true;
> > > > +if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> > > > +     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
> > > > +    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
> > > > +use_avx2 = true;
> > >
> > >
> > > Not sure if the right path will be selected when avx512 is not true,
> > > (CC_AVX512_SUPPORT is disable) while check_ret is
> > > IAVF_VECTOR_OFFLOAD_PATH?
> > >
> > > Currently we have
> > >
> > > if (!use_sse && !use_avx2 && !use_avx512) goto normal;
> > >
> > > Should we also add below check?
> > >
> > > if (!use_avx512 && check_ret == IAVF_VECTOR_OFFLOAD_PATH)
> > >    goto normal;
> > Not necessary. As explained in the commit log, AVX2 and SSE support
> > the offload features. The purpose of this patch is to let AVX2 be
> > chosen when offload needed and AVX512 disable.
> 
> OK, now I understand it, I will suggest to change the commit log.
> 
> From
> AVX2 and SSE don't have the offload path.
> 
> To
> AVX2 and SSE Tx path already support offloads but can't selected.
> 
> If you agree, will apply above during change during patch merge.
Sure. Thanks for that.
Qi Zhang April 27, 2021, 5:19 a.m. UTC | #5 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wenzhuo Lu
> Sent: Monday, April 26, 2021 1:30 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: [dpdk-dev] [PATCH] net/iavf: fix performance drop
> 
> AVX2 and SSE don't have the offload path.
> Not necessary doing any check. Or the scalar path will be chosen.

AVX2 and SSE Tx path already support offloads but can't be selected.
The patch remove the unnecessary check or the scalar path will always
be chosen.

> 
> Fixes: eff56a7b9f97 ("net/iavf: add offload path for Rx AVX512")
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel after refined commit log.

Thanks
Qi
Ferruh Yigit April 28, 2021, 11:32 a.m. UTC | #6 
On 4/26/2021 6:29 AM, Wenzhuo Lu wrote:
> AVX2 and SSE don't have the offload path.
> Not necessary doing any check. Or the scalar path
> will be chosen.

Hi Wenzhuo,

The fix by not changing Rx implementation, but making sure correct Rx path
selected, right? Can you please clarify this in the commit log?

So the performance drop fixed for whoever have the vector path supported and
offloads enabled, can be good to highlight in the patch title, otherwise it is
too generic.

> 
> Fixes: eff56a7b9f97 ("net/iavf: add offload path for Rx AVX512")
> 

I am not clear what caused the performance drop.
Before above patch, vector path was not supporting the offloads and scalar path
should be selected. After above patch, still scalar path selected, although
vector path supports offloads, but for both before and after scalar path is
selected, so why/when the performance drop happens?
Can you please clarify in the commit log, how to reproduce performance drop?


> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  drivers/net/iavf/iavf_rxtx.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> index 3f3cf63..0ba19dbf 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -2401,13 +2401,11 @@
>  	check_ret = iavf_rx_vec_dev_check(dev);
>  	if (check_ret >= 0 &&
>  	    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_128) {
> -		if (check_ret == IAVF_VECTOR_PATH) {
> -			use_sse = true;
> -			if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> -			     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
> -			    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
> -				use_avx2 = true;
> -		}
> +		use_sse = true;
> +		if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> +		     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
> +		    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
> +			use_avx2 = true;
>  
>  #ifdef CC_AVX512_SUPPORT
>  		if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1 &&
>
Ferruh Yigit April 28, 2021, 11:32 a.m. UTC | #7 
On 4/27/2021 3:57 AM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Sent: 2021年4月27日 9:51
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix performance drop
>>
>>>> +use_sse = true;
>>>> +if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
>>>> +     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
>>>> +    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
>>>> +use_avx2 = true;
>>>
>>>
>>> Not sure if the right path will be selected when avx512 is not true,
>>> (CC_AVX512_SUPPORT is disable) while check_ret is
>>> IAVF_VECTOR_OFFLOAD_PATH?
>>>
>>> Currently we have
>>>
>>> if (!use_sse && !use_avx2 && !use_avx512) goto normal;
>>>
>>> Should we also add below check?
>>>
>>> if (!use_avx512 && check_ret == IAVF_VECTOR_OFFLOAD_PATH)
>>>    goto normal;
>> Not necessary. As explained in the commit log, AVX2 and SSE support the
>> offload features. The purpose of this patch is to let AVX2 be chosen when
>> offload needed and AVX512 disable.
> 
> OK, now I understand it, I will suggest to change the commit log.
> 
> From 
> AVX2 and SSE don't have the offload path.
> 
> To 
> AVX2 and SSE Tx path already support offloads but can't selected.
> 

Isn't this related to the _Rx_ path?

> If you agree, will apply above during change during patch merge.
>
Qi Zhang April 28, 2021, 12:06 p.m. UTC | #8 
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Wednesday, April 28, 2021 7:32 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/iavf: fix performance drop
> 
> On 4/27/2021 3:57 AM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> >> Sent: 2021年4月27日 9:51
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> >> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix performance drop
> >>
> >>>> +use_sse = true;
> >>>> +if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
> >>>> +     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
> >>>> +    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
> >>>> +use_avx2 = true;
> >>>
> >>>
> >>> Not sure if the right path will be selected when avx512 is not true,
> >>> (CC_AVX512_SUPPORT is disable) while check_ret is
> >>> IAVF_VECTOR_OFFLOAD_PATH?
> >>>
> >>> Currently we have
> >>>
> >>> if (!use_sse && !use_avx2 && !use_avx512) goto normal;
> >>>
> >>> Should we also add below check?
> >>>
> >>> if (!use_avx512 && check_ret == IAVF_VECTOR_OFFLOAD_PATH)
> >>>    goto normal;
> >> Not necessary. As explained in the commit log, AVX2 and SSE support
> >> the offload features. The purpose of this patch is to let AVX2 be
> >> chosen when offload needed and AVX512 disable.
> >
> > OK, now I understand it, I will suggest to change the commit log.
> >
> > From
> > AVX2 and SSE don't have the offload path.
> >
> > To
> > AVX2 and SSE Tx path already support offloads but can't selected.
> >
> 
> Isn't this related to the _Rx_ path?

Yes, it should be Rx Path.

> 
> > If you agree, will apply above during change during patch merge.
> >
Wenzhuo Lu April 29, 2021, 1:08 a.m. UTC | #9 
> The fix by not changing Rx implementation, but making sure correct Rx path
> selected, right? Can you please clarify this in the commit log?
> 
> So the performance drop fixed for whoever have the vector path supported and
> offloads enabled, can be good to highlight in the patch title, otherwise it is too
> generic.
Yes, the implementation is not changed. It's only about the path selection. I'll clarify it.

> 
> >
> > Fixes: eff56a7b9f97 ("net/iavf: add offload path for Rx AVX512")
> >
> 
> I am not clear what caused the performance drop.
> Before above patch, vector path was not supporting the offloads and scalar
> path should be selected. After above patch, still scalar path selected, although
> vector path supports offloads, but for both before and after scalar path is
> selected, so why/when the performance drop happens?
> Can you please clarify in the commit log, how to reproduce performance drop?
Sure. I'll describe more about what's the issue.
diff mbox series

Patch

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index 3f3cf63..0ba19dbf 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -2401,13 +2401,11 @@ 
 	check_ret = iavf_rx_vec_dev_check(dev);
 	if (check_ret >= 0 &&
 	    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_128) {
-		if (check_ret == IAVF_VECTOR_PATH) {
-			use_sse = true;
-			if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
-			     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
-			    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
-				use_avx2 = true;
-		}
+		use_sse = true;
+		if ((rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) == 1 ||
+		     rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1) &&
+		    rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_256)
+			use_avx2 = true;
 
 #ifdef CC_AVX512_SUPPORT
 		if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) == 1 &&