Re: [dpdk-dev] [PATCH V3 3/7] app/testpmd: fix a segment fault when DCB test

[Huisong Li <lihuisong@huawei.com>]

在 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
>>> .