[dpdk-dev] [PATCH 0/3] testpmd updates

[dpdk-dev] [PATCH 0/3] testpmd updates

[Lijun Ou]

This series add a API implementation for testpmd as
well as fix two bugs.

Chengwen Feng (1):
  app/testpmd: support Tx mbuf free on demand cmd

Hongbo Zheng (1):
  app/testpmd: fix mixed use of RX/Rx/TX/Tx in testpmd

Huisong Li (1):
  app/testpmd: remove forwarding config from parsing Rx and Tx

 app/test-pmd/cmdline.c                      | 93 ++++++++++++++++++++++++++++-
 app/test-pmd/config.c                       | 18 +++---
 doc/guides/rel_notes/release_21_05.rst      |  2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
 4 files changed, 109 insertions(+), 11 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/3] app/testpmd: support Tx mbuf free on demand cmd

[Lijun Ou]

From: Chengwen Feng <fengchengwen@huawei.com>

This patch support tx_done_cleanup command:
tx_done_cleanup port (port_id) (queue_id) (free_cnt)

User must make sure there are no concurrent access to the same Tx
queue (like rte_eth_tx_burst, rte_eth_dev_tx_queue_stop and so on)
when this command executed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 app/test-pmd/cmdline.c                      | 91 +++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_21_05.rst      |  2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
 3 files changed, 100 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 14110eb..832ae70 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -36,6 +36,7 @@
 #include <rte_pci.h>
 #include <rte_ether.h>
 #include <rte_ethdev.h>
+#include <rte_ethdev_driver.h>
 #include <rte_string_fns.h>
 #include <rte_devargs.h>
 #include <rte_flow.h>
@@ -675,6 +676,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set port (port_id) ptype_mask (ptype_mask)\n"
 			"    set packet types classification for a specific port\n\n"
 
+			"tx_done_cleanup (port_id) (queue_id) (free_cnt)\n"
+			"    Cleanup a tx queue's mbuf on a port\n\n"
+
 			"set port (port_id) queue-region region_id (value) "
 			"queue_start_index (value) queue_num (value)\n"
 			"    Set a queue region on a port\n\n"
@@ -16910,6 +16914,92 @@ cmdline_parse_inst_t cmd_showport_macs = {
 	},
 };
 
+/* *** tx_done_cleanup *** */
+struct cmd_tx_done_cleanup_result {
+	cmdline_fixed_string_t clean;
+	cmdline_fixed_string_t port;
+	uint16_t port_id;
+	uint16_t queue_id;
+	uint32_t free_cnt;
+};
+
+static void
+cmd_tx_done_cleanup_parsed(void *parsed_result,
+			   __rte_unused struct cmdline *cl,
+			   __rte_unused void *data)
+{
+	struct cmd_tx_done_cleanup_result *res = parsed_result;
+	struct rte_eth_dev *dev;
+	uint16_t port_id = res->port_id;
+	uint16_t queue_id = res->queue_id;
+	uint32_t free_cnt = res->free_cnt;
+	int ret;
+
+	if (!rte_eth_dev_is_valid_port(port_id)) {
+		printf("Invalid port_id %u\n", port_id);
+		return;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	if (queue_id >= dev->data->nb_tx_queues) {
+		printf("Invalid TX queue_id %u\n", queue_id);
+		return;
+	}
+
+	if (dev->data->tx_queue_state[queue_id] !=
+		RTE_ETH_QUEUE_STATE_STARTED) {
+		printf("TX queue_id %u not started!\n", queue_id);
+		return;
+	}
+
+	/*
+	 * rte_eth_tx_done_cleanup is a dataplane API, user must make sure
+	 * there are no concurrent access to the same Tx queue (like
+	 * rte_eth_tx_burst, rte_eth_dev_tx_queue_stop and so on) when this API
+	 * called.
+	 */
+	ret = rte_eth_tx_done_cleanup(port_id, queue_id, free_cnt);
+	if (ret < 0) {
+		printf("Failed to cleanup mbuf for port %u TX queue %u "
+		       "error desc: %s(%d)\n",
+		       port_id, queue_id, strerror(-ret), ret);
+		return;
+	}
+
+	printf("Cleanup port %u TX queue %u mbuf nums: %u\n",
+	       port_id, queue_id, ret);
+}
+
+cmdline_parse_token_string_t cmd_tx_done_cleanup_clean =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_done_cleanup_result, clean,
+				 "tx_done_cleanup");
+cmdline_parse_token_string_t cmd_tx_done_cleanup_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_done_cleanup_result, port,
+				 "port");
+cmdline_parse_token_num_t cmd_tx_done_cleanup_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_tx_done_cleanup_result, port_id,
+			      UINT16);
+cmdline_parse_token_num_t cmd_tx_done_cleanup_queue_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_tx_done_cleanup_result, queue_id,
+			      UINT16);
+cmdline_parse_token_num_t cmd_tx_done_cleanup_free_cnt =
+	TOKEN_NUM_INITIALIZER(struct cmd_tx_done_cleanup_result, free_cnt,
+			      UINT32);
+
+cmdline_parse_inst_t cmd_tx_done_cleanup = {
+	.f = cmd_tx_done_cleanup_parsed,
+	.data = NULL,
+	.help_str = "tx_done_cleanup port <port_id> <queue_id> <free_cnt>",
+	.tokens = {
+		(void *)&cmd_tx_done_cleanup_clean,
+		(void *)&cmd_tx_done_cleanup_port,
+		(void *)&cmd_tx_done_cleanup_port_id,
+		(void *)&cmd_tx_done_cleanup_queue_id,
+		(void *)&cmd_tx_done_cleanup_free_cnt,
+		NULL,
+	},
+};
+
 /* ******************************************************************************** */
 
 /* list of instructions */
@@ -17035,6 +17125,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_config_rss_reta,
 	(cmdline_parse_inst_t *)&cmd_showport_reta,
 	(cmdline_parse_inst_t *)&cmd_showport_macs,
+	(cmdline_parse_inst_t *)&cmd_tx_done_cleanup,
 	(cmdline_parse_inst_t *)&cmd_config_burst,
 	(cmdline_parse_inst_t *)&cmd_config_thresh,
 	(cmdline_parse_inst_t *)&cmd_config_threshold,
diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 23f7f0b..8077573 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -69,6 +69,8 @@ New Features
 
   * Added command to display Rx queue used descriptor count.
     ``show port (port_id) rxq (queue_id) desc used count``
+  * Added command to cleanup a Tx queue's mbuf on a port.
+    ``tx_done_cleanup port <port_id> <queue_id> <free_cnt>``
 
 
 Removed Items
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index f59eb8a..39281f5 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -272,6 +272,13 @@ and ready to be processed by the driver on a given RX queue::
 
    testpmd> show port (port_id) rxq (queue_id) desc used count
 
+cleanup txq mbufs
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+Request the driver to free mbufs currently cached by the driver for a given port's
+Tx queue::
+   testpmd> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
+
 show config
 ~~~~~~~~~~~
 
-- 
2.7.4

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

[Lijun Ou]

From: Huisong Li <lihuisong@huawei.com>

The "fwd_config_setup()" function does release and apply for
memory of forwarding flows, and re-establish these flows when
rxq/txq or rxd/txd is changed. The function is also called by
"start_packet_forwarding()" when user executes "start" cmd.
All changes for rxq/txq or rxd/txd can be updated uniformly
when this command is executed. Therefore, it is a little
redundant in the "cmd_config_rx_tx_parsed" function.

