diff mbox series

[1/3] app/testpmd: fix forwarding configuration when DCB test

Message ID 1614939741-63927-2-git-send-email-oulijun@huawei.com (mailing list archive)
State New
Delegated to: Ferruh Yigit
Headers show
Series Fixes for testpmd | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Lijun Ou March 5, 2021, 10:22 a.m. UTC
From: Huisong Li <lihuisong@huawei.com>

Currently, 'nb_fwd_lcores' value are both adjusted based on
'nb_fwd_streams' in rss/simple/icmp_echo_fwd_config_setup. But the
operation is lack for dcb_fwd_config_setup, which may lead to a
bad events where multiple polling threads operate on the same
queue. As a result, various possible exceptions will occur. The
procedure is as follows:

startup testpmd app as follows:
testpmd> port stop all
testpmd> set nbcore 8
testpmd> port config 0 dcb vt off 4 pfc on
testpmd> port config all rxq 16
testpmd> port config all txq 16
testpmd> port start all
testpmd> set fwd mac
Logical Core 1 (socket 0) forwards packets on 4 streams:
RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00
Logical Core 2 (socket 0) forwards packets on 4 streams:
RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00
Logical Core 3 (socket 0) forwards packets on 4 streams:
RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00
Logical Core 4 (socket 0) forwards packets on 4 streams:
RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00
RX P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00
Logical Core 5 (socket 0) forwards packets on 1 streams:
RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00
Logical Core 6 (socket 0) forwards packets on 1 streams:
RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00
Logical Core 7 (socket 0) forwards packets on 1 streams:
RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00
Logical Core 8 (socket 0) forwards packets on 1 streams:
RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00

Notice: number of rxq and txq in running cmdline are greater than
used nb_tc.

For the DCB forwarding test, each core is assigned on each traffic class
and each core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
value can be adjusted based on 'total_tc_num' in all forwarding ports.

In addition, after operation of port stop/port start, forwarding
configuration is changed by rss_fwd_config_setup. In "start_port"
function, dcb_test is set to 1 based on dcb_config. So it also
should be cleared when dcb_config is 0.

Fixes: 900550de04a7 ("app/testpmd: add dcb support")
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>
---
 app/test-pmd/config.c  | 19 +++++++++++++++++++
 app/test-pmd/testpmd.c | 12 +++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

Comments

Li, Xiaoyun March 23, 2021, 8:55 a.m. UTC | #1
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 1/3] app/testpmd: fix forwarding configuration when DCB test
> 
> From: Huisong Li <lihuisong@huawei.com>
> 
The commit message is too long and redundant. Please simplify it.

> Currently, 'nb_fwd_lcores' value are both adjusted based on 'nb_fwd_streams' in
> rss/simple/icmp_echo_fwd_config_setup. But the operation is lack for
> dcb_fwd_config_setup, which may lead to a bad events where multiple polling
> threads operate on the same queue. As a result, various possible exceptions will
> occur. The procedure is as follows:
> 
> startup testpmd app as follows:
> testpmd> port stop all
> testpmd> set nbcore 8
> testpmd> port config 0 dcb vt off 4 pfc on port config all rxq 16 port
> testpmd> config all txq 16 port start all set fwd mac
> Logical Core 1 (socket 0) forwards packets on 4 streams:
> RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00 Logical
> Core 2 (socket 0) forwards packets on 4 streams:
> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00 Logical
> Core 3 (socket 0) forwards packets on 4 streams:
> RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00 Logical
> Core 4 (socket 0) forwards packets on 4 streams:
> RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00 RX
> P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00 Logical
> Core 5 (socket 0) forwards packets on 1 streams:
> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 Logical
> Core 6 (socket 0) forwards packets on 1 streams:
> RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 Logical
> Core 7 (socket 0) forwards packets on 1 streams:
> RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 Logical
> Core 8 (socket 0) forwards packets on 1 streams:
> RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00
> 
> Notice: number of rxq and txq in running cmdline are greater than used nb_tc.
> 
> For the DCB forwarding test, each core is assigned on each traffic class and each
> core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
> value can be adjusted based on 'total_tc_num' in all forwarding ports.
> 
> In addition, after operation of port stop/port start, forwarding configuration is
> changed by rss_fwd_config_setup. In "start_port"
> function, dcb_test is set to 1 based on dcb_config. So it also should be cleared
> when dcb_config is 0.
> 
> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
> 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>
> ---
>  app/test-pmd/config.c  | 19 +++++++++++++++++++  app/test-pmd/testpmd.c |
> 12 +++++++-----
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 576d5ac..c89f8cd 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2864,6 +2864,21 @@ rss_fwd_config_setup(void)
>  	}
>  }
> 
> +static uint16_t
> +get_fwd_port_total_tc_num(void)
> +{
> +	struct rte_eth_dcb_info dcb_info;
> +	uint16_t total_tc_num = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < nb_fwd_ports; i++) {
> +		(void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
> +		total_tc_num += dcb_info.nb_tcs;
> +	}
> +
> +	return total_tc_num;
> +}

It's only 3 lines. Is it really necessary to make it into a function which is not called by anyone else? A comment and the loop are enough.

> +
>  /**
>   * For the DCB forwarding test, each core is assigned on each traffic class.
>   *
> @@ -2883,12 +2898,16 @@ dcb_fwd_config_setup(void)
>  	lcoreid_t  lc_id;
>  	uint16_t nb_rx_queue, nb_tx_queue;
>  	uint16_t i, j, k, sm_id = 0;
> +	uint16_t total_tc_num = 0;
>  	uint8_t tc = 0;
> 
>  	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>  	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>  	cur_fwd_config.nb_fwd_streams =
>  		(streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
> +	total_tc_num = get_fwd_port_total_tc_num();
> +	if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
> +		cur_fwd_config.nb_fwd_lcores = total_tc_num;
> 
>  	/* reinitialize forwarding streams */
>  	init_fwd_streams();
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 1a57324..7eb810b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>  	portid_t peer_pl[RTE_MAX_ETHPORTS];
>  	int peer_pi;
> 
> -	if (dcb_test) {
> -		dcb_test = 0;
> -		dcb_config = 0;
> -	}
> -
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return;
> 
> +	/*
> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
> +	 * So it should be cleared when dcb_config is 0.
> +	 */
> +	if (dcb_config == 0)
> +		dcb_test = 0;
> +

I don't understand why are you changing this.
dcb_test will only be set when dcb_config is 1 when starting ports. And both dcb_test and dcb_config will be cleared when stopping ports.
So dcb will only affect when you set port dcb and then start port and when stop port dcb will be cleared.

So what's the problem of original codes?

Your change will cause issues that there's no place to set dcb_config as 0. If you config dcb, then it'll be always dcb mode unless restart the whole testpmd.

