Re: [dpdk-dev] [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB test

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

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