[V2,1/7] app/testpmd: fix forward lcores number when DCB test
Checks
Commit Message
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 missing in dcb_fwd_config_setup, which
may lead to a bad behavior in which multiple polling threads
operate on the same queue. In this case, the device sends and
receives packets, causing unexpected results. The procedure is
as follows:
1/ run testpmd with "--rxq=8 --txq=8"
2/ port stop all
3/ set nbcore 8
4/ port config 0 dcb vt off 4 pfc on
5/ port config all rxq 16
6/ port config all txq 16
7/ port start all
8/ set fwd mac
9/ start
For the DCB forwarding test, each core is assigned to each
traffic class and each core is assigned a multi-stream.
Therefore, 'nb_fwd_lcores' value needs to be adjusted based
on 'total_tc_num' in all forwarding ports.
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 +++++++++++++++++++
1 file changed, 19 insertions(+)
Comments
> -----Original Message-----
> From: Huisong Li <lihuisong@huawei.com>
> Sent: Tuesday, April 20, 2021 15:32
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> linuxarm@openeuler.org; lihuisong@huawei.com
> Subject: [PATCH V2 1/7] app/testpmd: fix forward lcores number when DCB test
>
> 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 missing in dcb_fwd_config_setup, which may lead to a bad
> behavior in which multiple polling threads operate on the same queue.
This patch is OK. But commit log is redundant and confusing.
The above is enough to explain what your patch is doing and can even be more simple.
>In this
> case, the device sends and receives packets, causing unexpected results. The
> procedure is as follows:
I don't understand what you're saying here. The commands you're showing is 8 nbcores dealing with 16 queues. So it's one thread dealing with multiple queues which doesn't have issues at all.
Please remove the useless and confusing commands.
> 1/ run testpmd with "--rxq=8 --txq=8"
> 2/ port stop all
> 3/ set nbcore 8
> 4/ port config 0 dcb vt off 4 pfc on
> 5/ port config all rxq 16
> 6/ port config all txq 16
> 7/ port start all
> 8/ set fwd mac
> 9/ start
>
> For the DCB forwarding test, each core is assigned to each traffic class and each
> core is assigned a multi-stream.
> Therefore, 'nb_fwd_lcores' value needs to be adjusted based on 'total_tc_num'
> in all forwarding ports.
Please refer to the RSS fwd config fix patch to write your own commit log. Use simple and easy-understanding words to explain yourself.
Below is the reference of RSS.
commit 017d680a91fcf30da14a6d3a2f96d41f6dda3a0f
Author: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Date: Mon Jun 27 23:35:19 2016 +0100
app/testpmd: limit number of forwarding cores
Number of forwarding cores must be equal or less than
number of forwarding streams, otherwise two cores
would try to use a same queue on a port, which is not allowed.
>
> 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 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> ccb9bd3..03ee40c 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2961,6 +2961,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.
> *
> @@ -2980,12 +2995,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;
> 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();
> --
> 2.7.4
在 2021/4/27 17:20, Li, Xiaoyun 写道:
>
>> -----Original Message-----
>> From: Huisong Li <lihuisong@huawei.com>
>> Sent: Tuesday, April 20, 2021 15:32
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>> linuxarm@openeuler.org; lihuisong@huawei.com
>> Subject: [PATCH V2 1/7] app/testpmd: fix forward lcores number when DCB test
>>
>> 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 missing in dcb_fwd_config_setup, which may lead to a bad
>> behavior in which multiple polling threads operate on the same queue.
> This patch is OK. But commit log is redundant and confusing.
> The above is enough to explain what your patch is doing and can even be more simple.
Sorry, I will fix it.
>> In this
>> case, the device sends and receives packets, causing unexpected results. The
>> procedure is as follows:
> I don't understand what you're saying here. The commands you're showing is 8 nbcores dealing with 16 queues. So it's one thread dealing with multiple queues which doesn't have issues at all.
> Please remove the useless and confusing commands.
In the DCB test scenario, each TC is assigned a core, and one core can
process multiple streams, that is, multiple queues.
In this scenario, if four TCs are enabled and the total number of Rx/Tx
queues are set to 16, each TC is responsible for four queues. If we set
8 core to start forwarding, and then 8 polling threads will be pulled
up. Actually, the last four threads are redundant.
Before changing the number of polling cores, if the number of columns is
greater than the number of TCs used later. In the running process of
the dcb_fwd_config_setup(), the data structure 'fwd_lcores[]'
corresponding to the last four cores is not reinitialized. As a result,
the last four threads would use the queue polled by the first four cores.
It's probably a bit of a twist. In general, it is conditional that
multiple cores operate on the same queue. That's why I posted the
following steps.
In different forwarding mode, 'fwd_lcores[]' of all cores may need to be
cleared before 'fwd_lcores[]' of each forwarding core is initialized.
What do you think?
>> 1/ run testpmd with "--rxq=8 --txq=8"
>> 2/ port stop all
>> 3/ set nbcore 8
>> 4/ port config 0 dcb vt off 4 pfc on
>> 5/ port config all rxq 16
>> 6/ port config all txq 16
>> 7/ port start all
>> 8/ set fwd mac
>> 9/ start
>>
>> For the DCB forwarding test, each core is assigned to each traffic class and each
>> core is assigned a multi-stream.
>> Therefore, 'nb_fwd_lcores' value needs to be adjusted based on 'total_tc_num'
>> in all forwarding ports.
> Please refer to the RSS fwd config fix patch to write your own commit log. Use simple and easy-understanding words to explain yourself.
> Below is the reference of RSS.
>
> commit 017d680a91fcf30da14a6d3a2f96d41f6dda3a0f
> Author: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Date: Mon Jun 27 23:35:19 2016 +0100
>
> app/testpmd: limit number of forwarding cores
>
> Number of forwarding cores must be equal or less than
> number of forwarding streams, otherwise two cores
> would try to use a same queue on a port, which is not allowed.
>
>> 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 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>> ccb9bd3..03ee40c 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2961,6 +2961,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.
>> *
>> @@ -2980,12 +2995,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;
>> 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();
>> --
>> 2.7.4
> .
> -----Original Message-----
> From: Huisong Li <lihuisong@huawei.com>
> Sent: Tuesday, April 27, 2021 21:50
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [PATCH V2 1/7] app/testpmd: fix forward lcores number when DCB
> test
>
>
> 在 2021/4/27 17:20, Li, Xiaoyun 写道:
> >
> >> -----Original Message-----
> >> From: Huisong Li <lihuisong@huawei.com>
> >> Sent: Tuesday, April 20, 2021 15:32
> >> To: dev@dpdk.org
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >> <xiaoyun.li@intel.com>; linuxarm@openeuler.org; lihuisong@huawei.com
> >> Subject: [PATCH V2 1/7] app/testpmd: fix forward lcores number when
> >> DCB test
> >>
> >> 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 missing in dcb_fwd_config_setup, which may lead
> >> to a bad behavior in which multiple polling threads operate on the same
> queue.
> > This patch is OK. But commit log is redundant and confusing.
> > The above is enough to explain what your patch is doing and can even be more
> simple.
> Sorry, I will fix it.
> >> In this
> >> case, the device sends and receives packets, causing unexpected
> >> results. The procedure is as follows:
> > I don't understand what you're saying here. The commands you're showing is 8
> nbcores dealing with 16 queues. So it's one thread dealing with multiple queues
> which doesn't have issues at all.
> > Please remove the useless and confusing commands.
>
> In the DCB test scenario, each TC is assigned a core, and one core can process
> multiple streams, that is, multiple queues.
>
> In this scenario, if four TCs are enabled and the total number of Rx/Tx queues
> are set to 16, each TC is responsible for four queues. If we set
> 8 core to start forwarding, and then 8 polling threads will be pulled up. Actually,
> the last four threads are redundant.
>
> Before changing the number of polling cores, if the number of columns is
> greater than the number of TCs used later. In the running process of the
> dcb_fwd_config_setup(), the data structure 'fwd_lcores[]'
> corresponding to the last four cores is not reinitialized. As a result, the last four
> threads would use the queue polled by the first four cores.
>
> It's probably a bit of a twist. In general, it is conditional that multiple cores
> operate on the same queue. That's why I posted the following steps.
>
> In different forwarding mode, 'fwd_lcores[]' of all cores may need to be cleared
> before 'fwd_lcores[]' of each forwarding core is initialized.
> What do you think?
Understood. But then you actually don't need to set rxq/txq to 16. Even if it's 8 queues, each tc is responsible for 2 queues. 8 cores will still cause that issue.
Changing queue number will be confusing. Like I didn't read it very carefully then I misunderstood what you were trying to explain here.
The example doesn't make your commit log clear but more confusing.
And the code is actually quite straightforward. I think the easy explanation is better.
But anyway, if you still want to keep the example, remove setting queue number again.
>
> >> 1/ run testpmd with "--rxq=8 --txq=8"
> >> 2/ port stop all
> >> 3/ set nbcore 8
> >> 4/ port config 0 dcb vt off 4 pfc on
> >> 5/ port config all rxq 16
> >> 6/ port config all txq 16
> >> 7/ port start all
> >> 8/ set fwd mac
> >> 9/ start
> >>
> >> For the DCB forwarding test, each core is assigned to each traffic
> >> class and each core is assigned a multi-stream.
> >> Therefore, 'nb_fwd_lcores' value needs to be adjusted based on
> 'total_tc_num'
> >> in all forwarding ports.
> > Please refer to the RSS fwd config fix patch to write your own commit log. Use
> simple and easy-understanding words to explain yourself.
> > Below is the reference of RSS.
> >
> > commit 017d680a91fcf30da14a6d3a2f96d41f6dda3a0f
> > Author: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > Date: Mon Jun 27 23:35:19 2016 +0100
> >
> > app/testpmd: limit number of forwarding cores
> >
> > Number of forwarding cores must be equal or less than
> > number of forwarding streams, otherwise two cores
> > would try to use a same queue on a port, which is not allowed.
> >
> >> 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 +++++++++++++++++++
> >> 1 file changed, 19 insertions(+)
> >>
> >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >> ccb9bd3..03ee40c 100644
> >> --- a/app/test-pmd/config.c
> >> +++ b/app/test-pmd/config.c
> >> @@ -2961,6 +2961,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.
> >> *
> >> @@ -2980,12 +2995,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;
> >> 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();
> >> --
> >> 2.7.4
> > .
@@ -2961,6 +2961,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.
*
@@ -2980,12 +2995,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;
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();