Re: [dpdk-dev] [PATCH] net/ice: fix wrong data path selection in secondary process
Hi, Qi
I've tested this patch and it works.
Best Regards,
Yixue.
> -Original Message-
> From: Zhang, Qi Z
> Sent: Monday, May 24, 2021 17:08
> To: Yang, Qiming
> Cc: Zhang, Liheng ; Wang, Yixue
> ; Dong, Yao ; dev@dpdk.org;
> Zhang, Qi Z ; sta...@dpdk.org
> Subject: [PATCH] net/ice: fix wrong data path selection in secondary process
>
> The flag use_avx2 and use_avx512 are defined as local variables, they will
> not be
> aware by the secondary process, then wrong data path is selected. Fix the
> issue
> by moving them into struct ice_adapter.
>
> Fixes: ae60d3c9b227 ("net/ice: support Rx AVX2 vector")
> Fixes: 2d5f6953d56d ("net/ice: support vector AVX2 in Tx")
> Fixes: 7f85d5ebcfe1 ("net/ice: add AVX512 vector path")
> Cc: sta...@dpdk.org
>
> Reported-by: Yixue Wang
> Signed-off-by: Qi Zhang
> Tested-by: Yixue Wang
> ---
> drivers/net/ice/ice_ethdev.h | 6 +
> drivers/net/ice/ice_rxtx.c | 44 ++--
> 2 files changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h index
> 2a8a8169d5..aebfd1b0b7 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -487,6 +487,12 @@ struct ice_adapter {
> struct ice_devargs devargs;
> enum ice_pkg_type active_pkg_type; /* loaded ddp package type */
> uint16_t fdir_ref_cnt;
> +#ifdef RTE_ARCH_X86
> + bool rx_use_avx2;
> + bool rx_use_avx512;
> + bool tx_use_avx2;
> + bool tx_use_avx512;
> +#endif
> };
>
> struct ice_vsi_vlan_pvid_info {
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index
> 49abcb2f5c..f4f6f48d78 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -3058,11 +3058,11 @@ ice_set_rx_function(struct rte_eth_dev *dev)
> #ifdef RTE_ARCH_X86
> struct ice_rx_queue *rxq;
> int i;
> - int rx_check_ret;
> - bool use_avx512 = false;
> - bool use_avx2 = false;
> + int rx_check_ret = 0;
>
> if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> + ad->rx_use_avx512 = false;
> + ad->rx_use_avx2 = false;
> rx_check_ret = ice_rx_vec_dev_check(dev);
> if (rx_check_ret >= 0 && ad->rx_bulk_alloc_allowed &&
> rte_vect_get_max_simd_bitwidth() >= RTE_VECT_SIMD_128)
> { @@ -3079,16 +3079,16 @@ ice_set_rx_function(struct rte_eth_dev *dev)
> rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) ==
> 1 &&
> rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512BW)
> == 1) #ifdef CC_AVX512_SUPPORT
> - use_avx512 = true;
> + ad->rx_use_avx512 = true;
> #else
> PMD_DRV_LOG(NOTICE,
> "AVX512 is not supported in build env"); #endif
> - if (!use_avx512 &&
> + if (!ad->rx_use_avx512 &&
> (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;
> + ad->rx_use_avx2 = true;
>
> } else {
> ad->rx_vec_allowed = false;
> @@ -3097,7 +3097,7 @@ ice_set_rx_function(struct rte_eth_dev *dev)
>
> if (ad->rx_vec_allowed) {
> if (dev->data->scattered_rx) {
> - if (use_avx512) {
> + if (ad->rx_use_avx512) {
> #ifdef CC_AVX512_SUPPORT
> if (rx_check_ret ==
> ICE_VECTOR_OFFLOAD_PATH) {
> PMD_DRV_LOG(NOTICE,
> @@ -3116,14 +3116,14 @@ ice_set_rx_function(struct rte_eth_dev *dev)
> } else {
> PMD_DRV_LOG(DEBUG,
> "Using %sVector Scattered Rx
> (port %d).",
> - use_avx2 ? "avx2 " : "",
> + ad->rx_use_avx2 ? "avx2 " : "",
> dev->data->port_id);
> - dev->rx_pkt_burst = use_avx2 ?
> + dev->rx_pkt_burst = ad->rx_use_avx2 ?
> ice_recv_scattered_pkts_vec_avx2 :
>
Re: [dpdk-dev] [PATCH v2] net/ice: fix data path corrupt on secondary process
Hi Qi,
Patch v2 has been tested.
Best Regards,
Yixue.
> -Original Message-
> From: Zhang, Qi Z
> Sent: Wednesday, May 26, 2021 14:13
> To: Yang, Qiming
> Cc: Zhang, Liheng ; Wang, Yixue
> ; Dong, Yao ; dev@dpdk.org;
> Zhang, Qi Z ; sta...@dpdk.org
> Subject: [PATCH v2] net/ice: fix data path corrupt on secondary process
>
> The rte_eth_devices array is not in share memory, it should not be referenced
> by
> ice_adapter which is shared by primary and secondary.
> Any process set ice_adapter->eth_dev will corrupt another process'
> context.
>
> The patch removed the field "eth_dev" from ice_adapter.
> Now, when the data paths try to access the rte_eth_dev_data instance, they
> should replace adapter->eth_dev->data with adapter->pf.dev_data.
>
> Fixes: f9cf4f864150 ("net/ice: support device initialization")
> Cc: sta...@dpdk.org
>
> Reported-by: Yixue Wang
> Signed-off-by: Qi Zhang
> Tested-by: Yixue Wang
> ---
>
> v2:
> - fix some wrong spell.
>
> drivers/net/ice/ice_dcf_parent.c | 1 -
> drivers/net/ice/ice_ethdev.c | 13 ++---
> drivers/net/ice/ice_ethdev.h | 3 ---
> drivers/net/ice/ice_fdir_filter.c | 6 +++---
> drivers/net/ice/ice_rxtx.c| 24 +---
> drivers/net/ice/ice_rxtx.h| 4 ++--
> drivers/net/ice/ice_rxtx_vec_avx2.c | 2 +-
> drivers/net/ice/ice_rxtx_vec_avx512.c | 2 +-
> drivers/net/ice/ice_rxtx_vec_common.h | 2 +-
> drivers/net/ice/ice_rxtx_vec_sse.c| 2 +-
> drivers/net/ice/ice_switch_filter.c | 6 +++---
> 11 files changed, 27 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/ice/ice_dcf_parent.c
> b/drivers/net/ice/ice_dcf_parent.c
> index 1d7aa8bc87..19420a0f58 100644
> --- a/drivers/net/ice/ice_dcf_parent.c
> +++ b/drivers/net/ice/ice_dcf_parent.c
> @@ -408,7 +408,6 @@ ice_dcf_init_parent_adapter(struct rte_eth_dev
> *eth_dev)
> const struct rte_ether_addr *mac;
> int err;
>
> - parent_adapter->eth_dev = eth_dev;
> parent_adapter->pf.adapter = parent_adapter;
> parent_adapter->pf.dev_data = eth_dev->data;
> /* create a dummy main_vsi */
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c index
> 5fd5f99b6f..6398e310aa 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -2055,7 +2055,6 @@ ice_dev_init(struct rte_eth_dev *dev)
> intr_handle = &pci_dev->intr_handle;
>
> pf->adapter = ICE_DEV_PRIVATE_TO_ADAPTER(dev->data-
> >dev_private);
> - pf->adapter->eth_dev = dev;
> pf->dev_data = dev->data;
> hw->back = pf->adapter;
> hw->hw_addr = (uint8_t *)pci_dev->mem_resource[0].addr; @@ -
> 2218,7 +2217,7 @@ ice_release_vsi(struct ice_vsi *vsi) void
> ice_vsi_disable_queues_intr(struct ice_vsi *vsi) {
> - struct rte_eth_dev *dev = vsi->adapter->eth_dev;
> + struct rte_eth_dev *dev =
> +&rte_eth_devices[vsi->adapter->pf.dev_data->port_id];
> struct rte_pci_device *pci_dev = ICE_DEV_TO_PCI(dev);
> struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
> struct ice_hw *hw = ICE_VSI_TO_HW(vsi); @@ -2994,7 +2993,7 @@
> static int ice_init_rss(struct ice_pf *pf) {
> struct ice_hw *hw = ICE_PF_TO_HW(pf);
> struct ice_vsi *vsi = pf->main_vsi;
> - struct rte_eth_dev *dev = pf->adapter->eth_dev;
> + struct rte_eth_dev_data *dev_data = pf->dev_data;
> struct ice_aq_get_set_rss_lut_params lut_params;
> struct rte_eth_rss_conf *rss_conf;
> struct ice_aqc_get_set_rss_keys key;
> @@ -3003,8 +3002,8 @@ static int ice_init_rss(struct ice_pf *pf)
> bool is_safe_mode = pf->adapter->is_safe_mode;
> uint32_t reg;
>
> - rss_conf = &dev->data->dev_conf.rx_adv_conf.rss_conf;
> - nb_q = dev->data->nb_rx_queues;
> + rss_conf = &dev_data->dev_conf.rx_adv_conf.rss_conf;
> + nb_q = dev_data->nb_rx_queues;
> vsi->rss_key_size = ICE_AQC_GET_SET_RSS_KEY_DATA_RSS_KEY_SIZE;
> vsi->rss_lut_size = pf->hash_lut_size;
>
> @@ -3138,7 +3137,7 @@ __vsi_queues_bind_intr(struct ice_vsi *vsi, uint16_t
> msix_vect, void ice_vsi_queues_bind_intr(struct ice_vsi *vsi) {
> - struct rte_eth_dev *dev = vsi->adapter->eth_dev;
> + struct rte_eth_dev *dev =
> +&rte_eth_devices[vsi->adapter->pf.dev_data->port_id];
> struct rte_pci_device *pci_dev = ICE_DEV_TO_PCI(dev);
> struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
> struct ice_hw *hw