In addition, the forwarding flows under one TC is configured
based on number of queues allocated to TC. And number of queues
allocated to TC is updated after calling  "rte_eth_dev_configure"
again. If the number of queues is reduced after configuring the
DCB, and then, release and apply for flow memory, and reinitialize
the forwarding flows under the DCB mode based on the old TC
information. As a result, null pointer may be accessed.

Like:
set nbcore 4
port stop all
port config 0 dcb vt off 4 pfc on
port start all
port stop all
port config all rxq 8
port config all txq 8

At the moment, a segmentation fault occurs.

Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 app/test-pmd/cmdline.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 832ae70..8b0f7d5 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
 		return;
 	}
 
-	fwd_config_setup();
-
 	init_port_config();
 
 	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
-- 
2.7.4

[dpdk-dev] [PATCH 3/3] app/testpmd: fix mixed use of RX/Rx/TX/Tx in testpmd

[Lijun Ou]

From: Hongbo Zheng <zhenghongbo3@huawei.com>

In testpmd, when we input "show config rxtx", we can see like this:

1: testpmd> show config rxtx
2:   io packet forwarding packets/burst=32
3:   nb forwarding cores=1 - nb forwarding ports=1
4:   port 0: RX queue number: 1 Tx queue number: 1
5:     Rx offloads=0x0 Tx offloads=0x10000
6:     RX queue: 0
7:       RX desc=1024 - RX free threshold=32
8:       RX threshold registers: pthresh=0 hthresh=0  wthresh=0
9:       RX Offloads=0x0
10:    TX queue: 0
11:      TX desc=1024 - TX free threshold=928
12:      TX threshold registers: pthresh=0 hthresh=0  wthresh=0
13:      TX offloads=0x10000 - TX RS bit threshold=32

In line 4, RX/Tx is mixed used. Also in other lines, RX/Rx/TX/Tx is
mixed used.

This patch fix the mixed use of RX/Rx/TX/Tx in testpmd command
"show config rxtx" output by change to unified use Rx/Tx.

Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 app/test-pmd/config.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 576d5ac..2435c26 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2505,7 +2505,7 @@ rxtx_config_display(void)
 		int32_t rc;
 
 		/* per port config */
