Re: [dpdk-dev] [PATCH 2/3] app/testpmd: remove forwarding config from parsing Rx and Tx

[oulijun <oulijun@huawei.com>]

在 2021/3/23 15:50, Li, Xiaoyun 写道:
> Hi
> 
>> -----Original Message-----
>> From: Lijun Ou <oulijun@huawei.com>
>> Sent: Friday, March 5, 2021 18:22
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
>> linuxarm@openeuler.org
>> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from parsing Rx
>> and Tx
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
> 
> The commit message should be more simple and avoids grammar mistakes.
> 
All right, I will simply it.
>> The "fwd_config_setup()" function does release and apply for memory of
>> forwarding flows, and re-establish these streams when rxq/txq or rxd/txd is
>> changed. The function is also called by "start_packet_forwarding()" when user
>> executes "start" cmd.
>> All changes for rxq/txq or rxd/txd can be updated uniformly when this command
>> is executed. Therefore, it is a little redundant in the "cmd_config_rx_tx_parsed"
>> function.
> 
> It's not redundant. This command may configure number of rxq/txq. So the fwd streams map may change.
> Then it's common to check the fwd streams after this command using "show config fwd".
> If you remove this fwd stream update, users can't get the correct new fwd streams until they start the traffic.
> But they may change a lot of things and want to check if the setting is correct before they start the traffic.
> 
Yes, you are right. It's really unfriendly.
>>
>> In addition, the forwarding stream under one TC is configured based on number
>> of queues allocated to TC. And number of queues allocated to TC is updated
>> after calling  "rte_eth_dev_configure"
>> again. If the number of queues is reduced after configuring the DCB, and then,
>> release and apply for stream memory, and reinitialize the forwarding stream
>> under the DCB mode based on the old TC information. As a result, null pointer
>> may be accessed.
> 
> I think you should add "rte_eth_dev_configure " into dcb_fwd_config_setup() before rte_eth_dev_get_dcb_info().
> 
> And the commit message should be similar like the following:
> Segment fault might happen after configuring queue number to less because dcb_fwd_config_setup setup dcb based on old dcb info.
> And dcb info can only update after rte_eth_dev_configure().
> So this patch adds rte_eth_dev_configure() before rte_eth_dev_get_dcb_info() to get updated dcb info to fix this issue.
> 
Thank you for your advice. But the above adjustments may still not work 
for some drivers. The mapping between queues and TCs
in these drivers is updated in the dev_start stage.

I have an idea. We can move fwd_config_setup() to start_port(), which is 
called by main() and after starting ports
This not only solves the segment fault, but also does not have the 
problem you mentioned above. I test it and it is ok.

What do you think, xiaoyun?


>>
>> 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
>>
>> At the moment, a segmentation fault occurs.
>>
>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V1->V2:
>> - use stream instead of flow
>> ---
>>   app/test-pmd/cmdline.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>> 4df0c32..e316f5c 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
>>   		return;
>>   	}
>>
>> -	fwd_config_setup();
>> -
>>   	init_port_config();
>>
>>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> --
>> 2.7.4
> 
> .
>