[v4] net/igc: add support for secondary processes

Message ID 20220630110330.211655-1-zhichaox.zeng@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Qi Zhang
Headers
Series [v4] net/igc: add support for secondary processes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Zeng, ZhichaoX June 30, 2022, 11:03 a.m. UTC
From: Zhichao Zeng <zhichaox.zeng@intel.com>

The RX function was not specified in the secondary process, causing the
secondary process to segfault in a multi-process environment.

This patch specify RX/TX functions in "dev_init" to support secondary
processes.

Fixes: 66fde1b943eb ("net/igc: add skeleton")
Cc: alvinx.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

---
v2:
remove unnecessary parameters, move declaration to relevant header file
---
v3:
remove redundant code, optimize commit log
---
v4:
rework patch
---
 drivers/net/igc/igc_ethdev.c | 9 ++++++++-
 drivers/net/igc/igc_txrx.c   | 8 ++++----
 drivers/net/igc/igc_txrx.h   | 6 ++++++
 3 files changed, 18 insertions(+), 5 deletions(-)
  

Comments

Qi Zhang June 30, 2022, 11:47 a.m. UTC | #1
> -----Original Message-----
> From: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Sent: Thursday, June 30, 2022 7:04 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zeng, ZhichaoX <zhichaox.zeng@intel.com>;
> alvinx.zhang@intel.com; Guo, Junfeng <junfeng.guo@intel.com>; Su, Simei
> <simei.su@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>; Ferruh
> Yigit <ferruh.yigit@intel.com>
> Subject: [PATCH v4] net/igc: add support for secondary processes
> 
> From: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> The RX function was not specified in the secondary process, causing the
> secondary process to segfault in a multi-process environment.
> 
> This patch specify RX/TX functions in "dev_init" to support secondary processes.
> 
> Fixes: 66fde1b943eb ("net/igc: add skeleton")
> Cc: alvinx.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> ---
> v2:
> remove unnecessary parameters, move declaration to relevant header file
> ---
> v3:
> remove redundant code, optimize commit log
> ---
> v4:
> rework patch
> ---
>  drivers/net/igc/igc_ethdev.c | 9 ++++++++-
>  drivers/net/igc/igc_txrx.c   | 8 ++++----
>  drivers/net/igc/igc_txrx.h   | 6 ++++++
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/igc/igc_ethdev.c b/drivers/net/igc/igc_ethdev.c index
> b9933b395d..7f221a5d34 100644
> --- a/drivers/net/igc/igc_ethdev.c
> +++ b/drivers/net/igc/igc_ethdev.c
> @@ -1240,8 +1240,15 @@ eth_igc_dev_init(struct rte_eth_dev *dev)
>  	 * has already done this work. Only check we don't need a different
>  	 * RX function.
>  	 */
> -	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +		dev->rx_pkt_burst = igc_recv_pkts;
> +		if (dev->data->scattered_rx)
> +			dev->rx_pkt_burst = igc_recv_scattered_pkts;

Please removed the redundant code in igc_rx_init
  
Zeng, ZhichaoX July 1, 2022, 3:13 a.m. UTC | #2
>> The RX function was not specified in the secondary process, causing 
>> the secondary process to segfault in a multi-process environment.
>> 
>> This patch specify RX/TX functions in "dev_init" to support secondary processes.
>> 
>> Fixes: 66fde1b943eb ("net/igc: add skeleton")
>> Cc: alvinx.zhang@intel.com
>> Cc: stable@dpdk.org
>> 
>> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
>> 
>> ---
>> v2:
>> remove unnecessary parameters, move declaration to relevant header 
>> file
>> ---
>> v3:
>> remove redundant code, optimize commit log
>> ---
>> v4:
>> rework patch
>> ---
>>  drivers/net/igc/igc_ethdev.c | 9 ++++++++-
>>  drivers/net/igc/igc_txrx.c   | 8 ++++----
>>  drivers/net/igc/igc_txrx.h   | 6 ++++++
>>  3 files changed, 18 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/net/igc/igc_ethdev.c 
>> b/drivers/net/igc/igc_ethdev.c index
>> b9933b395d..7f221a5d34 100644
>> --- a/drivers/net/igc/igc_ethdev.c
>> +++ b/drivers/net/igc/igc_ethdev.c
>> @@ -1240,8 +1240,15 @@ eth_igc_dev_init(struct rte_eth_dev *dev)
>>  	 * has already done this work. Only check we don't need a different
>>  	 * RX function.
>>  	 */
>> -	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>> +		dev->rx_pkt_burst = igc_recv_pkts;
>> +		if (dev->data->scattered_rx)
>> +			dev->rx_pkt_burst = igc_recv_scattered_pkts;
>
>Please removed the redundant code in igc_rx_init

Only the main process will execute "igc_rx_init", and the secondary process will not execute it.
So, the data path of the secondary process is not initialized. 

The code that this patch adds to initialize the data path in "dev_init" will only be executed in the
secondary process. The same code in "igc_rx_init" is not redundant.

May I ask if the commit log is confusing, and should I submit new patch to correct it?

Regards
Zhichao
  