-		printf("  port %d: RX queue number: %d Tx queue number: %d\n",
+		printf("  port %d: Rx queue number: %d Tx queue number: %d\n",
 				(unsigned int)pid, nb_rxq, nb_txq);
 
 		printf("    Rx offloads=0x%"PRIx64" Tx offloads=0x%"PRIx64"\n",
@@ -2533,13 +2533,13 @@ rxtx_config_display(void)
 				offloads_tmp = rx_qinfo.conf.offloads;
 			}
 
-			printf("    RX queue: %d\n", qid);
-			printf("      RX desc=%d - RX free threshold=%d\n",
+			printf("    Rx queue: %d\n", qid);
+			printf("      Rx desc=%d - Rx free threshold=%d\n",
 				nb_rx_desc_tmp, rx_free_thresh_tmp);
-			printf("      RX threshold registers: pthresh=%d hthresh=%d "
+			printf("      Rx threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
 				pthresh_tmp, hthresh_tmp, wthresh_tmp);
-			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
+			printf("      Rx Offloads=0x%"PRIx64"\n", offloads_tmp);
 		}
 
 		/* per tx queue config only for first queue to be less verbose */
@@ -2565,13 +2565,13 @@ rxtx_config_display(void)
 				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
 			}
 
-			printf("    TX queue: %d\n", qid);
-			printf("      TX desc=%d - TX free threshold=%d\n",
+			printf("    Tx queue: %d\n", qid);
+			printf("      Tx desc=%d - Tx free threshold=%d\n",
 				nb_tx_desc_tmp, tx_free_thresh_tmp);
-			printf("      TX threshold registers: pthresh=%d hthresh=%d "
+			printf("      Tx threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
 				pthresh_tmp, hthresh_tmp, wthresh_tmp);
-			printf("      TX offloads=0x%"PRIx64" - TX RS bit threshold=%d\n",
+			printf("      Tx offloads=0x%"PRIx64" - Tx RS bit threshold=%d\n",
 				offloads_tmp, tx_rs_thresh_tmp);
 		}
 	}
-- 
2.7.4

Re: [dpdk-dev] [PATCH 0/3] testpmd updates

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
> Sent: Friday, March 5, 2021 08:56
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: [dpdk-dev] [PATCH 0/3] testpmd updates
> 
> This series add a API implementation for testpmd as well as fix two bugs.
> 
> Chengwen Feng (1):
>   app/testpmd: support Tx mbuf free on demand cmd
> 
> Hongbo Zheng (1):
>   app/testpmd: fix mixed use of RX/Rx/TX/Tx in testpmd
> 
> Huisong Li (1):
>   app/testpmd: remove forwarding config from parsing Rx and Tx
> 

Please separate this patchset to 3 patches.
For the first patch, you should add a release note in release_21_05.rst.
And for the next 2 fixes, please add corresponding fix line and cc to stable@dpdk.org. 

And also, if you're sending patches related to testpmd, please also send/cc to me.
You can use add "--to-cmd ./devtools/get-maintainer.sh" when you use git send-email to make sure all of related people will be in mail list.

>  app/test-pmd/cmdline.c                      | 93 ++++++++++++++++++++++++++++-
>  app/test-pmd/config.c                       | 18 +++---
>  doc/guides/rel_notes/release_21_05.rst      |  2 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
>  4 files changed, 109 insertions(+), 11 deletions(-)
> 
> --
> 2.7.4

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

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
> Sent: Friday, March 5, 2021 08:56
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: [dpdk-dev] [PATCH 2/3] app/testpmd: remove forwarding config from
> parsing Rx and Tx
> 
> From: Huisong Li <lihuisong@huawei.com>
> 
> The "fwd_config_setup()" function does release and apply for memory of
> forwarding flows, and re-establish these flows when rxq/txq or rxd/txd is
> changed. The function is also called by "start_packet_forwarding()" when user
> executes "start" cmd.
> All changes for rxq/txq or rxd/txd can be updated uniformly when this command
> is executed. Therefore, it is a little redundant in the "cmd_config_rx_tx_parsed"
> function.
> 
> In addition, the forwarding flows under one TC is configured based on number
> of queues allocated to TC. And number of queues allocated to TC is updated
> after calling  "rte_eth_dev_configure"
> again. If the number of queues is reduced after configuring the DCB, and then,
> release and apply for flow memory, and reinitialize the forwarding flows under
> the DCB mode based on the old TC information. As a result, null pointer may be
> accessed.

The patch looks good to me. But in commit log, you mean forwarding streams, right? Flows will confuse to mean rte_flow.

> 
> Like:
> set nbcore 4
> port stop all
> port config 0 dcb vt off 4 pfc on
> port start all
> port stop all
> port config all rxq 8
> port config all txq 8
> 
> At the moment, a segmentation fault occurs.
> 
> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 832ae70..8b0f7d5 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
>  		return;
>  	}
> 
> -	fwd_config_setup();
> -
>  	init_port_config();
> 
>  	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> --
> 2.7.4

Re: [dpdk-dev] [PATCH 3/3] app/testpmd: fix mixed use of RX/Rx/TX/Tx in testpmd

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
> Sent: Friday, March 5, 2021 08:56
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: [dpdk-dev] [PATCH 3/3] app/testpmd: fix mixed use of RX/Rx/TX/Tx in
> testpmd
> 
> From: Hongbo Zheng <zhenghongbo3@huawei.com>
> 
> In testpmd, when we input "show config rxtx", we can see like this:
> 
> 1: testpmd> show config rxtx
> 2:   io packet forwarding packets/burst=32
> 3:   nb forwarding cores=1 - nb forwarding ports=1
> 4:   port 0: RX queue number: 1 Tx queue number: 1
> 5:     Rx offloads=0x0 Tx offloads=0x10000
> 6:     RX queue: 0
> 7:       RX desc=1024 - RX free threshold=32
> 8:       RX threshold registers: pthresh=0 hthresh=0  wthresh=0
> 9:       RX Offloads=0x0
> 10:    TX queue: 0
> 11:      TX desc=1024 - TX free threshold=928
> 12:      TX threshold registers: pthresh=0 hthresh=0  wthresh=0
> 13:      TX offloads=0x10000 - TX RS bit threshold=32
> 
> In line 4, RX/Tx is mixed used. Also in other lines, RX/Rx/TX/Tx is mixed used.

If you're going to unify the print to only use "Rx/Tx" in testpmd as your title said.
There're a lot of more places using RX/TX when printing. You should change them too.

> 
> This patch fix the mixed use of RX/Rx/TX/Tx in testpmd command "show config
> rxtx" output by change to unified use Rx/Tx.
> 
> Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>  app/test-pmd/config.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 576d5ac..2435c26 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2505,7 +2505,7 @@ rxtx_config_display(void)
>  		int32_t rc;
> 
>  		/* per port config */
> -		printf("  port %d: RX queue number: %d Tx queue
> number: %d\n",
> +		printf("  port %d: Rx queue number: %d Tx queue number: %d\n",
>  				(unsigned int)pid, nb_rxq, nb_txq);
> 
>  		printf("    Rx offloads=0x%"PRIx64" Tx offloads=0x%"PRIx64"\n",
> @@ -2533,13 +2533,13 @@ rxtx_config_display(void)
>  				offloads_tmp = rx_qinfo.conf.offloads;
>  			}
> 
> -			printf("    RX queue: %d\n", qid);
> -			printf("      RX desc=%d - RX free threshold=%d\n",
> +			printf("    Rx queue: %d\n", qid);
> +			printf("      Rx desc=%d - Rx free threshold=%d\n",
>  				nb_rx_desc_tmp, rx_free_thresh_tmp);
> -			printf("      RX threshold registers: pthresh=%d
> hthresh=%d "
> +			printf("      Rx threshold registers: pthresh=%d
> hthresh=%d "
>  				" wthresh=%d\n",
>  				pthresh_tmp, hthresh_tmp, wthresh_tmp);
> -			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
> +			printf("      Rx Offloads=0x%"PRIx64"\n", offloads_tmp);
>  		}
> 
>  		/* per tx queue config only for first queue to be less verbose */
> @@ -2565,13 +2565,13 @@ rxtx_config_display(void)
>  				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
>  			}
> 
> -			printf("    TX queue: %d\n", qid);
> -			printf("      TX desc=%d - TX free threshold=%d\n",
> +			printf("    Tx queue: %d\n", qid);
> +			printf("      Tx desc=%d - Tx free threshold=%d\n",
>  				nb_tx_desc_tmp, tx_free_thresh_tmp);
> -			printf("      TX threshold registers: pthresh=%d
> hthresh=%d "
> +			printf("      Tx threshold registers: pthresh=%d
> hthresh=%d "
>  				" wthresh=%d\n",
>  				pthresh_tmp, hthresh_tmp, wthresh_tmp);
> -			printf("      TX offloads=0x%"PRIx64" - TX RS bit
> threshold=%d\n",
> +			printf("      Tx offloads=0x%"PRIx64" - Tx RS bit
> threshold=%d\n",
>  				offloads_tmp, tx_rs_thresh_tmp);
>  		}
>  	}
> --
> 2.7.4

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

[oulijun]

在 2021/3/5 11:21, Li, Xiaoyun 写道:
> Hi
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
>> Sent: Friday, March 5, 2021 08:56
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>> Subject: [dpdk-dev] [PATCH 2/3] app/testpmd: remove forwarding config from
>> parsing Rx and Tx
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The "fwd_config_setup()" function does release and apply for memory of
>> forwarding flows, and re-establish these flows when rxq/txq or rxd/txd is
>> changed. The function is also called by "start_packet_forwarding()" when user
>> executes "start" cmd.
>> All changes for rxq/txq or rxd/txd can be updated uniformly when this command
>> is executed. Therefore, it is a little redundant in the "cmd_config_rx_tx_parsed"
>> function.
>>
>> In addition, the forwarding flows under one TC is configured based on number
>> of queues allocated to TC. And number of queues allocated to TC is updated
>> after calling  "rte_eth_dev_configure"
>> again. If the number of queues is reduced after configuring the DCB, and then,
>> release and apply for flow memory, and reinitialize the forwarding flows under
>> the DCB mode based on the old TC information. As a result, null pointer may be
>> accessed.
> 
> The patch looks good to me. But in commit log, you mean forwarding streams, right? Flows will confuse to mean rte_flow.
Yes. it is forwarding streams.
> 
>>
>> Like:
>> set nbcore 4
>> port stop all
>> port config 0 dcb vt off 4 pfc on
>> port start all
>> port stop all
>> port config all rxq 8
>> port config all txq 8
>>
>> At the moment, a segmentation fault occurs.
>>
>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>> 832ae70..8b0f7d5 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
>>   		return;
>>   	}
>>
>> -	fwd_config_setup();
>> -
>>   	init_port_config();
>>
>>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> --
>> 2.7.4
> 
> .
> 

Re: [dpdk-dev] [PATCH 3/3] app/testpmd: fix mixed use of RX/Rx/TX/Tx in testpmd

[oulijun]

在 2021/3/5 11:25, Li, Xiaoyun 写道:
> Hi
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
>> Sent: Friday, March 5, 2021 08:56
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>> Subject: [dpdk-dev] [PATCH 3/3] app/testpmd: fix mixed use of RX/Rx/TX/Tx in
>> testpmd
>>
>> From: Hongbo Zheng <zhenghongbo3@huawei.com>
>>
>> In testpmd, when we input "show config rxtx", we can see like this:
>>
>> 1: testpmd> show config rxtx
>> 2:   io packet forwarding packets/burst=32
>> 3:   nb forwarding cores=1 - nb forwarding ports=1
>> 4:   port 0: RX queue number: 1 Tx queue number: 1
>> 5:     Rx offloads=0x0 Tx offloads=0x10000
>> 6:     RX queue: 0
>> 7:       RX desc=1024 - RX free threshold=32
>> 8:       RX threshold registers: pthresh=0 hthresh=0  wthresh=0
>> 9:       RX Offloads=0x0
>> 10:    TX queue: 0
>> 11:      TX desc=1024 - TX free threshold=928
>> 12:      TX threshold registers: pthresh=0 hthresh=0  wthresh=0
>> 13:      TX offloads=0x10000 - TX RS bit threshold=32
>>
>> In line 4, RX/Tx is mixed used. Also in other lines, RX/Rx/TX/Tx is mixed used.
> 
> If you're going to unify the print to only use "Rx/Tx" in testpmd as your title said.
> There're a lot of more places using RX/TX when printing. You should change them too.
> 
OK， I will do it.
>>
>> This patch fix the mixed use of RX/Rx/TX/Tx in testpmd command "show config
>> rxtx" output by change to unified use Rx/Tx.
>>
>> Signed-off-by: Hongbo Zheng <zhenghongbo3@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>>   app/test-pmd/config.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>> 576d5ac..2435c26 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2505,7 +2505,7 @@ rxtx_config_display(void)
>>   		int32_t rc;
>>
>>   		/* per port config */
>> -		printf("  port %d: RX queue number: %d Tx queue
>> number: %d\n",
>> +		printf("  port %d: Rx queue number: %d Tx queue number: %d\n",
>>   				(unsigned int)pid, nb_rxq, nb_txq);
>>
>>   		printf("    Rx offloads=0x%"PRIx64" Tx offloads=0x%"PRIx64"\n",
>> @@ -2533,13 +2533,13 @@ rxtx_config_display(void)
>>   				offloads_tmp = rx_qinfo.conf.offloads;
>>   			}
>>
>> -			printf("    RX queue: %d\n", qid);
>> -			printf("      RX desc=%d - RX free threshold=%d\n",
>> +			printf("    Rx queue: %d\n", qid);
>> +			printf("      Rx desc=%d - Rx free threshold=%d\n",
>>   				nb_rx_desc_tmp, rx_free_thresh_tmp);
>> -			printf("      RX threshold registers: pthresh=%d
>> hthresh=%d "
>> +			printf("      Rx threshold registers: pthresh=%d
>> hthresh=%d "
>>   				" wthresh=%d\n",
>>   				pthresh_tmp, hthresh_tmp, wthresh_tmp);
>> -			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
>> +			printf("      Rx Offloads=0x%"PRIx64"\n", offloads_tmp);
>>   		}
>>
>>   		/* per tx queue config only for first queue to be less verbose */
>> @@ -2565,13 +2565,13 @@ rxtx_config_display(void)
>>   				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
>>   			}
>>
>> -			printf("    TX queue: %d\n", qid);
>> -			printf("      TX desc=%d - TX free threshold=%d\n",
>> +			printf("    Tx queue: %d\n", qid);
>> +			printf("      Tx desc=%d - Tx free threshold=%d\n",
>>   				nb_tx_desc_tmp, tx_free_thresh_tmp);
>> -			printf("      TX threshold registers: pthresh=%d
>> hthresh=%d "
>> +			printf("      Tx threshold registers: pthresh=%d
>> hthresh=%d "
>>   				" wthresh=%d\n",
>>   				pthresh_tmp, hthresh_tmp, wthresh_tmp);
>> -			printf("      TX offloads=0x%"PRIx64" - TX RS bit
>> threshold=%d\n",
>> +			printf("      Tx offloads=0x%"PRIx64" - Tx RS bit
>> threshold=%d\n",
>>   				offloads_tmp, tx_rs_thresh_tmp);
>>   		}
>>   	}
>> --
>> 2.7.4
> 
> .
> 

Re: [dpdk-dev] [PATCH 0/3] testpmd updates

[oulijun]

在 2021/3/5 11:18, Li, Xiaoyun 写道:
> Hi
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
>> Sent: Friday, March 5, 2021 08:56
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>> Subject: [dpdk-dev] [PATCH 0/3] testpmd updates
>>
>> This series add a API implementation for testpmd as well as fix two bugs.
>>
>> Chengwen Feng (1):
>>    app/testpmd: support Tx mbuf free on demand cmd
>>
>> Hongbo Zheng (1):
>>    app/testpmd: fix mixed use of RX/Rx/TX/Tx in testpmd
>>
>> Huisong Li (1):
>>    app/testpmd: remove forwarding config from parsing Rx and Tx
>>
> 
> Please separate this patchset to 3 patches.
> For the first patch, you should add a release note in release_21_05.rst.
> And for the next 2 fixes, please add corresponding fix line and cc to stable@dpdk.org.
> 
> And also, if you're sending patches related to testpmd, please also send/cc to me.
> You can use add "--to-cmd ./devtools/get-maintainer.sh" when you use git send-email to make sure all of related people will be in mail list.
> 
Thanks. I will do this.
>>   app/test-pmd/cmdline.c                      | 93 ++++++++++++++++++++++++++++-
>>   app/test-pmd/config.c                       | 18 +++---
>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  7 +++
>>   4 files changed, 109 insertions(+), 11 deletions(-)
>>
>> --
>> 2.7.4
> 
> .
> 

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

[oulijun]

在 2021/4/12 18:35, Ferruh Yigit 写道:
> On 3/5/2021 10:22 AM, Lijun Ou wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The "fwd_config_setup()" function does release and apply for
>> memory of forwarding flows, and re-establish these streams when
>> rxq/txq or rxd/txd is changed. The function is also called by
>> "start_packet_forwarding()" when user executes "start" cmd.
>> All changes for rxq/txq or rxd/txd can be updated uniformly
>> when this command is executed. Therefore, it is a little
>> redundant in the "cmd_config_rx_tx_parsed" function.
>>
>> In addition, the forwarding stream under one TC is configured
>> based on number of queues allocated to TC. And number of queues
>> allocated to TC is updated after calling  "rte_eth_dev_configure"
>> again. If the number of queues is reduced after configuring the
>> DCB, and then, release and apply for stream memory, and reinitialize
>> the forwarding stream under the DCB mode based on the old TC
>> information. As a result, null pointer may be accessed.
>>
>> Like:
>> set nbcore 4
>> port stop all
>> port config 0 dcb vt off 4 pfc on
>> port start all
>> port stop all
>> port config all rxq 8
>> port config all txq 8
>>
>> At the moment, a segmentation fault occurs.
>>
> 
> Hi Lijun, Xiaoyun,
> 
> I read the thread and checked the code, but still I am not completely 
> clear with the dcp implementation in testpmd.
> 
> But the "dcb_config = 0" in the 'stop_port()' that has been removed in 
> the patch 1/3 seems preventing these kind of config combination, with 
> implicitly resetting the dcb feature with port stop.
> 
> What do you think just have the number of forwarding cores adjustment 
> change in the patch 1/3, and keep the 'stop_port()' as it is, will it 
> solve this problem, and how acceptable it will be do you think?
> 
I think it might be more appropriate to follow the discussion with Xiao 
Yun, since it does have problems.

In addition, I found another problem. Before calling 
dcb_fwd_config_setup(), testpmd must ensure that all ports have been 
configured with dcb.
Let's look at the next V3.
>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V1->V2:
>> - use stream instead of flow
>> ---
>>   app/test-pmd/cmdline.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 4df0c32..e316f5c 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
>>           return;
>>       }
>> -    fwd_config_setup();
>> -
>>       init_port_config();
>>       cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>
> 
> .
> 

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

[Ferruh Yigit]

On 3/5/2021 10:22 AM, Lijun Ou wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> The "fwd_config_setup()" function does release and apply for
> memory of forwarding flows, and re-establish these streams when
> rxq/txq or rxd/txd is changed. The function is also called by
> "start_packet_forwarding()" when user executes "start" cmd.
> All changes for rxq/txq or rxd/txd can be updated uniformly
> when this command is executed. Therefore, it is a little
> redundant in the "cmd_config_rx_tx_parsed" function.
> 
> In addition, the forwarding stream under one TC is configured
> based on number of queues allocated to TC. And number of queues
> allocated to TC is updated after calling  "rte_eth_dev_configure"
> again. If the number of queues is reduced after configuring the
> DCB, and then, release and apply for stream memory, and reinitialize
> the forwarding stream under the DCB mode based on the old TC
> information. As a result, null pointer may be accessed.
> 
> Like:
> set nbcore 4
> port stop all
> port config 0 dcb vt off 4 pfc on
> port start all
> port stop all
> port config all rxq 8
> port config all txq 8
> 
> At the moment, a segmentation fault occurs.
> 

Hi Lijun, Xiaoyun,

I read the thread and checked the code, but still I am not completely clear with 
the dcp implementation in testpmd.

But the "dcb_config = 0" in the 'stop_port()' that has been removed in the patch 
1/3 seems preventing these kind of config combination, with implicitly resetting 
the dcb feature with port stop.

What do you think just have the number of forwarding cores adjustment change in 
the patch 1/3, and keep the 'stop_port()' as it is, will it solve this problem, 
and how acceptable it will be do you think?

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

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

[Li, Xiaoyun]

> -----Original Message-----
> From: oulijun <oulijun@huawei.com>
> Sent: Friday, April 2, 2021 09:45
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from parsing
> Rx and Tx
> 
> 
> 
> 在 2021/3/29 9:53, Li, Xiaoyun 写道:
> >
> >
> >> -----Original Message-----
> >> From: oulijun <oulijun@huawei.com>
> >> Sent: Thursday, March 25, 2021 11:04
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; linuxarm@openeuler.org
> >> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from
> >> parsing Rx and Tx
> >>
> >>
> >>
> >> 在 2021/3/24 9:44, Li, Xiaoyun 写道:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: oulijun <oulijun@huawei.com>
> >>>> Sent: Wednesday, March 24, 2021 09:01
> >>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >>>> <ferruh.yigit@intel.com>
> >>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
> >>>> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from
> >>>> parsing Rx and Tx
> >>>>
> >>>>
> >>>>
> >>>> 在 2021/3/23 15:50, Li, Xiaoyun 写道:
> >>>>> Hi
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Lijun Ou <oulijun@huawei.com>
> >>>>>> Sent: Friday, March 5, 2021 18:22
> >>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
> >>>>>> linuxarm@openeuler.org
> >>>>>> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from
> >>>>>> parsing Rx and Tx
> >>>>>>
> >>>>>> From: Huisong Li <lihuisong@huawei.com>
> >>>>>>
> >>>>>
> >>>>> The commit message should be more simple and avoids grammar
> mistakes.
> >>>>>
> >>>> All right, I will simply it.
> >>>>>> The "fwd_config_setup()" function does release and apply for
> >>>>>> memory of forwarding flows, and re-establish these streams when
> >>>>>> rxq/txq or rxd/txd is changed. The function is also called by
> >>>>>> "start_packet_forwarding()" when user executes "start" cmd.
> >>>>>> All changes for rxq/txq or rxd/txd can be updated uniformly when
> >>>>>> this command is executed. Therefore, it is a little redundant in
> >>>>>> the
> >>>> "cmd_config_rx_tx_parsed"
> >>>>>> function.
> >>>>>
> >>>>> It's not redundant. This command may configure number of rxq/txq.
> >>>>> So the
> >>>> fwd streams map may change.
> >>>>> Then it's common to check the fwd streams after this command using
> >>>>> "show
> >>>> config fwd".
> >>>>> If you remove this fwd stream update, users can't get the correct
> >>>>> new fwd
> >>>> streams until they start the traffic.
> >>>>> But they may change a lot of things and want to check if the
> >>>>> setting is correct
> >>>> before they start the traffic.
> >>>>>
> >>>> Yes, you are right. It's really unfriendly.
> >>>>>>
> >>>>>> In addition, the forwarding stream under one TC is configured
> >>>>>> based on number of queues allocated to TC. And number of queues
> >>>>>> allocated to TC is updated after calling  "rte_eth_dev_configure"
> >>>>>> again. If the number of queues is reduced after configuring the
> >>>>>> DCB, and then, release and apply for stream memory, and
> >>>>>> reinitialize the forwarding stream under the DCB mode based on
> >>>>>> the old TC
> >> information.
> >>>>>> As a result, null pointer may be accessed.
> >>>>>
> >>>>> I think you should add "rte_eth_dev_configure " into
> >>>>> dcb_fwd_config_setup()
> >>>> before rte_eth_dev_get_dcb_info().
> >>>>>
> >>>>> And the commit message should be similar like the following:
> >>>>> Segment fault might happen after configuring queue number to less
> >>>>> because
> >>>> dcb_fwd_config_setup setup dcb based on old dcb info.
> >>>>> And dcb info can only update after rte_eth_dev_configure().
> >>>>> So this patch adds rte_eth_dev_configure() before
> >>>>> rte_eth_dev_get_dcb_info()
> >>>> to get updated dcb info to fix this issue.
> >>>>>
> >>>> Thank you for your advice. But the above adjustments may still not
> >>>> work for some drivers. The mapping between queues and TCs in these
> >>>> drivers is updated in the dev_start stage.
> >>>>
> >>>> I have an idea. We can move fwd_config_setup() to start_port(),
> >>>> which is called by main() and after starting ports This not only
> >>>> solves the segment fault, but also does not have the problem you
> >>>> mentioned above. I
> >> test it and it is ok.
> >>>>
> >>>> What do you think, xiaoyun?
> >>>
> >>> How can you fix the issue I mentioned?
> >>> You still need to start port first to see the updated fwd config.
> >> Yes. But it can make sure that users get the correct new fwd streams
> >> before executing the 'start' command.
> >>> And for those drivers, why does the mapping has to be updated in
> >>> dev_start
> >> stage?
> >>> What does it need? Can't it be moved to dev_configure?
> >> The framework does not require that the configuration parameters
> >> transferred by the dev_configure API must be updated in the interface
> >> of driver. The driver can  verify the correctness of these parameters
> >> in the interface, and then complete the update in the dev_start
> >> stage. It depends on the design and need of the driver. Maybe it's
> >> more appropriate to put it in dev_configure, but now we can't ask all these
> drivers to modify the operation.
> >
> > I still think it's driver's responsibility to configure DCB in dev_configure.
> >
> > Calling fwd_config_setup here is because this command changes queue
> number. Users just change queue number so want to check fwd setup. It's a
> normal and reasonable behavior.
> > Starting port will be called in many situations. I don't think it's appropriate to
> call fwd_config_setup every time calling port_start. And you can't expect users
> know the fwd setup only change after port_start.
> >
> > These are only my thoughts. If anyone else agrees you, they can give you ack.
> >>>>
> @Ferruh, what do you think?
> Currently, both ixgbe and i40e also have this problem. Even if testpmd can be
> modified according to Xiaoyun's suggestion, and the driver is not modified, this
> problem still exists in some drivers, such as ixgbe and hns3.

NO. I40e and ixgbe DON'T have the issue.
I40e tc mapping is done in dev_configure.
ixgbe tc mapping is always fixed due to hw design. It only needs dev_configure to get dcb_conf info. No matter you configure dcb or not, the mapping is always the same. You can check ixgbe_dev_get_dcb_info().

So for i40e and ixgbe, you only need to add dev_configure in dcb_fwd_config_setup().

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

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

[oulijun]

在 2021/3/29 9:53, Li, Xiaoyun 写道:
> 
> 
>> -----Original Message-----
>> From: oulijun <oulijun@huawei.com>
>> Sent: Thursday, March 25, 2021 11:04
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from parsing
>> Rx and Tx
>>
>>
>>
>> 在 2021/3/24 9:44, Li, Xiaoyun 写道:
>>>
>>>
>>>> -----Original Message-----
>>>> From: oulijun <oulijun@huawei.com>
>>>> Sent: Wednesday, March 24, 2021 09:01
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>> <ferruh.yigit@intel.com>
>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>>>> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from
>>>> parsing Rx and Tx
>>>>
>>>>
>>>>
>>>> 在 2021/3/23 15:50, Li, Xiaoyun 写道:
>>>>> Hi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lijun Ou <oulijun@huawei.com>
>>>>>> Sent: Friday, March 5, 2021 18:22
>>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
>>>>>> linuxarm@openeuler.org
>>>>>> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from
>>>>>> parsing Rx and Tx
>>>>>>
>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>
>>>>>
>>>>> The commit message should be more simple and avoids grammar mistakes.
>>>>>
>>>> All right, I will simply it.
>>>>>> The "fwd_config_setup()" function does release and apply for memory
>>>>>> of forwarding flows, and re-establish these streams when rxq/txq or
>>>>>> rxd/txd is changed. The function is also called by
>>>>>> "start_packet_forwarding()" when user executes "start" cmd.
>>>>>> All changes for rxq/txq or rxd/txd can be updated uniformly when
>>>>>> this command is executed. Therefore, it is a little redundant in
>>>>>> the
>>>> "cmd_config_rx_tx_parsed"
>>>>>> function.
>>>>>
>>>>> It's not redundant. This command may configure number of rxq/txq. So
>>>>> the
>>>> fwd streams map may change.
>>>>> Then it's common to check the fwd streams after this command using
>>>>> "show
>>>> config fwd".
>>>>> If you remove this fwd stream update, users can't get the correct
>>>>> new fwd
>>>> streams until they start the traffic.
>>>>> But they may change a lot of things and want to check if the setting
>>>>> is correct
>>>> before they start the traffic.
>>>>>
>>>> Yes, you are right. It's really unfriendly.
>>>>>>
>>>>>> In addition, the forwarding stream under one TC is configured based
>>>>>> on number of queues allocated to TC. And number of queues allocated
>>>>>> to TC is updated after calling  "rte_eth_dev_configure"
>>>>>> again. If the number of queues is reduced after configuring the
>>>>>> DCB, and then, release and apply for stream memory, and
>>>>>> reinitialize the forwarding stream under the DCB mode based on the old TC
>> information.
>>>>>> As a result, null pointer may be accessed.
>>>>>
>>>>> I think you should add "rte_eth_dev_configure " into
>>>>> dcb_fwd_config_setup()
>>>> before rte_eth_dev_get_dcb_info().
>>>>>
>>>>> And the commit message should be similar like the following:
>>>>> Segment fault might happen after configuring queue number to less
>>>>> because
>>>> dcb_fwd_config_setup setup dcb based on old dcb info.
>>>>> And dcb info can only update after rte_eth_dev_configure().
>>>>> So this patch adds rte_eth_dev_configure() before
>>>>> rte_eth_dev_get_dcb_info()
>>>> to get updated dcb info to fix this issue.
>>>>>
>>>> Thank you for your advice. But the above adjustments may still not
>>>> work for some drivers. The mapping between queues and TCs in these
>>>> drivers is updated in the dev_start stage.
>>>>
>>>> I have an idea. We can move fwd_config_setup() to start_port(), which
>>>> is called by main() and after starting ports This not only solves the
>>>> segment fault, but also does not have the problem you mentioned above. I
>> test it and it is ok.
>>>>
>>>> What do you think, xiaoyun?
>>>
>>> How can you fix the issue I mentioned?
>>> You still need to start port first to see the updated fwd config.
>> Yes. But it can make sure that users get the correct new fwd streams before
>> executing the 'start' command.
>>> And for those drivers, why does the mapping has to be updated in dev_start
>> stage?
>>> What does it need? Can't it be moved to dev_configure?
>> The framework does not require that the configuration parameters transferred
>> by the dev_configure API must be updated in the interface of driver. The driver
>> can  verify the correctness of these parameters in the interface, and then
>> complete the update in the dev_start stage. It depends on the design and need
>> of the driver. Maybe it's more appropriate to put it in dev_configure, but now
>> we can't ask all these drivers to modify the operation.
> 
> I still think it's driver's responsibility to configure DCB in dev_configure.
> 
> Calling fwd_config_setup here is because this command changes queue number. Users just change queue number so want to check fwd setup. It's a normal and reasonable behavior.
> Starting port will be called in many situations. I don't think it's appropriate to call fwd_config_setup every time calling port_start. And you can't expect users know the fwd setup only change after port_start.
> 
> These are only my thoughts. If anyone else agrees you, they can give you ack.
>>>>
@Ferruh, what do you think?
Currently, both ixgbe and i40e also have this problem. Even if testpmd  
can be modified according to Xiaoyun's suggestion,
and the driver is not modified, this problem still exists in some  
drivers, such as ixgbe and hns3.
>>>>
>>>>>>
>>>>>> Like:
>>>>>> set nbcore 4
>>>>>> port stop all
>>>>>> port config 0 dcb vt off 4 pfc on
>>>>>> port start all
>>>>>> port stop all
>>>>>> port config all rxq 8
>>>>>> port config all txq 8
>>>>>>
>>>>>> At the moment, a segmentation fault occurs.
>>>>>>
>>>>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration
>>>>>> settings")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>> ---
>>>>>> V1->V2:
>>>>>> - use stream instead of flow
>>>>>> ---
>>>>>>     app/test-pmd/cmdline.c | 2 --
>>>>>>     1 file changed, 2 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>>>>> 4df0c32..e316f5c 100644
>>>>>> --- a/app/test-pmd/cmdline.c
>>>>>> +++ b/app/test-pmd/cmdline.c
>>>>>> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
>>>>>>     		return;
>>>>>>     	}
>>>>>>
>>>>>> -	fwd_config_setup();
>>>>>> -
>>>>>>     	init_port_config();
>>>>>>
>>>>>>     	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>>>>> --
>>>>>> 2.7.4
>>>>>
>>>>> .
>>>>>
>>> .
>>>
> .
> 

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

[Li, Xiaoyun]

> -----Original Message-----
> From: oulijun <oulijun@huawei.com>
> Sent: Thursday, March 25, 2021 11:04
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; linuxarm@openeuler.org
> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from parsing
> Rx and Tx
> 
> 
> 
> 在 2021/3/24 9:44, Li, Xiaoyun 写道:
> >
> >
> >> -----Original Message-----
> >> From: oulijun <oulijun@huawei.com>
> >> Sent: Wednesday, March 24, 2021 09:01
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; linuxarm@openeuler.org
> >> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from
> >> parsing Rx and Tx
> >>
> >>
> >>
> >> 在 2021/3/23 15:50, Li, Xiaoyun 写道:
> >>> Hi
> >>>
> >>>> -----Original Message-----
> >>>> From: Lijun Ou <oulijun@huawei.com>
> >>>> Sent: Friday, March 5, 2021 18:22
> >>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
> >>>> linuxarm@openeuler.org
> >>>> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from
> >>>> parsing Rx and Tx
> >>>>
> >>>> From: Huisong Li <lihuisong@huawei.com>
> >>>>
> >>>
> >>> The commit message should be more simple and avoids grammar mistakes.
> >>>
> >> All right, I will simply it.
> >>>> The "fwd_config_setup()" function does release and apply for memory
> >>>> of forwarding flows, and re-establish these streams when rxq/txq or
> >>>> rxd/txd is changed. The function is also called by
> >>>> "start_packet_forwarding()" when user executes "start" cmd.
> >>>> All changes for rxq/txq or rxd/txd can be updated uniformly when
> >>>> this command is executed. Therefore, it is a little redundant in
> >>>> the
> >> "cmd_config_rx_tx_parsed"
> >>>> function.
> >>>
> >>> It's not redundant. This command may configure number of rxq/txq. So
> >>> the
> >> fwd streams map may change.
> >>> Then it's common to check the fwd streams after this command using
> >>> "show
> >> config fwd".
> >>> If you remove this fwd stream update, users can't get the correct
> >>> new fwd
> >> streams until they start the traffic.
> >>> But they may change a lot of things and want to check if the setting
> >>> is correct
> >> before they start the traffic.
> >>>
> >> Yes, you are right. It's really unfriendly.
> >>>>
> >>>> In addition, the forwarding stream under one TC is configured based
> >>>> on number of queues allocated to TC. And number of queues allocated
> >>>> to TC is updated after calling  "rte_eth_dev_configure"
> >>>> again. If the number of queues is reduced after configuring the
> >>>> DCB, and then, release and apply for stream memory, and
> >>>> reinitialize the forwarding stream under the DCB mode based on the old TC
> information.
> >>>> As a result, null pointer may be accessed.
> >>>
> >>> I think you should add "rte_eth_dev_configure " into
> >>> dcb_fwd_config_setup()
> >> before rte_eth_dev_get_dcb_info().
> >>>
> >>> And the commit message should be similar like the following:
> >>> Segment fault might happen after configuring queue number to less
> >>> because
> >> dcb_fwd_config_setup setup dcb based on old dcb info.
> >>> And dcb info can only update after rte_eth_dev_configure().
> >>> So this patch adds rte_eth_dev_configure() before
> >>> rte_eth_dev_get_dcb_info()
> >> to get updated dcb info to fix this issue.
> >>>
> >> Thank you for your advice. But the above adjustments may still not
> >> work for some drivers. The mapping between queues and TCs in these
> >> drivers is updated in the dev_start stage.
> >>
> >> I have an idea. We can move fwd_config_setup() to start_port(), which
> >> is called by main() and after starting ports This not only solves the
> >> segment fault, but also does not have the problem you mentioned above. I
> test it and it is ok.
> >>
> >> What do you think, xiaoyun?
> >
> > How can you fix the issue I mentioned?
> > You still need to start port first to see the updated fwd config.
> Yes. But it can make sure that users get the correct new fwd streams before
> executing the 'start' command.
> > And for those drivers, why does the mapping has to be updated in dev_start
> stage?
> > What does it need? Can't it be moved to dev_configure?
> The framework does not require that the configuration parameters transferred
> by the dev_configure API must be updated in the interface of driver. The driver
> can  verify the correctness of these parameters in the interface, and then
> complete the update in the dev_start stage. It depends on the design and need
> of the driver. Maybe it's more appropriate to put it in dev_configure, but now
> we can't ask all these drivers to modify the operation.

I still think it's driver's responsibility to configure DCB in dev_configure.

Calling fwd_config_setup here is because this command changes queue number. Users just change queue number so want to check fwd setup. It's a normal and reasonable behavior.
Starting port will be called in many situations. I don't think it's appropriate to call fwd_config_setup every time calling port_start. And you can't expect users know the fwd setup only change after port_start.

These are only my thoughts. If anyone else agrees you, they can give you ack.
> >>
> >>
> >>>>
> >>>> Like:
> >>>> set nbcore 4
> >>>> port stop all
> >>>> port config 0 dcb vt off 4 pfc on
> >>>> port start all
> >>>> port stop all
> >>>> port config all rxq 8
> >>>> port config all txq 8
> >>>>
> >>>> At the moment, a segmentation fault occurs.
> >>>>
> >>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration
> >>>> settings")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>> ---
> >>>> V1->V2:
> >>>> - use stream instead of flow
> >>>> ---
> >>>>    app/test-pmd/cmdline.c | 2 --
> >>>>    1 file changed, 2 deletions(-)
> >>>>
> >>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> >>>> 4df0c32..e316f5c 100644
> >>>> --- a/app/test-pmd/cmdline.c
> >>>> +++ b/app/test-pmd/cmdline.c
> >>>> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
> >>>>    		return;
> >>>>    	}
> >>>>
> >>>> -	fwd_config_setup();
> >>>> -
> >>>>    	init_port_config();
> >>>>
> >>>>    	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> >>>> --
> >>>> 2.7.4
> >>>
> >>> .
> >>>
> > .
> >

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

[oulijun]

在 2021/3/24 9:44, Li, Xiaoyun 写道:
> 
> 
>> -----Original Message-----
>> From: oulijun <oulijun@huawei.com>
>> Sent: Wednesday, March 24, 2021 09:01
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>> Subject: Re: [PATCH 2/3] app/testpmd: remove forwarding config from parsing
>> Rx and Tx
>>
>>
>>
>> 在 2021/3/23 15:50, Li, Xiaoyun 写道:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Lijun Ou <oulijun@huawei.com>
>>>> Sent: Friday, March 5, 2021 18:22
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
>>>> linuxarm@openeuler.org
>>>> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from
>>>> parsing Rx and Tx
>>>>
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>
>>> The commit message should be more simple and avoids grammar mistakes.
>>>
>> All right, I will simply it.
>>>> The "fwd_config_setup()" function does release and apply for memory
>>>> of forwarding flows, and re-establish these streams when rxq/txq or
>>>> rxd/txd is changed. The function is also called by
>>>> "start_packet_forwarding()" when user executes "start" cmd.
>>>> All changes for rxq/txq or rxd/txd can be updated uniformly when this
>>>> command is executed. Therefore, it is a little redundant in the
>> "cmd_config_rx_tx_parsed"
>>>> function.
>>>
>>> It's not redundant. This command may configure number of rxq/txq. So the
>> fwd streams map may change.
>>> Then it's common to check the fwd streams after this command using "show
>> config fwd".
>>> If you remove this fwd stream update, users can't get the correct new fwd
>> streams until they start the traffic.
>>> But they may change a lot of things and want to check if the setting is correct
>> before they start the traffic.
>>>
>> Yes, you are right. It's really unfriendly.
>>>>
>>>> In addition, the forwarding stream under one TC is configured based
>>>> on number of queues allocated to TC. And number of queues allocated
>>>> to TC is updated after calling  "rte_eth_dev_configure"
>>>> again. If the number of queues is reduced after configuring the DCB,
>>>> and then, release and apply for stream memory, and reinitialize the
>>>> forwarding stream under the DCB mode based on the old TC information.
>>>> As a result, null pointer may be accessed.
>>>
>>> I think you should add "rte_eth_dev_configure " into dcb_fwd_config_setup()
>> before rte_eth_dev_get_dcb_info().
>>>
>>> And the commit message should be similar like the following:
>>> Segment fault might happen after configuring queue number to less because
>> dcb_fwd_config_setup setup dcb based on old dcb info.
>>> And dcb info can only update after rte_eth_dev_configure().
>>> So this patch adds rte_eth_dev_configure() before rte_eth_dev_get_dcb_info()
>> to get updated dcb info to fix this issue.
>>>
>> Thank you for your advice. But the above adjustments may still not work for
>> some drivers. The mapping between queues and TCs in these drivers is updated
>> in the dev_start stage.
>>
>> I have an idea. We can move fwd_config_setup() to start_port(), which is called
>> by main() and after starting ports This not only solves the segment fault, but also
>> does not have the problem you mentioned above. I test it and it is ok.
>>
>> What do you think, xiaoyun?
> 
> How can you fix the issue I mentioned?
> You still need to start port first to see the updated fwd config.
Yes. But it can make sure that users get the correct new fwd streams  
before executing the 'start' command.
> And for those drivers, why does the mapping has to be updated in dev_start stage?
> What does it need? Can't it be moved to dev_configure?
The framework does not require that the configuration parameters  
transferred by the dev_configure API must
be updated in the interface of driver. The driver can  verify the  
correctness of these parameters in the interface,
and then complete the update in the dev_start stage. It depends on the  
design and need of the driver. Maybe
it's more appropriate to put it in dev_configure, but now we can't ask  
all these drivers to modify the operation.
>>
>>
>>>>
>>>> Like:
>>>> set nbcore 4
>>>> port stop all
>>>> port config 0 dcb vt off 4 pfc on
>>>> port start all
>>>> port stop all
>>>> port config all rxq 8
>>>> port config all txq 8
>>>>
>>>> At the moment, a segmentation fault occurs.
>>>>
>>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> ---
>>>> V1->V2:
>>>> - use stream instead of flow
>>>> ---
>>>>    app/test-pmd/cmdline.c | 2 --
>>>>    1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>>> 4df0c32..e316f5c 100644
>>>> --- a/app/test-pmd/cmdline.c
>>>> +++ b/app/test-pmd/cmdline.c
>>>> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
>>>>    		return;
>>>>    	}
>>>>
>>>> -	fwd_config_setup();
>>>> -
>>>>    	init_port_config();
>>>>
>>>>    	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>>> --
>>>> 2.7.4
>>>
>>> .
>>>
> .
> 

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

