diff mbox series

[V3,3/7] app/testpmd: fix a segment fault when DCB test

Message ID 1618909266-17584-4-git-send-email-lihuisong@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series modifications about DCB forwarding configuration |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

lihuisong (C) April 20, 2021, 9:01 a.m. UTC 
  After DCB mode is configured, if we decrease the number of RX and TX
queues, fwd_config_setup() will be called to setup the DCB forwarding
configuration. And forwarding streams are updated based on new queue
numbers in fwd_config_setup(), but the mapping between the TC and
queues obtained by rte_eth_dev_get_dcb_info() is still old queue
numbers (old queue numbers are greater than new queue numbers).
In this case, the segment fault happens. So rte_eth_dev_configure()
should be called again to update the mapping between the TC and
queues before rte_eth_dev_get_dcb_info().

Like:
set nbcore 4
port stop all
port config 0 dcb vt off 4 pfc on
port start all
port stop all
port config all rxq 8
port config all txq 8

Fixes: 900550de04a7 ("app/testpmd: add dcb support")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 app/test-pmd/config.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Li, Xiaoyun April 27, 2021, 11:19 a.m. UTC | #1 
> -----Original Message-----
> From: Huisong Li <lihuisong@huawei.com>
> Sent: Tuesday, April 20, 2021 17:01
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> linuxarm@openeuler.org; lihuisong@huawei.com
> Subject: [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB test
> 
> After DCB mode is configured, if we decrease the number of RX and TX queues,
> fwd_config_setup() will be called to setup the DCB forwarding configuration.
> And forwarding streams are updated based on new queue numbers in
> fwd_config_setup(), but the mapping between the TC and queues obtained by
> rte_eth_dev_get_dcb_info() is still old queue numbers (old queue numbers are
> greater than new queue numbers).
> In this case, the segment fault happens. So rte_eth_dev_configure() should be
> called again to update the mapping between the TC and queues before
> rte_eth_dev_get_dcb_info().
> 
> Like:
> set nbcore 4
> port stop all
> port config 0 dcb vt off 4 pfc on
> port start all
> port stop all
> port config all rxq 8
> port config all txq 8
> 
> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>  app/test-pmd/config.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 03ee40c..18b197b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2996,7 +2996,33 @@ dcb_fwd_config_setup(void)
>  	uint16_t nb_rx_queue, nb_tx_queue;
>  	uint16_t i, j, k, sm_id = 0;
>  	uint16_t total_tc_num;
> +	struct rte_port *port;
>  	uint8_t tc = 0;
> +	portid_t pid;
> +	int ret;
> +
> +	/*
> +	 * The fwd_config_setup() is called when the port is
> RTE_PORT_STARTED

When the port is RTE_PORT_STARTED or RTE_PORT_STARTED? Missing an 'or' here.
Maybe something like the following will be better? (just a reference, you can put it in a better way)
/*
  * Re-configure ports to get updated mapping between tc and queue in case the queue number of the port is changed.
  * Skip for started ports since configuring queue number needs to stop ports first.
  */

> +	 * RTE_PORT_STOPPED. When a port is RTE_PORT_STARTED,
> dev_configure
> +	 * cannot be called.
> +	 *
> +	 * re-configure the device after changing queue numbers of stopped
> +	 * ports, so that the updated mapping between tc and queue can be
> +	 * obtained.
> +	 */
> +	for (pid = 0; pid < nb_fwd_ports; pid++) {
> +		if (port_is_started(pid) == 1)
> +			continue;
> +
> +		port = &ports[pid];
> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> +					    &port->dev_conf);
> +		if (ret < 0) {
> +			printf("Failed to re-configure port %d, ret = %d.\n",
> +				pid, ret);
> +			return;
> +		}
> +	}
> 
>  	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>  	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
> --
> 2.7.4
lihuisong (C) April 27, 2021, 2:10 p.m. UTC | #2 
在 2021/4/27 19:19, Li, Xiaoyun 写道:
>
>> -----Original Message-----
>> From: Huisong Li <lihuisong@huawei.com>
>> Sent: Tuesday, April 20, 2021 17:01
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>> linuxarm@openeuler.org; lihuisong@huawei.com
>> Subject: [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB test
>>
>> After DCB mode is configured, if we decrease the number of RX and TX queues,
>> fwd_config_setup() will be called to setup the DCB forwarding configuration.
>> And forwarding streams are updated based on new queue numbers in
>> fwd_config_setup(), but the mapping between the TC and queues obtained by
>> rte_eth_dev_get_dcb_info() is still old queue numbers (old queue numbers are
>> greater than new queue numbers).
>> In this case, the segment fault happens. So rte_eth_dev_configure() should be
>> called again to update the mapping between the TC and queues before
>> rte_eth_dev_get_dcb_info().
>>
>> Like:
>> set nbcore 4
>> port stop all
>> port config 0 dcb vt off 4 pfc on
>> port start all
>> port stop all
>> port config all rxq 8
>> port config all txq 8
>>
>> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>>   app/test-pmd/config.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>> 03ee40c..18b197b 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2996,7 +2996,33 @@ dcb_fwd_config_setup(void)
>>   	uint16_t nb_rx_queue, nb_tx_queue;
>>   	uint16_t i, j, k, sm_id = 0;
>>   	uint16_t total_tc_num;
>> +	struct rte_port *port;
>>   	uint8_t tc = 0;
>> +	portid_t pid;
>> +	int ret;
>> +
>> +	/*
>> +	 * The fwd_config_setup() is called when the port is
>> RTE_PORT_STARTED
> When the port is RTE_PORT_STARTED or RTE_PORT_STARTED? Missing an 'or' here.
ok
> Maybe something like the following will be better? (just a reference, you can put it in a better way)
> /*
>    * Re-configure ports to get updated mapping between tc and queue in case the queue number of the port is changed.
>    * Skip for started ports since configuring queue number needs to stop ports first.
>    */

Thank you😁 I'm going to fix it as follows:

     /*
      * The fwd_config_setup() is called when the port is RTE_PORT_STARTED
      * or RTE_PORT_STOPPED.
      *
      * Re-configure ports to get updated mapping between tc and queue when
      * the queue number of the port is changed. Skip for started ports 
since
      * modifying queue number and calling dev_configure need to stop ports
      * first.
      */

>> +	 * RTE_PORT_STOPPED. When a port is RTE_PORT_STARTED,
>> dev_configure
>> +	 * cannot be called.
>> +	 *
>> +	 * re-configure the device after changing queue numbers of stopped
>> +	 * ports, so that the updated mapping between tc and queue can be
>> +	 * obtained.
>> +	 */
>> +	for (pid = 0; pid < nb_fwd_ports; pid++) {
>> +		if (port_is_started(pid) == 1)
>> +			continue;
>> +
>> +		port = &ports[pid];
>> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
>> +					    &port->dev_conf);
>> +		if (ret < 0) {
>> +			printf("Failed to re-configure port %d, ret = %d.\n",
>> +				pid, ret);
>> +			return;
>> +		}
>> +	}
>>
>>   	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>>   	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>> --
>> 2.7.4
> .
Li, Xiaoyun April 28, 2021, 2:02 a.m. UTC | #3 
> -----Original Message-----
> From: Huisong Li <lihuisong@huawei.com>
> Sent: Tuesday, April 27, 2021 22:11
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB test
> 
> 
> 在 2021/4/27 19:19, Li, Xiaoyun 写道:
> >
> >> -----Original Message-----
> >> From: Huisong Li <lihuisong@huawei.com>
> >> Sent: Tuesday, April 20, 2021 17:01
> >> To: dev@dpdk.org
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >> <xiaoyun.li@intel.com>; linuxarm@openeuler.org; lihuisong@huawei.com
> >> Subject: [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB
> >> test
> >>
> >> After DCB mode is configured, if we decrease the number of RX and TX
> >> queues,
> >> fwd_config_setup() will be called to setup the DCB forwarding configuration.
> >> And forwarding streams are updated based on new queue numbers in
> >> fwd_config_setup(), but the mapping between the TC and queues
> >> obtained by
> >> rte_eth_dev_get_dcb_info() is still old queue numbers (old queue
> >> numbers are greater than new queue numbers).
> >> In this case, the segment fault happens. So rte_eth_dev_configure()
> >> should be called again to update the mapping between the TC and
> >> queues before rte_eth_dev_get_dcb_info().
> >>
> >> Like:
> >> set nbcore 4
> >> port stop all
> >> port config 0 dcb vt off 4 pfc on
> >> port start all
> >> port stop all
> >> port config all rxq 8
> >> port config all txq 8
> >>
> >> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >> ---
> >>   app/test-pmd/config.c | 26 ++++++++++++++++++++++++++
> >>   1 file changed, 26 insertions(+)
> >>
> >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >> 03ee40c..18b197b 100644
> >> --- a/app/test-pmd/config.c
> >> +++ b/app/test-pmd/config.c
> >> @@ -2996,7 +2996,33 @@ dcb_fwd_config_setup(void)
> >>   	uint16_t nb_rx_queue, nb_tx_queue;
> >>   	uint16_t i, j, k, sm_id = 0;
> >>   	uint16_t total_tc_num;
> >> +	struct rte_port *port;
> >>   	uint8_t tc = 0;
> >> +	portid_t pid;
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * The fwd_config_setup() is called when the port is
> >> RTE_PORT_STARTED
> > When the port is RTE_PORT_STARTED or RTE_PORT_STARTED? Missing an 'or'
> here.
> ok
> > Maybe something like the following will be better? (just a reference,
> > you can put it in a better way)
> > /*
> >    * Re-configure ports to get updated mapping between tc and queue in case
> the queue number of the port is changed.
> >    * Skip for started ports since configuring queue number needs to stop ports
> first.
> >    */
> 
> Thank you😁 I'm going to fix it as follows:
> 
>      /*
>       * The fwd_config_setup() is called when the port is RTE_PORT_STARTED
>       * or RTE_PORT_STOPPED.
>       *
>       * Re-configure ports to get updated mapping between tc and queue when
>       * the queue number of the port is changed. Skip for started ports since
>       * modifying queue number and calling dev_configure need to stop ports
>       * first.
>       */
> 
Overall, looks good to me.
Just one suggestion, what about change "when the queue number" to "in case the queue number" because you actually don't know if the queue number is changed or not.

> >> +	 * RTE_PORT_STOPPED. When a port is RTE_PORT_STARTED,
> >> dev_configure
> >> +	 * cannot be called.
> >> +	 *
> >> +	 * re-configure the device after changing queue numbers of stopped
> >> +	 * ports, so that the updated mapping between tc and queue can be
> >> +	 * obtained.
> >> +	 */
> >> +	for (pid = 0; pid < nb_fwd_ports; pid++) {
> >> +		if (port_is_started(pid) == 1)
> >> +			continue;
> >> +
> >> +		port = &ports[pid];
> >> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> >> +					    &port->dev_conf);
> >> +		if (ret < 0) {
> >> +			printf("Failed to re-configure port %d, ret = %d.\n",
> >> +				pid, ret);
> >> +			return;
> >> +		}
> >> +	}
> >>
> >>   	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
> >>   	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
> >> --
> >> 2.7.4
> > .
lihuisong (C) April 28, 2021, 2:26 a.m. UTC | #4 
在 2021/4/28 10:02, Li, Xiaoyun 写道:
>
>> -----Original Message-----
>> From: Huisong Li <lihuisong@huawei.com>
>> Sent: Tuesday, April 27, 2021 22:11
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: Re: [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB test
>>
>>
>> 在 2021/4/27 19:19, Li, Xiaoyun 写道:
>>>> -----Original Message-----
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>> Sent: Tuesday, April 20, 2021 17:01
>>>> To: dev@dpdk.org
>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
>>>> <xiaoyun.li@intel.com>; linuxarm@openeuler.org; lihuisong@huawei.com
>>>> Subject: [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB
>>>> test
>>>>
>>>> After DCB mode is configured, if we decrease the number of RX and TX
>>>> queues,
>>>> fwd_config_setup() will be called to setup the DCB forwarding configuration.
>>>> And forwarding streams are updated based on new queue numbers in
>>>> fwd_config_setup(), but the mapping between the TC and queues
>>>> obtained by
>>>> rte_eth_dev_get_dcb_info() is still old queue numbers (old queue
>>>> numbers are greater than new queue numbers).
>>>> In this case, the segment fault happens. So rte_eth_dev_configure()
>>>> should be called again to update the mapping between the TC and
>>>> queues before rte_eth_dev_get_dcb_info().
>>>>
>>>> Like:
>>>> set nbcore 4
>>>> port stop all
>>>> port config 0 dcb vt off 4 pfc on
>>>> port start all
>>>> port stop all
>>>> port config all rxq 8
>>>> port config all txq 8
>>>>
>>>> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> ---
>>>>    app/test-pmd/config.c | 26 ++++++++++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>> 03ee40c..18b197b 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -2996,7 +2996,33 @@ dcb_fwd_config_setup(void)
>>>>    	uint16_t nb_rx_queue, nb_tx_queue;
>>>>    	uint16_t i, j, k, sm_id = 0;
>>>>    	uint16_t total_tc_num;
>>>> +	struct rte_port *port;
>>>>    	uint8_t tc = 0;
>>>> +	portid_t pid;
>>>> +	int ret;
>>>> +
>>>> +	/*
>>>> +	 * The fwd_config_setup() is called when the port is
>>>> RTE_PORT_STARTED
>>> When the port is RTE_PORT_STARTED or RTE_PORT_STARTED? Missing an 'or'
>> here.
>> ok
>>> Maybe something like the following will be better? (just a reference,
>>> you can put it in a better way)
>>> /*
>>>     * Re-configure ports to get updated mapping between tc and queue in case
>> the queue number of the port is changed.
>>>     * Skip for started ports since configuring queue number needs to stop ports
>> first.
>>>     */
>> Thank you😁 I'm going to fix it as follows:
>>
>>       /*
>>        * The fwd_config_setup() is called when the port is RTE_PORT_STARTED
>>        * or RTE_PORT_STOPPED.
>>        *
>>        * Re-configure ports to get updated mapping between tc and queue when
>>        * the queue number of the port is changed. Skip for started ports since
>>        * modifying queue number and calling dev_configure need to stop ports
>>        * first.
>>        */
>>
> Overall, looks good to me.
> Just one suggestion, what about change "when the queue number" to "in case the queue number" because you actually don't know if the queue number is changed or not.
Yes, there is a difference in meaning. I will fix it in next version.
>
>>>> +	 * RTE_PORT_STOPPED. When a port is RTE_PORT_STARTED,
>>>> dev_configure
>>>> +	 * cannot be called.
>>>> +	 *
>>>> +	 * re-configure the device after changing queue numbers of stopped
>>>> +	 * ports, so that the updated mapping between tc and queue can be
>>>> +	 * obtained.
>>>> +	 */
>>>> +	for (pid = 0; pid < nb_fwd_ports; pid++) {
>>>> +		if (port_is_started(pid) == 1)
>>>> +			continue;
>>>> +
>>>> +		port = &ports[pid];
>>>> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
>>>> +					    &port->dev_conf);
>>>> +		if (ret < 0) {
>>>> +			printf("Failed to re-configure port %d, ret = %d.\n",
>>>> +				pid, ret);
>>>> +			return;
>>>> +		}
>>>> +	}
>>>>
>>>>    	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>>>>    	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>>>> --
>>>> 2.7.4
>>> .
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 03ee40c..18b197b 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2996,7 +2996,33 @@  dcb_fwd_config_setup(void)
 	uint16_t nb_rx_queue, nb_tx_queue;
 	uint16_t i, j, k, sm_id = 0;
 	uint16_t total_tc_num;
+	struct rte_port *port;
 	uint8_t tc = 0;
+	portid_t pid;
+	int ret;
+
+	/*
+	 * The fwd_config_setup() is called when the port is RTE_PORT_STARTED
+	 * RTE_PORT_STOPPED. When a port is RTE_PORT_STARTED, dev_configure
+	 * cannot be called.
+	 *
+	 * re-configure the device after changing queue numbers of stopped
+	 * ports, so that the updated mapping between tc and queue can be
+	 * obtained.
+	 */
+	for (pid = 0; pid < nb_fwd_ports; pid++) {
+		if (port_is_started(pid) == 1)
+			continue;
+
+		port = &ports[pid];
+		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
+					    &port->dev_conf);
+		if (ret < 0) {
+			printf("Failed to re-configure port %d, ret = %d.\n",
+				pid, ret);
+			return;
+		}
+	}
 
 	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
 	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;