Hi Ferruh,

-----Original Message-----
From: Ferruh Yigit <redacted>
Sent: Tuesday, April 13, 2021 8:37 PM
To: Lu, Wenzhuo <redacted>; dev@dpdk.org; Richardson,
Bruce [off-list ref]
Cc: stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v4 1/3] net/iavf: fix segment fault in AVX512

On 4/9/2021 4:01 AM, Wenzhuo Lu wrote:

quoted

Fix segment fault when failing to get the memory from the pool.

Can be good to clarify there is no change in the moved code, it is not possible
to recognize this from patch without using a compare app.

Sure. Will add more description here.

quoted

Fixes: 31737f2b66fb ("net/iavf: enable AVX512 for legacy Rx")
Cc: stable@dpdk.org

Reported-by: David Coyle <redacted>
Signed-off-by: Wenzhuo Lu <redacted>
---
  drivers/net/iavf/iavf_rxtx_vec_avx2.c   | 120 +------------------
  drivers/net/iavf/iavf_rxtx_vec_avx512.c |   5 +-
  drivers/net/iavf/iavf_rxtx_vec_common.h | 203
++++++++++++++++++++++++++++++++

The common vector code seems moved to 'iavf_rxtx_vec_common.h' but
that header is included by 'iavf_rxtx_vec_sse.c' too, and the moved function
has AVX2 code in it.
Won't this fail to build if the AVX2 is not enabled?

Agree. There may be a compile error. I'll change the ' RTE_ARCH_X86 ' to ' CC_AVX2_SUPPORT ' to make the code only for avx2 and avx512.

Bruce, is there an easy way to test this, meson detects the AVX2 support
even I provide c_args march that doesn't have AVX2 support.

<...>

quoted

index 46a1873..57b4381 100644

--- a/drivers/net/iavf/iavf_rxtx_vec_common.h
+++ b/drivers/net/iavf/iavf_rxtx_vec_common.h

@@ -11,6 +11,10 @@
  #include "iavf.h"
  #include "iavf_rxtx.h"

+#ifndef __INTEL_COMPILER
+#pragma GCC diagnostic ignored "-Wcast-qual"
+#endif
+

Is this pragma needed or carried here to be sure?

It's necessary for a compile warning. Just leveraged from the existing code.

quoted

  static inline uint16_t
  reassemble_packets(struct iavf_rx_queue *rxq, struct rte_mbuf **rx_bufs,
  		   uint16_t nb_bufs, uint8_t *split_flags) @@ -276,4 +280,203

@@

quoted

  	return 0;
  }

+#ifdef RTE_ARCH_X86
+static __rte_always_inline void
+iavf_rxq_rearm_cmn(struct iavf_rx_queue *rxq, __rte_unused bool
+avx512)

What do you think expand 'cmn' to full 'common', it is clear from this patch
what it stands for but later if you just look this function it is not that clear if it
is an abbreviation for something else or common.

Sure. Will change it to 'common'.