[Li, Xiaoyun]

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

How can you fix the issue I mentioned?
You still need to start port first to see the updated fwd config.
And for those drivers, why does the mapping has to be updated in dev_start stage?
What does it need? Can't it be moved to dev_configure?
> 
> 
> >>
> >> Like:
> >> set nbcore 4
> >> port stop all
> >> port config 0 dcb vt off 4 pfc on
> >> port start all
> >> port stop all
> >> port config all rxq 8
> >> port config all txq 8
> >>
> >> At the moment, a segmentation fault occurs.
> >>
> >> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >> ---
> >> V1->V2:
> >> - use stream instead of flow
> >> ---
> >>   app/test-pmd/cmdline.c | 2 --
> >>   1 file changed, 2 deletions(-)
> >>
> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> >> 4df0c32..e316f5c 100644
> >> --- a/app/test-pmd/cmdline.c
> >> +++ b/app/test-pmd/cmdline.c
> >> @@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
> >>   		return;
> >>   	}
> >>
> >> -	fwd_config_setup();
> >> -
> >>   	init_port_config();
> >>
> >>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> >> --
> >> 2.7.4
> >
> > .
> >

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

[oulijun]

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

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

What do you think, xiaoyun?


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

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

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Lijun Ou <oulijun@huawei.com>
> Sent: Friday, March 5, 2021 18:22
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
> linuxarm@openeuler.org
> Subject: [PATCH 2/3] app/testpmd: remove forwarding config from parsing Rx
> and Tx
> 
> From: Huisong Li <lihuisong@huawei.com>
> 