Qi Zhang July 1, 2022, 3:47 a.m. UTC | #3
> -----Original Message-----
> From: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Sent: Friday, July 1, 2022 11:13 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>;
> alvinx.zhang@intel.com; Guo, Junfeng <junfeng.guo@intel.com>; Su, Simei
> <simei.su@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
> Ferruh Yigit <ferruh.yigit@intel.com>
> Subject: RE: [PATCH v4] net/igc: add support for secondary processes
> 
> >> The RX function was not specified in the secondary process, causing
> >> the secondary process to segfault in a multi-process environment.
> >>
> >> This patch specify RX/TX functions in "dev_init" to support secondary
> processes.
> >>
> >> Fixes: 66fde1b943eb ("net/igc: add skeleton")
> >> Cc: alvinx.zhang@intel.com
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> >>
> >> ---
> >> v2:
> >> remove unnecessary parameters, move declaration to relevant header
> >> file
> >> ---
> >> v3:
> >> remove redundant code, optimize commit log
> >> ---
> >> v4:
> >> rework patch
> >> ---
> >>  drivers/net/igc/igc_ethdev.c | 9 ++++++++-
> >>  drivers/net/igc/igc_txrx.c   | 8 ++++----
> >>  drivers/net/igc/igc_txrx.h   | 6 ++++++
> >>  3 files changed, 18 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/igc/igc_ethdev.c
> >> b/drivers/net/igc/igc_ethdev.c index
> >> b9933b395d..7f221a5d34 100644
> >> --- a/drivers/net/igc/igc_ethdev.c
> >> +++ b/drivers/net/igc/igc_ethdev.c
> >> @@ -1240,8 +1240,15 @@ eth_igc_dev_init(struct rte_eth_dev *dev)
> >>  	 * has already done this work. Only check we don't need a different
> >>  	 * RX function.
> >>  	 */
> >> -	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> >> +		dev->rx_pkt_burst = igc_recv_pkts;
> >> +		if (dev->data->scattered_rx)
> >> +			dev->rx_pkt_burst = igc_recv_scattered_pkts;
> >
> >Please removed the redundant code in igc_rx_init
> 
> Only the main process will execute "igc_rx_init", and the secondary process
> will not execute it.
> So, the data path of the secondary process is not initialized.
> 
> The code that this patch adds to initialize the data path in "dev_init" will
> only be executed in the secondary process. The same code in "igc_rx_init" is
> not redundant.

Ok, I missed this point,  the implementation is correct.

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

Applied to dpdk-next-net-intel.

Thanks
Qi

> 
> May I ask if the commit log is confusing, and should I submit new patch to
> correct it?
> 
> Regards
> Zhichao
  

Patch

diff --git a/drivers/net/igc/igc_ethdev.c b/drivers/net/igc/igc_ethdev.c
index b9933b395d..7f221a5d34 100644
--- a/drivers/net/igc/igc_ethdev.c
+++ b/drivers/net/igc/igc_ethdev.c
@@ -1240,8 +1240,15 @@  eth_igc_dev_init(struct rte_eth_dev *dev)
 	 * has already done this work. Only check we don't need a different
 	 * RX function.
 	 */
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		dev->rx_pkt_burst = igc_recv_pkts;
+		if (dev->data->scattered_rx)
+			dev->rx_pkt_burst = igc_recv_scattered_pkts;
+
+		dev->tx_pkt_burst = igc_xmit_pkts;
+		dev->tx_pkt_prepare = eth_igc_prep_pkts;
 		return 0;
+	}
 
 	rte_eth_copy_pci_info(dev, pci_dev);
 	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
index e48d5df11a..ffd219b0df 100644
--- a/drivers/net/igc/igc_txrx.c
+++ b/drivers/net/igc/igc_txrx.c
@@ -345,7 +345,7 @@  rx_desc_get_pkt_info(struct igc_rx_queue *rxq, struct rte_mbuf *rxm,
 	rxm->packet_type = rx_desc_pkt_info_to_pkt_type(pkt_info);
 }
 
-static uint16_t
+uint16_t
 igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 {
 	struct igc_rx_queue * const rxq = rx_queue;
@@ -488,7 +488,7 @@  igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 	return nb_rx;
 }
 
-static uint16_t
+uint16_t
 igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 			uint16_t nb_pkts)
 {
@@ -1397,7 +1397,7 @@  eth_igc_rx_queue_setup(struct rte_eth_dev *dev,
 }
 
 /* prepare packets for transmit */
-static uint16_t
+uint16_t
 eth_igc_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 		uint16_t nb_pkts)
 {
@@ -1604,7 +1604,7 @@  tx_desc_cksum_flags_to_olinfo(uint64_t ol_flags)
 	return tmp;
 }
 
-static uint16_t
+uint16_t
 igc_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 {
 	struct igc_tx_queue * const txq = tx_queue;
diff --git a/drivers/net/igc/igc_txrx.h b/drivers/net/igc/igc_txrx.h
index 535108a868..02a0a051bb 100644
--- a/drivers/net/igc/igc_txrx.h
+++ b/drivers/net/igc/igc_txrx.h
@@ -49,6 +49,12 @@  void eth_igc_txq_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
 	struct rte_eth_txq_info *qinfo);
 void eth_igc_vlan_strip_queue_set(struct rte_eth_dev *dev,
 			uint16_t rx_queue_id, int on);
+uint16_t igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
+uint16_t igc_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
+uint16_t eth_igc_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+	uint16_t nb_pkts);
+uint16_t igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
+	uint16_t nb_pkts);
 #ifdef __cplusplus
 }
 #endif