>  	printf("Stopping ports...\n");
> 
>  	RTE_ETH_FOREACH_DEV(pi) {
> --
> 2.7.4
Lijun Ou March 23, 2021, 2:17 p.m. UTC | #2
在 2021/3/23 16:55, 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 1/3] app/testpmd: fix forwarding configuration when DCB test
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
> The commit message is too long and redundant. Please simplify it.
> 
OK, I will fix it.
>> Currently, 'nb_fwd_lcores' value are both adjusted based on 'nb_fwd_streams' in
>> rss/simple/icmp_echo_fwd_config_setup. But the operation is lack for
>> dcb_fwd_config_setup, which may lead to a bad events where multiple polling
>> threads operate on the same queue. As a result, various possible exceptions will
>> occur. The procedure is as follows:
>>
>> startup testpmd app as follows:
>> testpmd> port stop all
>> testpmd> set nbcore 8
>> testpmd> port config 0 dcb vt off 4 pfc on port config all rxq 16 port
>> testpmd> config all txq 16 port start all set fwd mac
>> Logical Core 1 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 2 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 3 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 4 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 5 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 6 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 7 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 8 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00
>>
>> Notice: number of rxq and txq in running cmdline are greater than used nb_tc.
>>
>> For the DCB forwarding test, each core is assigned on each traffic class and each
>> core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
>> value can be adjusted based on 'total_tc_num' in all forwarding ports.
>>
>> In addition, after operation of port stop/port start, forwarding configuration is
>> changed by rss_fwd_config_setup. In "start_port"
>> function, dcb_test is set to 1 based on dcb_config. So it also should be cleared
>> when dcb_config is 0.
>>
>> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
>> 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>
>> ---
>>   app/test-pmd/config.c  | 19 +++++++++++++++++++  app/test-pmd/testpmd.c |
>> 12 +++++++-----
>>   2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>> 576d5ac..c89f8cd 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2864,6 +2864,21 @@ rss_fwd_config_setup(void)
>>   	}
>>   }
>>
>> +static uint16_t
>> +get_fwd_port_total_tc_num(void)
>> +{
>> +	struct rte_eth_dcb_info dcb_info;
>> +	uint16_t total_tc_num = 0;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < nb_fwd_ports; i++) {
>> +		(void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
>> +		total_tc_num += dcb_info.nb_tcs;
>> +	}
>> +
>> +	return total_tc_num;
>> +}
> 
> It's only 3 lines. Is it really necessary to make it into a function which is not called by anyone else? A comment and the loop are enough.
> 
>> +
>>   /**
>>    * For the DCB forwarding test, each core is assigned on each traffic class.
>>    *
>> @@ -2883,12 +2898,16 @@ dcb_fwd_config_setup(void)
>>   	lcoreid_t  lc_id;
>>   	uint16_t nb_rx_queue, nb_tx_queue;
>>   	uint16_t i, j, k, sm_id = 0;
>> +	uint16_t total_tc_num = 0;
>>   	uint8_t tc = 0;
>>
>>   	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>>   	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>>   	cur_fwd_config.nb_fwd_streams =
>>   		(streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
>> +	total_tc_num = get_fwd_port_total_tc_num();
>> +	if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
>> +		cur_fwd_config.nb_fwd_lcores = total_tc_num;
>>
>>   	/* reinitialize forwarding streams */
>>   	init_fwd_streams();
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 1a57324..7eb810b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>   	portid_t peer_pl[RTE_MAX_ETHPORTS];
>>   	int peer_pi;
>>
>> -	if (dcb_test) {
>> -		dcb_test = 0;
>> -		dcb_config = 0;
>> -	}
>> -
>>   	if (port_id_is_invalid(pid, ENABLED_WARN))
>>   		return;
>>
>> +	/*
>> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>> +	 * So it should be cleared when dcb_config is 0.
>> +	 */
>> +	if (dcb_config == 0)
>> +		dcb_test = 0;
>> +
> 
> I don't understand why are you changing this.
> dcb_test will only be set when dcb_config is 1 when starting ports. And both dcb_test and dcb_config will be cleared when stopping ports.
> So dcb will only affect when you set port dcb and then start port and when stop port dcb will be cleared.
> 
> So what's the problem of original codes?
> 
> Your change will cause issues that there's no place to set dcb_config as 0. If you config dcb, then it'll be always dcb mode unless restart the whole testpmd.
> 
>>   	printf("Stopping ports...\n");
>>
>>   	RTE_ETH_FOREACH_DEV(pi) {
>> --
>> 2.7.4
> 
> .
>
Lijun Ou March 23, 2021, 2:17 p.m. UTC | #3
在 2021/3/23 16:55, 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 1/3] app/testpmd: fix forwarding configuration when DCB test
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
> The commit message is too long and redundant. Please simplify it.
> 
>> Currently, 'nb_fwd_lcores' value are both adjusted based on 'nb_fwd_streams' in
>> rss/simple/icmp_echo_fwd_config_setup. But the operation is lack for
>> dcb_fwd_config_setup, which may lead to a bad events where multiple polling
>> threads operate on the same queue. As a result, various possible exceptions will
>> occur. The procedure is as follows:
>>
>> startup testpmd app as follows:
>> testpmd> port stop all
>> testpmd> set nbcore 8
>> testpmd> port config 0 dcb vt off 4 pfc on port config all rxq 16 port
>> testpmd> config all txq 16 port start all set fwd mac
>> Logical Core 1 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 2 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 3 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 4 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 5 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 6 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 7 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 8 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00
>>
>> Notice: number of rxq and txq in running cmdline are greater than used nb_tc.
>>
>> For the DCB forwarding test, each core is assigned on each traffic class and each
>> core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
>> value can be adjusted based on 'total_tc_num' in all forwarding ports.
>>
>> In addition, after operation of port stop/port start, forwarding configuration is
>> changed by rss_fwd_config_setup. In "start_port"
>> function, dcb_test is set to 1 based on dcb_config. So it also should be cleared
>> when dcb_config is 0.
>>
>> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
>> 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>
>> ---
>>   app/test-pmd/config.c  | 19 +++++++++++++++++++  app/test-pmd/testpmd.c |
>> 12 +++++++-----
>>   2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>> 576d5ac..c89f8cd 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2864,6 +2864,21 @@ rss_fwd_config_setup(void)
>>   	}
>>   }
>>
>> +static uint16_t
>> +get_fwd_port_total_tc_num(void)
>> +{
>> +	struct rte_eth_dcb_info dcb_info;
>> +	uint16_t total_tc_num = 0;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < nb_fwd_ports; i++) {
>> +		(void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
>> +		total_tc_num += dcb_info.nb_tcs;
>> +	}
>> +
>> +	return total_tc_num;
>> +}
> 
> It's only 3 lines. Is it really necessary to make it into a function which is not called by anyone else? A comment and the loop are enough.
> 
To calculate the total number of TCs on all forwarding ports, we have to 
call  rte_eth_dev_get_dcb_info().  However,  rte_eth_dev_get_dcb_info()
has been called twice by dcb_fwd_config_setup() to setup dcb forwarding 
configuration. The dcb_fwd_config_setup() might be more readable
if encapsulated, I think.  In addition, variables in "cur_fwd_config" 
are initialized more centrally. What do you think?
>> +
>>   /**
>>    * For the DCB forwarding test, each core is assigned on each traffic class.
>>    *
>> @@ -2883,12 +2898,16 @@ dcb_fwd_config_setup(void)
>>   	lcoreid_t  lc_id;
>>   	uint16_t nb_rx_queue, nb_tx_queue;
>>   	uint16_t i, j, k, sm_id = 0;
>> +	uint16_t total_tc_num = 0;
>>   	uint8_t tc = 0;
>>
>>   	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>>   	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>>   	cur_fwd_config.nb_fwd_streams =
>>   		(streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
>> +	total_tc_num = get_fwd_port_total_tc_num();
>> +	if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
>> +		cur_fwd_config.nb_fwd_lcores = total_tc_num;
>>
>>   	/* reinitialize forwarding streams */
>>   	init_fwd_streams();
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 1a57324..7eb810b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>   	portid_t peer_pl[RTE_MAX_ETHPORTS];
>>   	int peer_pi;
>>
>> -	if (dcb_test) {
>> -		dcb_test = 0;
>> -		dcb_config = 0;
>> -	}
>> -
>>   	if (port_id_is_invalid(pid, ENABLED_WARN))
>>   		return;
>>
>> +	/*
>> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>> +	 * So it should be cleared when dcb_config is 0.
>> +	 */
>> +	if (dcb_config == 0)
>> +		dcb_test = 0;
>> +
> 
> I don't understand why are you changing this.
> dcb_test will only be set when dcb_config is 1 when starting ports. And both dcb_test and dcb_config will be cleared when stopping ports.
> So dcb will only affect when you set port dcb and then start port and when stop port dcb will be cleared.
> 
> So what's the problem of original codes?
> 
> Your change will cause issues that there's no place to set dcb_config as 0. If you config dcb, then it'll be always dcb mode unless restart the whole testpmd.
> 
>>   	printf("Stopping ports...\n");
>>
>>   	RTE_ETH_FOREACH_DEV(pi) {
>> --
>> 2.7.4
> 
> .
>
Lijun Ou March 23, 2021, 2:18 p.m. UTC | #4
在 2021/3/23 16:55, 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 1/3] app/testpmd: fix forwarding configuration when DCB test
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
> The commit message is too long and redundant. Please simplify it.
> 
>> Currently, 'nb_fwd_lcores' value are both adjusted based on 'nb_fwd_streams' in
>> rss/simple/icmp_echo_fwd_config_setup. But the operation is lack for
>> dcb_fwd_config_setup, which may lead to a bad events where multiple polling
>> threads operate on the same queue. As a result, various possible exceptions will
>> occur. The procedure is as follows:
>>
>> startup testpmd app as follows:
>> testpmd> port stop all
>> testpmd> set nbcore 8
>> testpmd> port config 0 dcb vt off 4 pfc on port config all rxq 16 port
>> testpmd> config all txq 16 port start all set fwd mac
>> Logical Core 1 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 2 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 3 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 4 (socket 0) forwards packets on 4 streams:
>> RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00 RX
>> P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 5 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 6 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 7 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 Logical
>> Core 8 (socket 0) forwards packets on 1 streams:
>> RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00
>>
>> Notice: number of rxq and txq in running cmdline are greater than used nb_tc.
>>
>> For the DCB forwarding test, each core is assigned on each traffic class and each
>> core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
>> value can be adjusted based on 'total_tc_num' in all forwarding ports.
>>
>> In addition, after operation of port stop/port start, forwarding configuration is
>> changed by rss_fwd_config_setup. In "start_port"
>> function, dcb_test is set to 1 based on dcb_config. So it also should be cleared
>> when dcb_config is 0.
>>
>> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
>> 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>
>> ---
>>   app/test-pmd/config.c  | 19 +++++++++++++++++++  app/test-pmd/testpmd.c |
>> 12 +++++++-----
>>   2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>> 576d5ac..c89f8cd 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2864,6 +2864,21 @@ rss_fwd_config_setup(void)
>>   	}
>>   }
>>
>> +static uint16_t
>> +get_fwd_port_total_tc_num(void)
>> +{
>> +	struct rte_eth_dcb_info dcb_info;
>> +	uint16_t total_tc_num = 0;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < nb_fwd_ports; i++) {
>> +		(void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
>> +		total_tc_num += dcb_info.nb_tcs;
>> +	}
>> +
>> +	return total_tc_num;
>> +}
> 
> It's only 3 lines. Is it really necessary to make it into a function which is not called by anyone else? A comment and the loop are enough.
> 
>> +
>>   /**
>>    * For the DCB forwarding test, each core is assigned on each traffic class.
>>    *
>> @@ -2883,12 +2898,16 @@ dcb_fwd_config_setup(void)
>>   	lcoreid_t  lc_id;
>>   	uint16_t nb_rx_queue, nb_tx_queue;
>>   	uint16_t i, j, k, sm_id = 0;
>> +	uint16_t total_tc_num = 0;
>>   	uint8_t tc = 0;
>>
>>   	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>>   	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>>   	cur_fwd_config.nb_fwd_streams =
>>   		(streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
>> +	total_tc_num = get_fwd_port_total_tc_num();
>> +	if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
>> +		cur_fwd_config.nb_fwd_lcores = total_tc_num;
>>
>>   	/* reinitialize forwarding streams */
>>   	init_fwd_streams();
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 1a57324..7eb810b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>   	portid_t peer_pl[RTE_MAX_ETHPORTS];
>>   	int peer_pi;
>>
>> -	if (dcb_test) {
>> -		dcb_test = 0;
>> -		dcb_config = 0;
>> -	}
>> -
>>   	if (port_id_is_invalid(pid, ENABLED_WARN))
>>   		return;
>>
>> +	/*
>> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>> +	 * So it should be cleared when dcb_config is 0.
>> +	 */
>> +	if (dcb_config == 0)
>> +		dcb_test = 0;
>> +
> 
> I don't understand why are you changing this.
> dcb_test will only be set when dcb_config is 1 when starting ports. And both dcb_test and dcb_config will be cleared when stopping ports.
> So dcb will only affect when you set port dcb and then start port and when stop port dcb will be cleared.
> 
Yes, I think. The forwarding streams should not be changed from 
"dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is 
configured.
But, now, the logical codes do it when stopping ports and then starting 
ports.
> So what's the problem of original codes?
> 
> Your change will cause issues that there's no place to set dcb_config as 0. If you config dcb, then it'll be always dcb mode unless restart the whole testpmd.
> 
As far as I know, the current testpmd only supports switching from the 
normal mode to the dcb mode, but does not support the reverse operation.
And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after 
config.
>>   	printf("Stopping ports...\n");
>>
>>   	RTE_ETH_FOREACH_DEV(pi) {
>> --
>> 2.7.4
> 
> .
>
Li, Xiaoyun March 24, 2021, 1:51 a.m. UTC | #5
> -----Original Message-----
> From: oulijun <oulijun@huawei.com>
> Sent: Tuesday, March 23, 2021 22:18
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB
> test
> 
> 
> 
> 在 2021/3/23 16:55, 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 1/3] app/testpmd: fix forwarding configuration when
> >> DCB test
> >>
> >> From: Huisong Li <lihuisong@huawei.com>
> >>
> > The commit message is too long and redundant. Please simplify it.
> >
<snip>
> >> +static uint16_t
> >> +get_fwd_port_total_tc_num(void)
> >> +{
> >> +	struct rte_eth_dcb_info dcb_info;
> >> +	uint16_t total_tc_num = 0;
> >> +	unsigned int i;
> >> +
> >> +	for (i = 0; i < nb_fwd_ports; i++) {
> >> +		(void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
> >> +		total_tc_num += dcb_info.nb_tcs;
> >> +	}
> >> +
> >> +	return total_tc_num;
> >> +}
> >
> > It's only 3 lines. Is it really necessary to make it into a function which is not
> called by anyone else? A comment and the loop are enough.
> >
> To calculate the total number of TCs on all forwarding ports, we have to call
> rte_eth_dev_get_dcb_info().  However,  rte_eth_dev_get_dcb_info() has been
> called twice by dcb_fwd_config_setup() to setup dcb forwarding configuration.
> The dcb_fwd_config_setup() might be more readable if encapsulated, I think.  In
> addition, variables in "cur_fwd_config"
> are initialized more centrally. What do you think?

I'm not familiar with dcb. Why does the rxp_dcb_info get from fwd_ports_ids[0] and txp_dcb_info get from fwd_ports_ids[1]?
Is this a have-to-be? Can it be both from port 0 or last port?
If it can be port 0 or last port, you can just store the last dcb_info after your loop (loop from 0, i++ or loop from last i--).

> >> +
<snip>
> >> --
> >> 2.7.4
> >
> > .
> >
Li, Xiaoyun March 24, 2021, 2:03 a.m. UTC | #6
> -----Original Message-----
> From: oulijun <oulijun@huawei.com>
> Sent: Tuesday, March 23, 2021 22:19
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB
> test
> 
<snip>
> >> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
> >>   	portid_t peer_pl[RTE_MAX_ETHPORTS];
> >>   	int peer_pi;
> >>
> >> -	if (dcb_test) {
> >> -		dcb_test = 0;
> >> -		dcb_config = 0;
> >> -	}
> >> -
> >>   	if (port_id_is_invalid(pid, ENABLED_WARN))
> >>   		return;
> >>
> >> +	/*
> >> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
> >> +	 * So it should be cleared when dcb_config is 0.
> >> +	 */
> >> +	if (dcb_config == 0)
> >> +		dcb_test = 0;
> >> +
> >
> > I don't understand why are you changing this.
> > dcb_test will only be set when dcb_config is 1 when starting ports. And both
> dcb_test and dcb_config will be cleared when stopping ports.
> > So dcb will only affect when you set port dcb and then start port and when
> stop port dcb will be cleared.
> >
> Yes, I think. The forwarding streams should not be changed from
> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is configured.
> But, now, the logical codes do it when stopping ports and then starting ports.
> > So what's the problem of original codes?
> >
> > Your change will cause issues that there's no place to set dcb_config as 0. If
> you config dcb, then it'll be always dcb mode unless restart the whole testpmd.
> >
> As far as I know, the current testpmd only supports switching from the
> normal mode to the dcb mode, but does not support the reverse operation.
> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
> config.

You're not answering my questions. Why are you changing the behavior of testpmd?
Your change will make testpmd stay dcb mode once set dcb mode and can't go back to normal mode. If users want to go back to normal mode, he/she has to restart the whole testpmd.

It worked as you can set dcb mode and start port. After stopping port, if you still want dcb mode, you need to set dcb mode command again. But at least the old way won't break anything.
@Yigit, Ferruh Not sure which behavior is better, what do you think?

And @oulijun can you just answer all comments in one thread?

> >>   	printf("Stopping ports...\n");
> >>
> >>   	RTE_ETH_FOREACH_DEV(pi) {
> >> --
> >> 2.7.4
> >
> > .
> >
Lijun Ou March 24, 2021, 1:40 p.m. UTC | #7
在 2021/3/24 10:03, Li, Xiaoyun 写道:
> 
> 
>> -----Original Message-----
>> From: oulijun <oulijun@huawei.com>
>> Sent: Tuesday, March 23, 2021 22:19
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB
>> test
>>
> <snip>
>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>    	portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>    	int peer_pi;
>>>>
>>>> -	if (dcb_test) {
>>>> -		dcb_test = 0;
>>>> -		dcb_config = 0;
>>>> -	}
>>>> -
>>>>    	if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>    		return;
>>>>
>>>> +	/*
>>>> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>>>> +	 * So it should be cleared when dcb_config is 0.
>>>> +	 */
>>>> +	if (dcb_config == 0)
>>>> +		dcb_test = 0;
>>>> +
>>>
>>> I don't understand why are you changing this.
>>> dcb_test will only be set when dcb_config is 1 when starting ports. And both
>> dcb_test and dcb_config will be cleared when stopping ports.
>>> So dcb will only affect when you set port dcb and then start port and when
>> stop port dcb will be cleared.
>>>
>> Yes, I think. The forwarding streams should not be changed from
>> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is configured.
>> But, now, the logical codes do it when stopping ports and then starting ports.
>>> So what's the problem of original codes?
>>>
>>> Your change will cause issues that there's no place to set dcb_config as 0. If
>> you config dcb, then it'll be always dcb mode unless restart the whole testpmd.
>>>
>> As far as I know, the current testpmd only supports switching from the
>> normal mode to the dcb mode, but does not support the reverse operation.
>> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
>> config.
> 
> You're not answering my questions. Why are you changing the behavior of testpmd?
> Your change will make testpmd stay dcb mode once set dcb mode and can't go back to normal mode. If users want to go back to normal mode, he/she has to restart the whole testpmd.
> 
Yes. Testpmd and PMD driver are both in dcb mode after dcb info is 
configured. In my opinion, the 'dcb_test' flag can't be clear to go back 
to normal mode after
stopping port and then starting port. Because PMD driver is still dcb 
mode. If users want to go back it, users should disable dcb mode and 
enable RSS or other mode.

> It worked as you can set dcb mode and start port. After stopping port, if you still want dcb mode, you need to set dcb mode command again. But at least the old way won't break anything.
> @Yigit, Ferruh Not sure which behavior is better, what do you think?
> 
> And @oulijun can you just answer all comments in one thread?
> 
After stopping port, the 'dcb_test' flag is clear. At this moment, the 
dcb configuration in testpmd has not be changed.  users may not 
understand why the DCB mode needs to be reconfigured.
>>>>    	printf("Stopping ports...\n");
>>>>
>>>>    	RTE_ETH_FOREACH_DEV(pi) {
>>>> --
>>>> 2.7.4
>>>
>>> .
>>>
> _______________________________________________
> Linuxarm mailing list -- linuxarm@openeuler.org
> To unsubscribe send an email to linuxarm-leave@openeuler.org
>
Li, Xiaoyun March 25, 2021, 2:21 a.m. UTC | #8
> -----Original Message-----
> From: oulijun <oulijun@huawei.com>
> Sent: Wednesday, March 24, 2021 21:40
> To: linuxarm@openeuler.org; Li, Xiaoyun <xiaoyun.li@intel.com>; dev
> <dev@dpdk.org>
> Subject: Re: [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding
> configuration when DCB test
> 
> 
> 
> 在 2021/3/24 10:03, Li, Xiaoyun 写道:
> >
> >
> >> -----Original Message-----
> >> From: oulijun <oulijun@huawei.com>
> >> Sent: Tuesday, March 23, 2021 22:19
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; linuxarm@openeuler.org
> >> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration
> >> when DCB test
> >>
> > <snip>
> >>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
> >>>>    	portid_t peer_pl[RTE_MAX_ETHPORTS];
> >>>>    	int peer_pi;
> >>>>
> >>>> -	if (dcb_test) {
> >>>> -		dcb_test = 0;
> >>>> -		dcb_config = 0;
> >>>> -	}
> >>>> -
> >>>>    	if (port_id_is_invalid(pid, ENABLED_WARN))
> >>>>    		return;
> >>>>
> >>>> +	/*
> >>>> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
> >>>> +	 * So it should be cleared when dcb_config is 0.
> >>>> +	 */
> >>>> +	if (dcb_config == 0)
> >>>> +		dcb_test = 0;
> >>>> +
> >>>
> >>> I don't understand why are you changing this.
> >>> dcb_test will only be set when dcb_config is 1 when starting ports.
> >>> And both
> >> dcb_test and dcb_config will be cleared when stopping ports.
> >>> So dcb will only affect when you set port dcb and then start port
> >>> and when
> >> stop port dcb will be cleared.
> >>>
> >> Yes, I think. The forwarding streams should not be changed from
> >> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is
> configured.
> >> But, now, the logical codes do it when stopping ports and then starting ports.
> >>> So what's the problem of original codes?
> >>>
> >>> Your change will cause issues that there's no place to set
> >>> dcb_config as 0. If
> >> you config dcb, then it'll be always dcb mode unless restart the whole
> testpmd.
> >>>
> >> As far as I know, the current testpmd only supports switching from
> >> the normal mode to the dcb mode, but does not support the reverse
> operation.
> >> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
> >> config.
> >
> > You're not answering my questions. Why are you changing the behavior of
> testpmd?
> > Your change will make testpmd stay dcb mode once set dcb mode and can't go
> back to normal mode. If users want to go back to normal mode, he/she has to
> restart the whole testpmd.
> >
> Yes. Testpmd and PMD driver are both in dcb mode after dcb info is configured.
> In my opinion, the 'dcb_test' flag can't be clear to go back to normal mode after
> stopping port and then starting port. Because PMD driver is still dcb mode. If
> users want to go back it, users should disable dcb mode and enable RSS or other
> mode.
> 
> > It worked as you can set dcb mode and start port. After stopping port, if you
> still want dcb mode, you need to set dcb mode command again. But at least the
> old way won't break anything.
> > @Yigit, Ferruh Not sure which behavior is better, what do you think?
> >
> > And @oulijun can you just answer all comments in one thread?
> >
> After stopping port, the 'dcb_test' flag is clear. At this moment, the dcb
> configuration in testpmd has not be changed.  users may not understand why
> the DCB mode needs to be reconfigured.

OK. You're right. There's no place writing port->dcb_flag back to 0.

> >>>>    	printf("Stopping ports...\n");
> >>>>
> >>>>    	RTE_ETH_FOREACH_DEV(pi) {
> >>>> --
> >>>> 2.7.4
> >>>
> >>> .
> >>>
> > _______________________________________________
> > Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an
> > email to linuxarm-leave@openeuler.org
> >
Lijun Ou March 27, 2021, 3:53 a.m. UTC | #9
在 2021/3/25 10:21, Li, Xiaoyun 写道:
> 
> 
>> -----Original Message-----
>> From: oulijun <oulijun@huawei.com>
>> Sent: Wednesday, March 24, 2021 21:40
>> To: linuxarm@openeuler.org; Li, Xiaoyun <xiaoyun.li@intel.com>; dev
>> <dev@dpdk.org>
>> Subject: Re: [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding
>> configuration when DCB test
>>
>>
>>
>> 在 2021/3/24 10:03, Li, Xiaoyun 写道:
>>>
>>>
>>>> -----Original Message-----
>>>> From: oulijun <oulijun@huawei.com>
>>>> Sent: Tuesday, March 23, 2021 22:19
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>> <ferruh.yigit@intel.com>
>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>>>> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration
>>>> when DCB test
>>>>
>>> <snip>
>>>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>>>     	portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>>>     	int peer_pi;
>>>>>>
>>>>>> -	if (dcb_test) {
>>>>>> -		dcb_test = 0;
>>>>>> -		dcb_config = 0;
>>>>>> -	}
>>>>>> -
>>>>>>     	if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>>>     		return;
>>>>>>
>>>>>> +	/*
>>>>>> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>>>>>> +	 * So it should be cleared when dcb_config is 0.
>>>>>> +	 */
>>>>>> +	if (dcb_config == 0)
>>>>>> +		dcb_test = 0;
>>>>>> +
>>>>>
>>>>> I don't understand why are you changing this.
>>>>> dcb_test will only be set when dcb_config is 1 when starting ports.
>>>>> And both
>>>> dcb_test and dcb_config will be cleared when stopping ports.
>>>>> So dcb will only affect when you set port dcb and then start port
>>>>> and when
>>>> stop port dcb will be cleared.
>>>>>
>>>> Yes, I think. The forwarding streams should not be changed from
>>>> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is
>> configured.
>>>> But, now, the logical codes do it when stopping ports and then starting ports.
>>>>> So what's the problem of original codes?
>>>>>
>>>>> Your change will cause issues that there's no place to set
>>>>> dcb_config as 0. If
>>>> you config dcb, then it'll be always dcb mode unless restart the whole
>> testpmd.
>>>>>
>>>> As far as I know, the current testpmd only supports switching from
>>>> the normal mode to the dcb mode, but does not support the reverse
>> operation.
>>>> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
>>>> config.
>>>
>>> You're not answering my questions. Why are you changing the behavior of
>> testpmd?
>>> Your change will make testpmd stay dcb mode once set dcb mode and can't go
>> back to normal mode. If users want to go back to normal mode, he/she has to
>> restart the whole testpmd.
>>>
>> Yes. Testpmd and PMD driver are both in dcb mode after dcb info is configured.
>> In my opinion, the 'dcb_test' flag can't be clear to go back to normal mode after
>> stopping port and then starting port. Because PMD driver is still dcb mode. If
>> users want to go back it, users should disable dcb mode and enable RSS or other
>> mode.
>>
>>> It worked as you can set dcb mode and start port. After stopping port, if you
>> still want dcb mode, you need to set dcb mode command again. But at least the
>> old way won't break anything.
>>> @Yigit, Ferruh Not sure which behavior is better, what do you think?
>>>
>>> And @oulijun can you just answer all comments in one thread?
>>>
>> After stopping port, the 'dcb_test' flag is clear. At this moment, the dcb
>> configuration in testpmd has not be changed.  users may not understand why
>> the DCB mode needs to be reconfigured.
> 
> OK. You're right. There's no place writing port->dcb_flag back to 0.
> 
Yes. Unless testpmd supports switching from DCB mode to other modes.
Do you have any other suggestions about this patch set, xiaoyun?
>>>>>>     	printf("Stopping ports...\n");
>>>>>>
>>>>>>     	RTE_ETH_FOREACH_DEV(pi) {
>>>>>> --
>>>>>> 2.7.4
>>>>>
>>>>> .
>>>>>
>>> _______________________________________________
>>> Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an
>>> email to linuxarm-leave@openeuler.org
>>>
Ferruh Yigit April 10, 2021, 12:56 a.m. UTC | #10
On 3/25/2021 2:21 AM, Li, Xiaoyun wrote:
> 
> 
>> -----Original Message-----
>> From: oulijun <oulijun@huawei.com>
>> Sent: Wednesday, March 24, 2021 21:40
>> To: linuxarm@openeuler.org; Li, Xiaoyun <xiaoyun.li@intel.com>; dev
>> <dev@dpdk.org>
>> Subject: Re: [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding
>> configuration when DCB test
>>
>>
>>
>> 在 2021/3/24 10:03, Li, Xiaoyun 写道:
>>>
>>>
>>>> -----Original Message-----
>>>> From: oulijun <oulijun@huawei.com>
>>>> Sent: Tuesday, March 23, 2021 22:19
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>> <ferruh.yigit@intel.com>
>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>>>> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration
>>>> when DCB test
>>>>
>>> <snip>
>>>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>>>     	portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>>>     	int peer_pi;
>>>>>>
>>>>>> -	if (dcb_test) {
>>>>>> -		dcb_test = 0;
>>>>>> -		dcb_config = 0;
>>>>>> -	}
>>>>>> -
>>>>>>     	if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>>>     		return;
>>>>>>
>>>>>> +	/*
>>>>>> +	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>>>>>> +	 * So it should be cleared when dcb_config is 0.
>>>>>> +	 */
>>>>>> +	if (dcb_config == 0)
>>>>>> +		dcb_test = 0;
>>>>>> +
>>>>>
>>>>> I don't understand why are you changing this.
>>>>> dcb_test will only be set when dcb_config is 1 when starting ports.
>>>>> And both
>>>> dcb_test and dcb_config will be cleared when stopping ports.
>>>>> So dcb will only affect when you set port dcb and then start port
>>>>> and when
>>>> stop port dcb will be cleared.
>>>>>
>>>> Yes, I think. The forwarding streams should not be changed from
>>>> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is
>> configured.
>>>> But, now, the logical codes do it when stopping ports and then starting ports.
>>>>> So what's the problem of original codes?
>>>>>
>>>>> Your change will cause issues that there's no place to set
>>>>> dcb_config as 0. If
>>>> you config dcb, then it'll be always dcb mode unless restart the whole
>> testpmd.
>>>>>
>>>> As far as I know, the current testpmd only supports switching from
>>>> the normal mode to the dcb mode, but does not support the reverse
>> operation.
>>>> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
>>>> config.
>>>
>>> You're not answering my questions. Why are you changing the behavior of
>> testpmd?
>>> Your change will make testpmd stay dcb mode once set dcb mode and can't go
>> back to normal mode. If users want to go back to normal mode, he/she has to
>> restart the whole testpmd.
>>>
>> Yes. Testpmd and PMD driver are both in dcb mode after dcb info is configured.
>> In my opinion, the 'dcb_test' flag can't be clear to go back to normal mode after
>> stopping port and then starting port. Because PMD driver is still dcb mode. If
>> users want to go back it, users should disable dcb mode and enable RSS or other
>> mode.
>>
>>> It worked as you can set dcb mode and start port. After stopping port, if you
>> still want dcb mode, you need to set dcb mode command again. But at least the
>> old way won't break anything.
>>> @Yigit, Ferruh Not sure which behavior is better, what do you think?
>>>
>>> And @oulijun can you just answer all comments in one thread?
>>>
>> After stopping port, the 'dcb_test' flag is clear. At this moment, the dcb
>> configuration in testpmd has not be changed.  users may not understand why
>> the DCB mode needs to be reconfigured.
> 
> OK. You're right. There's no place writing port->dcb_flag back to 0.
> 

If there is no way to turn off the DCB, isn't this code logically dead? This 
code is run when "dcb_config == 0" but in that case 'dcb_test' is already 0.
If so what is the point of the code, can it be removed?

btw, do you know what 'dcb_test' does at all? It seems it is set when 
'dcb_config' set, and reset when 'dcb_config' reset.

>>>>>>     	printf("Stopping ports...\n");
>>>>>>
>>>>>>     	RTE_ETH_FOREACH_DEV(pi) {
>>>>>> --
>>>>>> 2.7.4
>>>>>
>>>>> .
>>>>>
>>> _______________________________________________
>>> Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an
>>> email to linuxarm-leave@openeuler.org
>>>
Ferruh Yigit April 10, 2021, 1:12 a.m. UTC | #11
On 3/23/2021 2:17 PM, oulijun wrote:
> 
> 
> 在 2021/3/23 16:55, 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 1/3] app/testpmd: fix forwarding configuration when DCB test
>>>
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>> The commit message is too long and redundant. Please simplify it.
>>
>>> Currently, 'nb_fwd_lcores' value are both adjusted based on 'nb_fwd_streams' in
>>> rss/simple/icmp_echo_fwd_config_setup. But the operation is lack for
>>> dcb_fwd_config_setup, which may lead to a bad events where multiple polling
>>> threads operate on the same queue. As a result, various possible exceptions will
>>> occur. The procedure is as follows:
>>>
>>> startup testpmd app as follows:
>>> testpmd> port stop all
>>> testpmd> set nbcore 8
>>> testpmd> port config 0 dcb vt off 4 pfc on port config all rxq 16 port
>>> testpmd> config all txq 16 port start all set fwd mac
>>> Logical Core 1 (socket 0) forwards packets on 4 streams:
>>> RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00 Logical
>>> Core 2 (socket 0) forwards packets on 4 streams:
>>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00 Logical
>>> Core 3 (socket 0) forwards packets on 4 streams:
>>> RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00 Logical
>>> Core 4 (socket 0) forwards packets on 4 streams:
>>> RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00 RX
>>> P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00 Logical
>>> Core 5 (socket 0) forwards packets on 1 streams:
>>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) peer=02:00:00:00:00:00 Logical
>>> Core 6 (socket 0) forwards packets on 1 streams:
>>> RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 Logical
>>> Core 7 (socket 0) forwards packets on 1 streams:
>>> RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 Logical
>>> Core 8 (socket 0) forwards packets on 1 streams:
>>> RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00
>>>
>>> Notice: number of rxq and txq in running cmdline are greater than used nb_tc.
>>>
>>> For the DCB forwarding test, each core is assigned on each traffic class and 
>>> each
>>> core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
>>> value can be adjusted based on 'total_tc_num' in all forwarding ports.
>>>
>>> In addition, after operation of port stop/port start, forwarding 
>>> configuration is
>>> changed by rss_fwd_config_setup. In "start_port"
>>> function, dcb_test is set to 1 based on dcb_config. So it also should be cleared
>>> when dcb_config is 0.
>>>
>>> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
>>> 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>
>>> ---
>>>   app/test-pmd/config.c  | 19 +++++++++++++++++++  app/test-pmd/testpmd.c |
>>> 12 +++++++-----
>>>   2 files changed, 26 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>> 576d5ac..c89f8cd 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -2864,6 +2864,21 @@ rss_fwd_config_setup(void)
>>>       }
>>>   }
>>>
>>> +static uint16_t
>>> +get_fwd_port_total_tc_num(void)
>>> +{
>>> +    struct rte_eth_dcb_info dcb_info;
>>> +    uint16_t total_tc_num = 0;
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < nb_fwd_ports; i++) {
>>> +        (void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
>>> +        total_tc_num += dcb_info.nb_tcs;
>>> +    }
>>> +
>>> +    return total_tc_num;
>>> +}
>>
>> It's only 3 lines. Is it really necessary to make it into a function which is 
>> not called by anyone else? A comment and the loop are enough.
>>
> To calculate the total number of TCs on all forwarding ports, we have to call  
> rte_eth_dev_get_dcb_info().  However,  rte_eth_dev_get_dcb_info()
> has been called twice by dcb_fwd_config_setup() to setup dcb forwarding 
> configuration. The dcb_fwd_config_setup() might be more readable
> if encapsulated, I think.  In addition, variables in "cur_fwd_config" are 
> initialized more centrally. What do you think?

+1 to have the function, agree it makes more readable.

But for the PMDs that doesn't support '.get_dcb_info' it will return 0, causing 
'nb_fwd_lcores' to be zero, I think this should be prevented.
Either can add a zero check here, or do the check during "port config # dcb ..." 
and prevent setting 'dcb_config' at first place for those pmds. I think second 
option is better, what do you think.

Overall DCB support in the testpmd seems missing some corner cases and requires 
more update/fix.

>>> +
>>>   /**
>>>    * For the DCB forwarding test, each core is assigned on each traffic class.
>>>    *
>>> @@ -2883,12 +2898,16 @@ dcb_fwd_config_setup(void)
>>>       lcoreid_t  lc_id;
>>>       uint16_t nb_rx_queue, nb_tx_queue;
>>>       uint16_t i, j, k, sm_id = 0;
>>> +    uint16_t total_tc_num = 0;
>>>       uint8_t tc = 0;
>>>
>>>       cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>>>       cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>>>       cur_fwd_config.nb_fwd_streams =
>>>           (streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
>>> +    total_tc_num = get_fwd_port_total_tc_num();
>>> +    if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
>>> +        cur_fwd_config.nb_fwd_lcores = total_tc_num;
>>>
>>>       /* reinitialize forwarding streams */
>>>       init_fwd_streams();
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 1a57324..7eb810b 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>       portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>       int peer_pi;
>>>
>>> -    if (dcb_test) {
>>> -        dcb_test = 0;
>>> -        dcb_config = 0;
>>> -    }
>>> -
>>>       if (port_id_is_invalid(pid, ENABLED_WARN))
>>>           return;
>>>
>>> +    /*
>>> +     * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>>> +     * So it should be cleared when dcb_config is 0.
>>> +     */
>>> +    if (dcb_config == 0)
>>> +        dcb_test = 0;
>>> +
>>
>> I don't understand why are you changing this.
>> dcb_test will only be set when dcb_config is 1 when starting ports. And both 
>> dcb_test and dcb_config will be cleared when stopping ports.
>> So dcb will only affect when you set port dcb and then start port and when 
>> stop port dcb will be cleared.
>>
>> So what's the problem of original codes?
>>
>> Your change will cause issues that there's no place to set dcb_config as 0. If 
>> you config dcb, then it'll be always dcb mode unless restart the whole testpmd.
>>
>>>       printf("Stopping ports...\n");
>>>
>>>       RTE_ETH_FOREACH_DEV(pi) {
>>> -- 
>>> 2.7.4
>>
>> .
>>
Lijun Ou April 10, 2021, 2:53 a.m. UTC | #12
在 2021/4/10 9:12, Ferruh Yigit 写道:
> On 3/23/2021 2:17 PM, oulijun wrote:
>>
>>
>> 在 2021/3/23 16:55, 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 1/3] app/testpmd: fix forwarding configuration when 
>>>> DCB test
>>>>
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>> The commit message is too long and redundant. Please simplify it.
>>>
>>>> Currently, 'nb_fwd_lcores' value are both adjusted based on 
>>>> 'nb_fwd_streams' in
>>>> rss/simple/icmp_echo_fwd_config_setup. But the operation is lack for
>>>> dcb_fwd_config_setup, which may lead to a bad events where multiple 
>>>> polling
>>>> threads operate on the same queue. As a result, various possible 
>>>> exceptions will
>>>> occur. The procedure is as follows:
>>>>
>>>> startup testpmd app as follows:
>>>> testpmd> port stop all
>>>> testpmd> set nbcore 8
>>>> testpmd> port config 0 dcb vt off 4 pfc on port config all rxq 16 port
>>>> testpmd> config all txq 16 port start all set fwd mac
>>>> Logical Core 1 (socket 0) forwards packets on 4 streams:
>>>> RX P=0/Q=0 (socket 0) -> TX P=0/Q=0 (socket 0) 
>>>> peer=02:00:00:00:00:00 RX
>>>> P=0/Q=1 (socket 0) -> TX P=0/Q=1 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=2 (socket 0) -> TX P=0/Q=2 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=3 (socket 0) -> TX P=0/Q=3 (socket 0) peer=02:00:00:00:00:00 
>>>> Logical
>>>> Core 2 (socket 0) forwards packets on 4 streams:
>>>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) 
>>>> peer=02:00:00:00:00:00 RX
>>>> P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00 
>>>> Logical
>>>> Core 3 (socket 0) forwards packets on 4 streams:
>>>> RX P=0/Q=8 (socket 0) -> TX P=0/Q=8 (socket 0) 
>>>> peer=02:00:00:00:00:00 RX
>>>> P=0/Q=9 (socket 0) -> TX P=0/Q=9 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=10 (socket 0) -> TX P=0/Q=10 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=11 (socket 0) -> TX P=0/Q=11 (socket 0) peer=02:00:00:00:00:00 
>>>> Logical
>>>> Core 4 (socket 0) forwards packets on 4 streams:
>>>> RX P=0/Q=12 (socket 0) -> TX P=0/Q=12 (socket 0) 
>>>> peer=02:00:00:00:00:00 RX
>>>> P=0/Q=13 (socket 0) -> TX P=0/Q=13 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=14 (socket 0) -> TX P=0/Q=14 (socket 0) peer=02:00:00:00:00:00 RX
>>>> P=0/Q=15 (socket 0) -> TX P=0/Q=15 (socket 0) peer=02:00:00:00:00:00 
>>>> Logical
>>>> Core 5 (socket 0) forwards packets on 1 streams:
>>>> RX P=0/Q=4 (socket 0) -> TX P=0/Q=4 (socket 0) 
>>>> peer=02:00:00:00:00:00 Logical
>>>> Core 6 (socket 0) forwards packets on 1 streams:
>>>> RX P=0/Q=5 (socket 0) -> TX P=0/Q=5 (socket 0) 
>>>> peer=02:00:00:00:00:00 Logical
>>>> Core 7 (socket 0) forwards packets on 1 streams:
>>>> RX P=0/Q=6 (socket 0) -> TX P=0/Q=6 (socket 0) 
>>>> peer=02:00:00:00:00:00 Logical
>>>> Core 8 (socket 0) forwards packets on 1 streams:
>>>> RX P=0/Q=7 (socket 0) -> TX P=0/Q=7 (socket 0) peer=02:00:00:00:00:00
>>>>
>>>> Notice: number of rxq and txq in running cmdline are greater than 
>>>> used nb_tc.
>>>>
>>>> For the DCB forwarding test, each core is assigned on each traffic 
>>>> class and each
>>>> core is assigned a multi-stream. Therefore, 'nb_fwd_lcores'
>>>> value can be adjusted based on 'total_tc_num' in all forwarding ports.
>>>>
>>>> In addition, after operation of port stop/port start, forwarding 
>>>> configuration is
>>>> changed by rss_fwd_config_setup. In "start_port"
>>>> function, dcb_test is set to 1 based on dcb_config. So it also 
>>>> should be cleared
>>>> when dcb_config is 0.
>>>>
>>>> Fixes: 900550de04a7 ("app/testpmd: add dcb support")
>>>> 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>
>>>> ---
>>>>   app/test-pmd/config.c  | 19 +++++++++++++++++++  
>>>> app/test-pmd/testpmd.c |
>>>> 12 +++++++-----
>>>>   2 files changed, 26 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>> 576d5ac..c89f8cd 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -2864,6 +2864,21 @@ rss_fwd_config_setup(void)
>>>>       }
>>>>   }
>>>>
>>>> +static uint16_t
>>>> +get_fwd_port_total_tc_num(void)
>>>> +{
>>>> +    struct rte_eth_dcb_info dcb_info;
>>>> +    uint16_t total_tc_num = 0;
>>>> +    unsigned int i;
>>>> +
>>>> +    for (i = 0; i < nb_fwd_ports; i++) {
>>>> +        (void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
>>>> +        total_tc_num += dcb_info.nb_tcs;
>>>> +    }
>>>> +
>>>> +    return total_tc_num;
>>>> +}
>>>
>>> It's only 3 lines. Is it really necessary to make it into a function 
>>> which is not called by anyone else? A comment and the loop are enough.
>>>
>> To calculate the total number of TCs on all forwarding ports, we have 
>> to call rte_eth_dev_get_dcb_info().  However,  rte_eth_dev_get_dcb_info()
>> has been called twice by dcb_fwd_config_setup() to setup dcb 
>> forwarding configuration. The dcb_fwd_config_setup() might be more 
>> readable
>> if encapsulated, I think.  In addition, variables in "cur_fwd_config" 
>> are initialized more centrally. What do you think?
> 
> +1 to have the function, agree it makes more readable.
> 
> But for the PMDs that doesn't support '.get_dcb_info' it will return 0, 
> causing 'nb_fwd_lcores' to be zero, I think this should be prevented.
> Either can add a zero check here, or do the check during "port config # 
> dcb ..." and prevent setting 'dcb_config' at first place for those pmds. 
> I think second option is better, what do you think.
> 
Yes, the second one is the source.
> Overall DCB support in the testpmd seems missing some corner cases and 
> requires more update/fix.
> 
In testpmd, it seems that the code has not been updated for a long time. 
I don't know how other dcb-enabled drivers are tested and how they 
should be used.
Do they not apply this method to test the DCB?
>>>> +
>>>>   /**
>>>>    * For the DCB forwarding test, each core is assigned on each 
>>>> traffic class.
>>>>    *
>>>> @@ -2883,12 +2898,16 @@ dcb_fwd_config_setup(void)
>>>>       lcoreid_t  lc_id;
>>>>       uint16_t nb_rx_queue, nb_tx_queue;
>>>>       uint16_t i, j, k, sm_id = 0;
>>>> +    uint16_t total_tc_num = 0;
>>>>       uint8_t tc = 0;
>>>>
>>>>       cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
>>>>       cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
>>>>       cur_fwd_config.nb_fwd_streams =
>>>>           (streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
>>>> +    total_tc_num = get_fwd_port_total_tc_num();
>>>> +    if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
>>>> +        cur_fwd_config.nb_fwd_lcores = total_tc_num;
>>>>
>>>>       /* reinitialize forwarding streams */
>>>>       init_fwd_streams();
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>> 1a57324..7eb810b 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>       portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>       int peer_pi;
>>>>
>>>> -    if (dcb_test) {
>>>> -        dcb_test = 0;
>>>> -        dcb_config = 0;
>>>> -    }
>>>> -
>>>>       if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>           return;
>>>>
>>>> +    /*
>>>> +     * In "start_port" function, dcb_test is set to 1 based on 
>>>> dcb_config.
>>>> +     * So it should be cleared when dcb_config is 0.
>>>> +     */
>>>> +    if (dcb_config == 0)
>>>> +        dcb_test = 0;
>>>> +
>>>
>>> I don't understand why are you changing this.
>>> dcb_test will only be set when dcb_config is 1 when starting ports. 
>>> And both dcb_test and dcb_config will be cleared when stopping ports.
>>> So dcb will only affect when you set port dcb and then start port and 
>>> when stop port dcb will be cleared.
>>>
>>> So what's the problem of original codes?
>>>
>>> Your change will cause issues that there's no place to set dcb_config 
>>> as 0. If you config dcb, then it'll be always dcb mode unless restart 
>>> the whole testpmd.
>>>
>>>>       printf("Stopping ports...\n");
>>>>
>>>>       RTE_ETH_FOREACH_DEV(pi) {
>>>> -- 
>>>> 2.7.4
>>>
>>> .
>>>
> 
> .
>
Lijun Ou April 10, 2021, 2:58 a.m. UTC | #13
在 2021/4/10 8:56, Ferruh Yigit 写道:
> On 3/25/2021 2:21 AM, Li, Xiaoyun wrote:
>>
>>
>>> -----Original Message-----
>>> From: oulijun <oulijun@huawei.com>
>>> Sent: Wednesday, March 24, 2021 21:40
>>> To: linuxarm@openeuler.org; Li, Xiaoyun <xiaoyun.li@intel.com>; dev
>>> <dev@dpdk.org>
>>> Subject: Re: [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding
>>> configuration when DCB test
>>>
>>>
>>>
>>> 在 2021/3/24 10:03, Li, Xiaoyun 写道:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: oulijun <oulijun@huawei.com>
>>>>> Sent: Tuesday, March 23, 2021 22:19
>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>>> <ferruh.yigit@intel.com>
>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>>>>> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration
>>>>> when DCB test
>>>>>
>>>> <snip>
>>>>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>>>>         portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>>>>         int peer_pi;
>>>>>>>
>>>>>>> -    if (dcb_test) {
>>>>>>> -        dcb_test = 0;
>>>>>>> -        dcb_config = 0;
>>>>>>> -    }
>>>>>>> -
>>>>>>>         if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>>>>             return;
>>>>>>>
>>>>>>> +    /*
>>>>>>> +     * In "start_port" function, dcb_test is set to 1 based on 
>>>>>>> dcb_config.
>>>>>>> +     * So it should be cleared when dcb_config is 0.
>>>>>>> +     */
>>>>>>> +    if (dcb_config == 0)
>>>>>>> +        dcb_test = 0;
>>>>>>> +
>>>>>>
>>>>>> I don't understand why are you changing this.
>>>>>> dcb_test will only be set when dcb_config is 1 when starting ports.
>>>>>> And both
>>>>> dcb_test and dcb_config will be cleared when stopping ports.
>>>>>> So dcb will only affect when you set port dcb and then start port
>>>>>> and when
>>>>> stop port dcb will be cleared.
>>>>>>
>>>>> Yes, I think. The forwarding streams should not be changed from
>>>>> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is
>>> configured.
>>>>> But, now, the logical codes do it when stopping ports and then 
>>>>> starting ports.
>>>>>> So what's the problem of original codes?
>>>>>>
>>>>>> Your change will cause issues that there's no place to set
>>>>>> dcb_config as 0. If
>>>>> you config dcb, then it'll be always dcb mode unless restart the 
>>>>> whole
>>> testpmd.
>>>>>>
>>>>> As far as I know, the current testpmd only supports switching from
>>>>> the normal mode to the dcb mode, but does not support the reverse
>>> operation.
>>>>> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
>>>>> config.
>>>>
>>>> You're not answering my questions. Why are you changing the 
>>>> behavior of
>>> testpmd?
>>>> Your change will make testpmd stay dcb mode once set dcb mode and 
>>>> can't go
>>> back to normal mode. If users want to go back to normal mode, he/she 
>>> has to
>>> restart the whole testpmd.
>>>>
>>> Yes. Testpmd and PMD driver are both in dcb mode after dcb info is 
>>> configured.
>>> In my opinion, the 'dcb_test' flag can't be clear to go back to 
>>> normal mode after
>>> stopping port and then starting port. Because PMD driver is still 
>>> dcb mode. If
>>> users want to go back it, users should disable dcb mode and enable 
>>> RSS or other
>>> mode.
>>>
>>>> It worked as you can set dcb mode and start port. After stopping 
>>>> port, if you
>>> still want dcb mode, you need to set dcb mode command again. But at 
>>> least the
>>> old way won't break anything.
>>>> @Yigit, Ferruh Not sure which behavior is better, what do you think?
>>>>
>>>> And @oulijun can you just answer all comments in one thread?
>>>>
>>> After stopping port, the 'dcb_test' flag is clear. At this moment, 
>>> the dcb
>>> configuration in testpmd has not be changed.  users may not 
>>> understand why
>>> the DCB mode needs to be reconfigured.
>>
>> OK. You're right. There's no place writing port->dcb_flag back to 0.
>>
>
> If there is no way to turn off the DCB, isn't this code logically 
> dead? This code is run when "dcb_config == 0" but in that case 
> 'dcb_test' is already 0.
> If so what is the point of the code, can it be removed?
>
yes. Currently, it is a dead logic unless testpmd adds support for 
switching from DCB mode to other modes.
I think dcb_test is a flag at the packet forwarding layer in testpmd. 
The original purpose is to indicate that the current forwarding test is 
being performed in DCB mode.
> btw, do you know what 'dcb_test' does at all? It seems it is set when 
> 'dcb_config' set, and reset when 'dcb_config' reset.
>
In my opinion, it is only a global flag. During a DCB test, DCB must be 
configured on all forwarding ports.
>>>>>>>         printf("Stopping ports...\n");
>>>>>>>
>>>>>>>         RTE_ETH_FOREACH_DEV(pi) {
>>>>>>> -- 
>>>>>>> 2.7.4
>>>>>>
>>>>>> .
>>>>>>
>>>> _______________________________________________
>>>> Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an
>>>> email to linuxarm-leave@openeuler.org
>>>>
>
> .
>
Ferruh Yigit April 12, 2021, 7:52 a.m. UTC | #14
On 4/10/2021 3:58 AM, oulijun wrote:
> 
> 
> 在 2021/4/10 8:56, Ferruh Yigit 写道:
>> On 3/25/2021 2:21 AM, Li, Xiaoyun wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: oulijun <oulijun@huawei.com>
>>>> Sent: Wednesday, March 24, 2021 21:40
>>>> To: linuxarm@openeuler.org; Li, Xiaoyun <xiaoyun.li@intel.com>; dev
>>>> <dev@dpdk.org>
>>>> Subject: Re: [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding
>>>> configuration when DCB test
>>>>
>>>>
>>>>
>>>> 在 2021/3/24 10:03, Li, Xiaoyun 写道:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: oulijun <oulijun@huawei.com>
>>>>>> Sent: Tuesday, March 23, 2021 22:19
>>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>>>> <ferruh.yigit@intel.com>
>>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>>>>>> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration
>>>>>> when DCB test
>>>>>>
>>>>> <snip>
>>>>>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>>>>>         portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>>>>>         int peer_pi;
>>>>>>>>
>>>>>>>> -    if (dcb_test) {
>>>>>>>> -        dcb_test = 0;
>>>>>>>> -        dcb_config = 0;
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>>         if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>>>>>             return;
>>>>>>>>
>>>>>>>> +    /*
>>>>>>>> +     * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>>>>>>>> +     * So it should be cleared when dcb_config is 0.
>>>>>>>> +     */
>>>>>>>> +    if (dcb_config == 0)
>>>>>>>> +        dcb_test = 0;
>>>>>>>> +
>>>>>>>
>>>>>>> I don't understand why are you changing this.
>>>>>>> dcb_test will only be set when dcb_config is 1 when starting ports.
>>>>>>> And both
>>>>>> dcb_test and dcb_config will be cleared when stopping ports.
>>>>>>> So dcb will only affect when you set port dcb and then start port
>>>>>>> and when
>>>>>> stop port dcb will be cleared.
>>>>>>>
>>>>>> Yes, I think. The forwarding streams should not be changed from
>>>>>> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is
>>>> configured.
>>>>>> But, now, the logical codes do it when stopping ports and then starting 
>>>>>> ports.
>>>>>>> So what's the problem of original codes?
>>>>>>>
>>>>>>> Your change will cause issues that there's no place to set
>>>>>>> dcb_config as 0. If
>>>>>> you config dcb, then it'll be always dcb mode unless restart the whole
>>>> testpmd.
>>>>>>>
>>>>>> As far as I know, the current testpmd only supports switching from
>>>>>> the normal mode to the dcb mode, but does not support the reverse
>>>> operation.
>>>>>> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
>>>>>> config.
>>>>>
>>>>> You're not answering my questions. Why are you changing the behavior of
>>>> testpmd?
>>>>> Your change will make testpmd stay dcb mode once set dcb mode and can't go
>>>> back to normal mode. If users want to go back to normal mode, he/she has to
>>>> restart the whole testpmd.
>>>>>
>>>> Yes. Testpmd and PMD driver are both in dcb mode after dcb info is configured.
>>>> In my opinion, the 'dcb_test' flag can't be clear to go back to normal mode 
>>>> after
>>>> stopping port and then starting port. Because PMD driver is still dcb mode. If
>>>> users want to go back it, users should disable dcb mode and enable RSS or other
>>>> mode.
>>>>
>>>>> It worked as you can set dcb mode and start port. After stopping port, if you
>>>> still want dcb mode, you need to set dcb mode command again. But at least the
>>>> old way won't break anything.
>>>>> @Yigit, Ferruh Not sure which behavior is better, what do you think?
>>>>>
>>>>> And @oulijun can you just answer all comments in one thread?
>>>>>
>>>> After stopping port, the 'dcb_test' flag is clear. At this moment, the dcb
>>>> configuration in testpmd has not be changed.  users may not understand why
>>>> the DCB mode needs to be reconfigured.
>>>
>>> OK. You're right. There's no place writing port->dcb_flag back to 0.
>>>
>>
>> If there is no way to turn off the DCB, isn't this code logically dead? This 
>> code is run when "dcb_config == 0" but in that case 'dcb_test' is already 0.
>> If so what is the point of the code, can it be removed?
>>
> yes. Currently, it is a dead logic unless testpmd adds support for switching 
> from DCB mode to other modes.
> I think dcb_test is a flag at the packet forwarding layer in testpmd. The 
> original purpose is to indicate that the current forwarding test is being 
> performed in DCB mode.

can 'dcb_config' be sufficient on its own for this? so that we can remove 
'dcb_test' and simplify the logic.

>> btw, do you know what 'dcb_test' does at all? It seems it is set when 
>> 'dcb_config' set, and reset when 'dcb_config' reset.
>>
> In my opinion, it is only a global flag. During a DCB test, DCB must be 
> configured on all forwarding ports.
>>>>>>>>         printf("Stopping ports...\n");
>>>>>>>>
>>>>>>>>         RTE_ETH_FOREACH_DEV(pi) {
>>>>>>>> -- 
>>>>>>>> 2.7.4
>>>>>>>
>>>>>>> .
>>>>>>>
>>>>> _______________________________________________
>>>>> Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an
>>>>> email to linuxarm-leave@openeuler.org
>>>>>
>>
>> .
>>
>
Lijun Ou April 12, 2021, 12:20 p.m. UTC | #15
在 2021/4/12 15:52, Ferruh Yigit 写道:
> On 4/10/2021 3:58 AM, oulijun wrote:
>>
>>
>> 在 2021/4/10 8:56, Ferruh Yigit 写道:
>>> On 3/25/2021 2:21 AM, Li, Xiaoyun wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: oulijun <oulijun@huawei.com>
>>>>> Sent: Wednesday, March 24, 2021 21:40
>>>>> To: linuxarm@openeuler.org; Li, Xiaoyun <xiaoyun.li@intel.com>; dev
>>>>> <dev@dpdk.org>
>>>>> Subject: Re: [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding
>>>>> configuration when DCB test
>>>>>
>>>>>
>>>>>
>>>>> 在 2021/3/24 10:03, Li, Xiaoyun 写道:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: oulijun <oulijun@huawei.com>
>>>>>>> Sent: Tuesday, March 23, 2021 22:19
>>>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>>>>> <ferruh.yigit@intel.com>
>>>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>>>>>>> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration
>>>>>>> when DCB test
>>>>>>>
>>>>>> <snip>
>>>>>>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>>>>>>         portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>>>>>>         int peer_pi;
>>>>>>>>>
>>>>>>>>> -    if (dcb_test) {
>>>>>>>>> -        dcb_test = 0;
>>>>>>>>> -        dcb_config = 0;
>>>>>>>>> -    }
>>>>>>>>> -
>>>>>>>>>         if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>>>>>>             return;
>>>>>>>>>
>>>>>>>>> +    /*
>>>>>>>>> +     * In "start_port" function, dcb_test is set to 1 based 
>>>>>>>>> on dcb_config.
>>>>>>>>> +     * So it should be cleared when dcb_config is 0.
>>>>>>>>> +     */
>>>>>>>>> +    if (dcb_config == 0)
>>>>>>>>> +        dcb_test = 0;
>>>>>>>>> +
>>>>>>>>
>>>>>>>> I don't understand why are you changing this.
>>>>>>>> dcb_test will only be set when dcb_config is 1 when starting 
>>>>>>>> ports.
>>>>>>>> And both
>>>>>>> dcb_test and dcb_config will be cleared when stopping ports.
>>>>>>>> So dcb will only affect when you set port dcb and then start port
>>>>>>>> and when
>>>>>>> stop port dcb will be cleared.
>>>>>>>>
>>>>>>> Yes, I think. The forwarding streams should not be changed from
>>>>>>> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is
>>>>> configured.
>>>>>>> But, now, the logical codes do it when stopping ports and then 
>>>>>>> starting ports.
>>>>>>>> So what's the problem of original codes?
>>>>>>>>
>>>>>>>> Your change will cause issues that there's no place to set
>>>>>>>> dcb_config as 0. If
>>>>>>> you config dcb, then it'll be always dcb mode unless restart the 
>>>>>>> whole
>>>>> testpmd.
>>>>>>>>
>>>>>>> As far as I know, the current testpmd only supports switching from
>>>>>>> the normal mode to the dcb mode, but does not support the reverse
>>>>> operation.
>>>>>>> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 
>>>>>>> after
>>>>>>> config.
>>>>>>
>>>>>> You're not answering my questions. Why are you changing the 
>>>>>> behavior of
>>>>> testpmd?
>>>>>> Your change will make testpmd stay dcb mode once set dcb mode and 
>>>>>> can't go
>>>>> back to normal mode. If users want to go back to normal mode, 
>>>>> he/she has to
>>>>> restart the whole testpmd.
>>>>>>
>>>>> Yes. Testpmd and PMD driver are both in dcb mode after dcb info is 
>>>>> configured.
>>>>> In my opinion, the 'dcb_test' flag can't be clear to go back to 
>>>>> normal mode after
>>>>> stopping port and then starting port. Because PMD driver is still 
>>>>> dcb mode. If
>>>>> users want to go back it, users should disable dcb mode and enable 
>>>>> RSS or other
>>>>> mode.
>>>>>
>>>>>> It worked as you can set dcb mode and start port. After stopping 
>>>>>> port, if you
>>>>> still want dcb mode, you need to set dcb mode command again. But 
>>>>> at least the
>>>>> old way won't break anything.
>>>>>> @Yigit, Ferruh Not sure which behavior is better, what do you think?
>>>>>>
>>>>>> And @oulijun can you just answer all comments in one thread?
>>>>>>
>>>>> After stopping port, the 'dcb_test' flag is clear. At this moment, 
>>>>> the dcb
>>>>> configuration in testpmd has not be changed.  users may not 
>>>>> understand why
>>>>> the DCB mode needs to be reconfigured.
>>>>
>>>> OK. You're right. There's no place writing port->dcb_flag back to 0.
>>>>
>>>
>>> If there is no way to turn off the DCB, isn't this code logically 
>>> dead? This code is run when "dcb_config == 0" but in that case 
>>> 'dcb_test' is already 0.
>>> If so what is the point of the code, can it be removed?
>>>
>> yes. Currently, it is a dead logic unless testpmd adds support for 
>> switching from DCB mode to other modes.
>> I think dcb_test is a flag at the packet forwarding layer in testpmd. 
>> The original purpose is to indicate that the current forwarding test 
>> is being performed in DCB mode.
>
> can 'dcb_config' be sufficient on its own for this? so that we can 
> remove 'dcb_test' and simplify the logic.
>

I agree with your proposal. Because the 'dcb_flag' field in struct 
rte_port  indicates whether the current port is DCB, It is sufficient to 
have 'dcb_config' as a global variable to control the DCB test status.

ok,I will send V2 to update all of the previous discussions.
>>> btw, do you know what 'dcb_test' does at all? It seems it is set 
>>> when 'dcb_config' set, and reset when 'dcb_config' reset.
>>>
>> In my opinion, it is only a global flag. During a DCB test, DCB must 
>> be configured on all forwarding ports.
>>>>>>>>>         printf("Stopping ports...\n");
>>>>>>>>>
>>>>>>>>>         RTE_ETH_FOREACH_DEV(pi) {
>>>>>>>>> -- 
>>>>>>>>> 2.7.4
>>>>>>>>
>>>>>>>> .
>>>>>>>>
>>>>>> _______________________________________________
>>>>>> Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe 
>>>>>> send an
>>>>>> email to linuxarm-leave@openeuler.org
>>>>>>
>>>
>>> .
>>>
>>
>
> .
>
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 576d5ac..c89f8cd 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2864,6 +2864,21 @@  rss_fwd_config_setup(void)
 	}
 }
 
+static uint16_t
+get_fwd_port_total_tc_num(void)
+{
+	struct rte_eth_dcb_info dcb_info;
+	uint16_t total_tc_num = 0;
+	unsigned int i;
+
+	for (i = 0; i < nb_fwd_ports; i++) {
+		(void)rte_eth_dev_get_dcb_info(fwd_ports_ids[i], &dcb_info);
+		total_tc_num += dcb_info.nb_tcs;
+	}
+
+	return total_tc_num;
+}
+
 /**
  * For the DCB forwarding test, each core is assigned on each traffic class.
  *
@@ -2883,12 +2898,16 @@  dcb_fwd_config_setup(void)
 	lcoreid_t  lc_id;
 	uint16_t nb_rx_queue, nb_tx_queue;
 	uint16_t i, j, k, sm_id = 0;
+	uint16_t total_tc_num = 0;
 	uint8_t tc = 0;
 
 	cur_fwd_config.nb_fwd_lcores = (lcoreid_t) nb_fwd_lcores;
 	cur_fwd_config.nb_fwd_ports = nb_fwd_ports;
 	cur_fwd_config.nb_fwd_streams =
 		(streamid_t) (nb_rxq * cur_fwd_config.nb_fwd_ports);
+	total_tc_num = get_fwd_port_total_tc_num();
+	if (cur_fwd_config.nb_fwd_lcores > total_tc_num)
+		cur_fwd_config.nb_fwd_lcores = total_tc_num;
 
 	/* reinitialize forwarding streams */
 	init_fwd_streams();
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1a57324..7eb810b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2707,14 +2707,16 @@  stop_port(portid_t pid)
 	portid_t peer_pl[RTE_MAX_ETHPORTS];
 	int peer_pi;
 
-	if (dcb_test) {
-		dcb_test = 0;
-		dcb_config = 0;
-	}
-
 	if (port_id_is_invalid(pid, ENABLED_WARN))
 		return;
 
+	/*
+	 * In "start_port" function, dcb_test is set to 1 based on dcb_config.
+	 * So it should be cleared when dcb_config is 0.
+	 */
+	if (dcb_config == 0)
+		dcb_test = 0;
+
 	printf("Stopping ports...\n");
 
 	RTE_ETH_FOREACH_DEV(pi) {