The commit message should be more simple and avoids grammar mistakes.

> The "fwd_config_setup()" function does release and apply for memory of
> forwarding flows, and re-establish these streams when rxq/txq or rxd/txd is
> changed. The function is also called by "start_packet_forwarding()" when user
> executes "start" cmd.
> All changes for rxq/txq or rxd/txd can be updated uniformly when this command
> is executed. Therefore, it is a little redundant in the "cmd_config_rx_tx_parsed"
> function.

It's not redundant. This command may configure number of rxq/txq. So the fwd streams map may change.
Then it's common to check the fwd streams after this command using "show config fwd".
If you remove this fwd stream update, users can't get the correct new fwd streams until they start the traffic.
But they may change a lot of things and want to check if the setting is correct before they start the traffic.

> 
> In addition, the forwarding stream under one TC is configured based on number
> of queues allocated to TC. And number of queues allocated to TC is updated
> after calling  "rte_eth_dev_configure"
> again. If the number of queues is reduced after configuring the DCB, and then,
> release and apply for stream memory, and reinitialize the forwarding stream
> under the DCB mode based on the old TC information. As a result, null pointer
> may be accessed.

I think you should add "rte_eth_dev_configure " into dcb_fwd_config_setup() before rte_eth_dev_get_dcb_info().

And the commit message should be similar like the following:
Segment fault might happen after configuring queue number to less because dcb_fwd_config_setup setup dcb based on old dcb info.
And dcb info can only update after rte_eth_dev_configure().
So this patch adds rte_eth_dev_configure() before rte_eth_dev_get_dcb_info() to get updated dcb info to fix this issue.

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

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

[Lijun Ou]

From: Huisong Li <lihuisong@huawei.com>

The "fwd_config_setup()" function does release and apply for
memory of forwarding flows, and re-establish these streams when
rxq/txq or rxd/txd is changed. The function is also called by
"start_packet_forwarding()" when user executes "start" cmd.
All changes for rxq/txq or rxd/txd can be updated uniformly
when this command is executed. Therefore, it is a little
redundant in the "cmd_config_rx_tx_parsed" function.

In addition, the forwarding stream under one TC is configured
based on number of queues allocated to TC. And number of queues
allocated to TC is updated after calling  "rte_eth_dev_configure"
again. If the number of queues is reduced after configuring the
DCB, and then, release and apply for stream memory, and reinitialize
the forwarding stream under the DCB mode based on the old TC
information. As a result, null pointer may be accessed.

Like:
set nbcore 4
port stop all
port config 0 dcb vt off 4 pfc on
port start all
port stop all
port config all rxq 8
port config all txq 8

At the moment, a segmentation fault occurs.

Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V1->V2:
- use stream instead of flow
---
 app/test-pmd/cmdline.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 4df0c32..e316f5c 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1837,8 +1837,6 @@ cmd_config_rx_tx_parsed(void *parsed_result,
 		return;
 	}
 
-	fwd_config_setup();
-
 	init_port_config();
 
 	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
-- 
2.7.4