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

[dpdk-dev] [PATCH] 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

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

[Li, Xiaoyun]

Hi
Sorry, forgot to send this in last patchset.

> -----Original Message-----
> From: Lijun Ou <oulijun@huawei.com>
> Sent: Friday, March 5, 2021 15:33
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
> linuxarm@openeuler.org
> Subject: [PATCH] app/testpmd: support Tx mbuf free on demand cmd
> 
> 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

Users ...... this command is executed.

> 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);

Tx? You just want to send a patch to use Rx/Tx. You should keep concurrency.

> +		return;
> +	}
> +
> +	if (dev->data->tx_queue_state[queue_id] !=
> +		RTE_ETH_QUEUE_STATE_STARTED) {
> +		printf("TX queue_id %u not started!\n", queue_id);

Tx?

> +		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.
> +	 */

And I have some concerns on this. Users cannot know about this unless they read your code. I don't think this will likely happen.
So you should document this in testpmd doc when you introduced this command.

Or maybe you can have a way to stop that to happen. Maybe checking "test_done && engine != rxonly" before this?
In your comment, the tx forwarding shouldn't happen, right? Every fwd engine except rxonly will contain tx.
Well, it's still a rough way because tx may not happen even if it's io fwd.

Maybe other people have better ideas. Otherwise, you should at least document it.
@Yigit, Ferruh What do you think? Is there a better way?

> +	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 "

Tx?

> +		       "error desc: %s(%d)\n",
> +		       port_id, queue_id, strerror(-ret), ret);
> +		return;
> +	}
> +
> +	printf("Cleanup port %u TX queue %u mbuf nums: %u\n",

Tx?

> +	       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 V2] 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)

Users 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)
this command executed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V1->V2:
- use Tx instead of TX
- add note in doc
---
 app/test-pmd/cmdline.c                      | 91 +++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_21_05.rst      |  2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
 3 files changed, 104 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 14110eb..4df0c32 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..fa49e58 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -272,6 +272,17 @@ 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)
+
+.. note::
+   This command is dangerous, users must make sure there are no cucurrent access to
+   the same Tx queue (link rte_eth_tx_burst, rte_eth_dev_tx_queue_stop and so on).
+
 show config
 ~~~~~~~~~~~
 
-- 
2.7.4

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

[oulijun]

在 2021/3/5 15:46, Li, Xiaoyun 写道:
> Hi
> Sorry, forgot to send this in last patchset.
>
>> -----Original Message-----
>> From: Lijun Ou <oulijun@huawei.com>
>> Sent: Friday, March 5, 2021 15:33
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
>> linuxarm@openeuler.org
>> Subject: [PATCH] app/testpmd: support Tx mbuf free on demand cmd
>>
>> 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
> Users ...... this command is executed.
OK， I will fix.
>> 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);
> Tx? You just want to send a patch to use Rx/Tx. You should keep concurrency.
Good idea. I will fix it.
>> +		return;
>> +	}
>> +
>> +	if (dev->data->tx_queue_state[queue_id] !=
>> +		RTE_ETH_QUEUE_STATE_STARTED) {
>> +		printf("TX queue_id %u not started!\n", queue_id);
> Tx?
>
>> +		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.
>> +	 */
> And I have some concerns on this. Users cannot know about this unless they read your code. I don't think this will likely happen.
> So you should document this in testpmd doc when you introduced this command.
>
> Or maybe you can have a way to stop that to happen. Maybe checking "test_done && engine != rxonly" before this?
> In your comment, the tx forwarding shouldn't happen, right? Every fwd engine except rxonly will contain tx.
> Well, it's still a rough way because tx may not happen even if it's io fwd.
>
> Maybe other people have better ideas. Otherwise, you should at least document it.
> @Yigit, Ferruh What do you think? Is there a better way?
I have add doc for noting.
>> +	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 "
> Tx?
>
>> +		       "error desc: %s(%d)\n",
>> +		       port_id, queue_id, strerror(-ret), ret);
>> +		return;
>> +	}
>> +
>> +	printf("Cleanup port %u TX queue %u mbuf nums: %u\n",
> Tx?
>
>> +	       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
> .
>

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

[Ferruh Yigit]

On 3/5/2021 9:57 AM, Lijun Ou wrote:
> From: Chengwen Feng <fengchengwen@huawei.com>
> 
> This patch support tx_done_cleanup command:
> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
> 
> Users 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)
> this command executed.
> 

Hi Lijun,

Is the intention to test the PMD implementation?
As you highlighted the API is for the datapath, a command for it is not easy to 
use, not sure how useful it will be.
Perhaps it can be option to use this API in a forwarding engine, like 'txonly', 
controlled by a command, but again not sure what to observe/measure etc..

> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V1->V2:
> - use Tx instead of TX
> - add note in doc
> ---
>   app/test-pmd/cmdline.c                      | 91 +++++++++++++++++++++++++++++
>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>   3 files changed, 104 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 14110eb..4df0c32 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>

This header for PMDs to include, applications shouldn't include this, including 
this means you are accessing dpdk internals which you shouldn't access.

>   #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];

Similar to above comment 'rte_eth_devices' is the internal variable, 
applications should not access it directly.

> +	if (queue_id >= dev->data->nb_tx_queues) {
> +		printf("Invalid Tx queue_id %u\n", queue_id);
> +		return;
> +	}
> +

Number of the queues can be get via 'rte_eth_dev_info_get()'.

> +	if (dev->data->tx_queue_state[queue_id] !=
> +		RTE_ETH_QUEUE_STATE_STARTED) {
> +		printf("Tx queue_id %u not started!\n", queue_id);
> +		return;
> +	}
> +

I am not sure if we have an API to get this information, if there is a need from 
application to get this data we should consider adding a new API. cc'ed Andrew 
and Thomas.

> +	/*
> +	 * 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..fa49e58 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -272,6 +272,17 @@ 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)
> +
> +.. note::
> +   This command is dangerous, users must make sure there are no cucurrent access to
> +   the same Tx queue (link rte_eth_tx_burst, rte_eth_dev_tx_queue_stop and so on).
> +
>   show config
>   ~~~~~~~~~~~
>   
> 

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

[oulijun]

在 2021/3/9 1:33, Ferruh Yigit 写道:
> On 3/5/2021 9:57 AM, Lijun Ou wrote:
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>
>> This patch support tx_done_cleanup command:
>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>
>> Users 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)
>> this command executed.
>>
> 
> Hi Lijun,
> 
> Is the intention to test the PMD implementation?
Yes
> As you highlighted the API is for the datapath, a command for it is not 
> easy to use, not sure how useful it will be.
> Perhaps it can be option to use this API in a forwarding engine, like 
> 'txonly', controlled by a command, but again not sure what to 
> observe/measure etc..
> 
We want to do this. But it is diffcult to control the number of sent 
packets when used together with other txonly.
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V1->V2:
>> - use Tx instead of TX
>> - add note in doc
>> ---
>>   app/test-pmd/cmdline.c                      | 91 
>> +++++++++++++++++++++++++++++
>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>   3 files changed, 104 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 14110eb..4df0c32 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>
> 
> This header for PMDs to include, applications shouldn't include this, 
> including this means you are accessing dpdk internals which you 
> shouldn't access.
> 
Thanks. I will fix it.
>>   #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];
> 
> Similar to above comment 'rte_eth_devices' is the internal variable, 
> applications should not access it directly.
> 
No API is available, and multiple references exist in the testpmd file.
>> +    if (queue_id >= dev->data->nb_tx_queues) {
>> +        printf("Invalid Tx queue_id %u\n", queue_id);
>> +        return;
>> +    }
>> +
> 
> Number of the queues can be get via 'rte_eth_dev_info_get()'.
> 
This is also called in txonly. Do you want to replace it?
>> +    if (dev->data->tx_queue_state[queue_id] !=
>> +        RTE_ETH_QUEUE_STATE_STARTED) {
>> +        printf("Tx queue_id %u not started!\n", queue_id);
>> +        return;
>> +    }
>> +
> 
> I am not sure if we have an API to get this information, if there is a 
> need from application to get this data we should consider adding a new 
> API. cc'ed Andrew and Thomas.
> 
>> +    /*
>> +     * 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..fa49e58 100644
>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> @@ -272,6 +272,17 @@ 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)
>> +
>> +.. note::
>> +   This command is dangerous, users must make sure there are no 
>> cucurrent access to
>> +   the same Tx queue (link rte_eth_tx_burst, 
>> rte_eth_dev_tx_queue_stop and so on).
>> +
>>   show config
>>   ~~~~~~~~~~~
>>
> 
> .
> 

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

[Ferruh Yigit]

On 3/9/2021 8:49 AM, oulijun wrote:
> 
> 
> 在 2021/3/9 1:33, Ferruh Yigit 写道:
>> On 3/5/2021 9:57 AM, Lijun Ou wrote:
>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>
>>> This patch support tx_done_cleanup command:
>>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>>
>>> Users 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)
>>> this command executed.
>>>
>>
>> Hi Lijun,
>>
>> Is the intention to test the PMD implementation?
> Yes
>> As you highlighted the API is for the datapath, a command for it is not easy 
>> to use, not sure how useful it will be.
>> Perhaps it can be option to use this API in a forwarding engine, like 
>> 'txonly', controlled by a command, but again not sure what to observe/measure 
>> etc..
>>
> We want to do this. But it is diffcult to control the number of sent packets 
> when used together with other txonly.

Agree hard to verify that the implementation this way.

What do you think adding an unit test for it, 'app/test/test_ethdev_xx', that 
can send some packets get the free mbufs number, call the 
'rte_eth_tx_done_cleanup()' and check the free mbuf numbers again and return a 
fail/success accordingly.

And this can be a good start for our long missing ethdev unit tests, cc'ed Aaron 
and Honnappa for the unit test perspective.

And if we go with unit test, I think we need to find a way to mark the unit 
tests that requires HW (this case) for the automation usecases.

>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> ---
>>> V1->V2:
>>> - use Tx instead of TX
>>> - add note in doc
>>> ---
>>>   app/test-pmd/cmdline.c                      | 91 +++++++++++++++++++++++++++++
>>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>>   3 files changed, 104 insertions(+)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index 14110eb..4df0c32 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>
>>
>> This header for PMDs to include, applications shouldn't include this, 
>> including this means you are accessing dpdk internals which you shouldn't access.
>>
> Thanks. I will fix it.
>>>   #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];
>>
>> Similar to above comment 'rte_eth_devices' is the internal variable, 
>> applications should not access it directly.
>>
> No API is available, and multiple references exist in the testpmd file.

Technically 'rte_eth_devices' is still visible to the applications because of 
the static inline functions, in theory it should be hidden.

But this variable accessed by our test application multiple times may be the 
sign that something more is missing.

Thomas, Andrew, what to you think to try to clean this usage from testpmd and 
add more APIs if needed for this?

>>> +    if (queue_id >= dev->data->nb_tx_queues) {
>>> +        printf("Invalid Tx queue_id %u\n", queue_id);
>>> +        return;
>>> +    }
>>> +
>>
>> Number of the queues can be get via 'rte_eth_dev_info_get()'.
>>
> This is also called in txonly. Do you want to replace it?

That would be good if you can do it in a separate patch, thank you.

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

[Thomas Monjalon]

09/03/2021 10:53, Ferruh Yigit:
> On 3/9/2021 8:49 AM, oulijun wrote:
> > 2021/3/9 1:33, Ferruh Yigit:
> >> Similar to above comment 'rte_eth_devices' is the internal variable, 
> >> applications should not access it directly.
> >>
> > No API is available, and multiple references exist in the testpmd file.
> 
> Technically 'rte_eth_devices' is still visible to the applications because of 
> the static inline functions, in theory it should be hidden.
> 
> But this variable accessed by our test application multiple times may be the 
> sign that something more is missing.
> 
> Thomas, Andrew, what to you think to try to clean this usage from testpmd and 
> add more APIs if needed for this?

I fully agree.
The test applications and examples should help identifying gaps
in the libraries. So we should not workaround the official API.

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

[Andrew Rybchenko]

On 3/9/21 12:57 PM, Thomas Monjalon wrote:
> 09/03/2021 10:53, Ferruh Yigit:
>> On 3/9/2021 8:49 AM, oulijun wrote:
>>> 2021/3/9 1:33, Ferruh Yigit:
>>>> Similar to above comment 'rte_eth_devices' is the internal variable, 
>>>> applications should not access it directly.
>>>>
>>> No API is available, and multiple references exist in the testpmd file.
>>
>> Technically 'rte_eth_devices' is still visible to the applications because of 
>> the static inline functions, in theory it should be hidden.
>>
>> But this variable accessed by our test application multiple times may be the 
>> sign that something more is missing.
>>
>> Thomas, Andrew, what to you think to try to clean this usage from testpmd and 
>> add more APIs if needed for this?
> 
> I fully agree.
> The test applications and examples should help identifying gaps
> in the libraries. So we should not workaround the official API.
> 

+1

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

[Aaron Conole]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> On 3/9/2021 8:49 AM, oulijun wrote:
>>
>>
>> 在 2021/3/9 1:33, Ferruh Yigit 写道:
>>> On 3/5/2021 9:57 AM, Lijun Ou wrote:
>>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>>
>>>> This patch support tx_done_cleanup command:
>>>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>>>
>>>> Users 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)
>>>> this command executed.
>>>>
>>>
>>> Hi Lijun,
>>>
>>> Is the intention to test the PMD implementation?
>> Yes
>>> As you highlighted the API is for the datapath, a command for it is
>>> not easy to use, not sure how useful it will be.
>>> Perhaps it can be option to use this API in a forwarding engine,
>>> like 'txonly', controlled by a command, but again not sure what to
>>> observe/measure etc..
>>>
>> We want to do this. But it is diffcult to control the number of sent
>> packets when used together with other txonly.
>
> Agree hard to verify that the implementation this way.
>
> What do you think adding an unit test for it,
> 'app/test/test_ethdev_xx', that can send some packets get the free
> mbufs number, call the 'rte_eth_tx_done_cleanup()' and check the free
> mbuf numbers again and return a fail/success accordingly.
>
> And this can be a good start for our long missing ethdev unit tests,
> cc'ed Aaron and Honnappa for the unit test perspective.

Definitely, let's do it.  There's a start to a guide detailing how to
create unit tests and suites ;).  I'll post the latest version this week.

> And if we go with unit test, I think we need to find a way to mark the
> unit tests that requires HW (this case) for the automation usecases.

Does it really need HW, though?  Can we use a software device for it?
Maybe it's a good time to use the null dev for test purposes for these
libraries.  After all, we want to be testing the library.

>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> ---
>>>> V1->V2:
>>>> - use Tx instead of TX
>>>> - add note in doc
>>>> ---
>>>>   app/test-pmd/cmdline.c                      | 91 +++++++++++++++++++++++++++++
>>>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>>>   3 files changed, 104 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>> index 14110eb..4df0c32 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>
>>>
>>> This header for PMDs to include, applications shouldn't include
>>> this, including this means you are accessing dpdk internals which
>>> you shouldn't access.
>>>
>> Thanks. I will fix it.
>>>>   #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];
>>>
>>> Similar to above comment 'rte_eth_devices' is the internal
>>> variable, applications should not access it directly.
>>>
>> No API is available, and multiple references exist in the testpmd file.
>
> Technically 'rte_eth_devices' is still visible to the applications
> because of the static inline functions, in theory it should be hidden.
>
> But this variable accessed by our test application multiple times may
> be the sign that something more is missing.
>
> Thomas, Andrew, what to you think to try to clean this usage from
> testpmd and add more APIs if needed for this?
>
>>>> +    if (queue_id >= dev->data->nb_tx_queues) {
>>>> +        printf("Invalid Tx queue_id %u\n", queue_id);
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> Number of the queues can be get via 'rte_eth_dev_info_get()'.
>>>
>> This is also called in txonly. Do you want to replace it?
>
> That would be good if you can do it in a separate patch, thank you.

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

[Ferruh Yigit]

On 3/9/2021 2:00 PM, Aaron Conole wrote:
> Ferruh Yigit <ferruh.yigit@intel.com> writes:
> 
>> On 3/9/2021 8:49 AM, oulijun wrote:
>>>
>>>
>>> 在 2021/3/9 1:33, Ferruh Yigit 写道:
>>>> On 3/5/2021 9:57 AM, Lijun Ou wrote:
>>>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>>>
>>>>> This patch support tx_done_cleanup command:
>>>>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>>>>
>>>>> Users 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)
>>>>> this command executed.
>>>>>
>>>>
>>>> Hi Lijun,
>>>>
>>>> Is the intention to test the PMD implementation?
>>> Yes
>>>> As you highlighted the API is for the datapath, a command for it is
>>>> not easy to use, not sure how useful it will be.
>>>> Perhaps it can be option to use this API in a forwarding engine,
>>>> like 'txonly', controlled by a command, but again not sure what to
>>>> observe/measure etc..
>>>>
>>> We want to do this. But it is diffcult to control the number of sent
>>> packets when used together with other txonly.
>>
>> Agree hard to verify that the implementation this way.
>>
>> What do you think adding an unit test for it,
>> 'app/test/test_ethdev_xx', that can send some packets get the free
>> mbufs number, call the 'rte_eth_tx_done_cleanup()' and check the free
>> mbuf numbers again and return a fail/success accordingly.
>>
>> And this can be a good start for our long missing ethdev unit tests,
>> cc'ed Aaron and Honnappa for the unit test perspective.
> 
> Definitely, let's do it.  There's a start to a guide detailing how to
> create unit tests and suites ;).  I'll post the latest version this week.
> 
>> And if we go with unit test, I think we need to find a way to mark the
>> unit tests that requires HW (this case) for the automation usecases.
> 
> Does it really need HW, though?  Can we use a software device for it?
> Maybe it's a good time to use the null dev for test purposes for these
> libraries.  After all, we want to be testing the library.
> 

For some cases we can use virtual devices, but for this case the actual 
intention is to test the PMD implementation.

Overall I guess we need both, the HW dependent and independent unit tests.

>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>> ---
>>>>> V1->V2:
>>>>> - use Tx instead of TX
>>>>> - add note in doc
>>>>> ---
>>>>>    app/test-pmd/cmdline.c                      | 91 +++++++++++++++++++++++++++++
>>>>>    doc/guides/rel_notes/release_21_05.rst      |  2 +
>>>>>    doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>>>>    3 files changed, 104 insertions(+)
>>>>>
>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>>> index 14110eb..4df0c32 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>
>>>>
>>>> This header for PMDs to include, applications shouldn't include
>>>> this, including this means you are accessing dpdk internals which
>>>> you shouldn't access.
>>>>
>>> Thanks. I will fix it.
>>>>>    #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];
>>>>
>>>> Similar to above comment 'rte_eth_devices' is the internal
>>>> variable, applications should not access it directly.
>>>>
>>> No API is available, and multiple references exist in the testpmd file.
>>
>> Technically 'rte_eth_devices' is still visible to the applications
>> because of the static inline functions, in theory it should be hidden.
>>
>> But this variable accessed by our test application multiple times may
>> be the sign that something more is missing.
>>
>> Thomas, Andrew, what to you think to try to clean this usage from
>> testpmd and add more APIs if needed for this?
>>
>>>>> +    if (queue_id >= dev->data->nb_tx_queues) {
>>>>> +        printf("Invalid Tx queue_id %u\n", queue_id);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>
>>>> Number of the queues can be get via 'rte_eth_dev_info_get()'.
>>>>
>>> This is also called in txonly. Do you want to replace it?
>>
>> That would be good if you can do it in a separate patch, thank you.
> 

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

[oulijun]

在 2021/3/9 17:53, Ferruh Yigit 写道:
> On 3/9/2021 8:49 AM, oulijun wrote:
>>
>>
>> 在 2021/3/9 1:33, Ferruh Yigit 写道:
>>> On 3/5/2021 9:57 AM, Lijun Ou wrote:
>>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>>
>>>> This patch support tx_done_cleanup command:
>>>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>>>
>>>> Users 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)
>>>> this command executed.
>>>>
>>>
>>> Hi Lijun,
>>>
>>> Is the intention to test the PMD implementation?
>> Yes
>>> As you highlighted the API is for the datapath, a command for it is 
>>> not easy to use, not sure how useful it will be.
>>> Perhaps it can be option to use this API in a forwarding engine, like 
>>> 'txonly', controlled by a command, but again not sure what to 
>>> observe/measure etc..
>>>
>> We want to do this. But it is diffcult to control the number of sent 
>> packets when used together with other txonly.
> 
> Agree hard to verify that the implementation this way.
> 
> What do you think adding an unit test for it, 'app/test/test_ethdev_xx', 
> that can send some packets get the free mbufs number, call the 
> 'rte_eth_tx_done_cleanup()' and check the free mbuf numbers again and 
> return a fail/success accordingly.
> 
> And this can be a good start for our long missing ethdev unit tests, 
> cc'ed Aaron and Honnappa for the unit test perspective.
> 
> And if we go with unit test, I think we need to find a way to mark the 
> unit tests that requires HW (this case) for the automation usecases.
> 
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> ---
>>>> V1->V2:
>>>> - use Tx instead of TX
>>>> - add note in doc
>>>> ---
>>>>   app/test-pmd/cmdline.c                      | 91 
>>>> +++++++++++++++++++++++++++++
>>>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>>>   3 files changed, 104 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>> index 14110eb..4df0c32 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>
>>>
>>> This header for PMDs to include, applications shouldn't include this, 
>>> including this means you are accessing dpdk internals which you 
>>> shouldn't access.
>>>
>> Thanks. I will fix it.
>>>>   #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];
>>>
>>> Similar to above comment 'rte_eth_devices' is the internal variable, 
>>> applications should not access it directly.
>>>
>> No API is available, and multiple references exist in the testpmd file.
> 
> Technically 'rte_eth_devices' is still visible to the applications 
> because of the static inline functions, in theory it should be hidden.
> 
> But this variable accessed by our test application multiple times may be 
> the sign that something more is missing.
> 
> Thomas, Andrew, what to you think to try to clean this usage from 
> testpmd and add more APIs if needed for this?
> 
Can we add an API such as rte_eth_get_device(pord_id)

for example:
struct rte_eth_dev *	332
rte_eth_get_device(uint16_t port_id）
{
        return &rte_eth_devices[port_id];
}
>>>> +    if (queue_id >= dev->data->nb_tx_queues) {
>>>> +        printf("Invalid Tx queue_id %u\n", queue_id);
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> Number of the queues can be get via 'rte_eth_dev_info_get()'.
>>>
>> This is also called in txonly. Do you want to replace it?
> 
> That would be good if you can do it in a separate patch, thank you.
> .
 From my understand, it will not be replaced. The value of 
rte_eth_dev_info_get is the maximum number of queues supported by the 
device, which is different from the number of actually enabled queues. 
Therefore, you need to check the number of actually enabled queues.
What do you think?
> 

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

[Thomas Monjalon]

10/03/2021 02:48, oulijun:
> Can we add an API such as rte_eth_get_device(pord_id)
> 
> for example:
> struct rte_eth_dev *
> rte_eth_get_device(uint16_t port_id)
> {
>         return &rte_eth_devices[port_id];
> }

An application is not supposed to access the struct rte_eth_dev.
Which info do you need from this struct?

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd

[oulijun]

在 2021/3/10 15:59, Thomas Monjalon 写道:
> 10/03/2021 02:48, oulijun:
>> Can we add an API such as rte_eth_get_device(pord_id)
>>
>> for example:
>> struct rte_eth_dev *
>> rte_eth_get_device(uint16_t port_id)
>> {
>>          return &rte_eth_devices[port_id];
>> }
> An application is not supposed to access the struct rte_eth_dev.
> Which info do you need from this struct?
Applications cannot directly access the global variable 
rte_eth_devices[]. To obtain information about rte_eth_dev, they need to 
access the global variable through APIs instead of directly.
> _______________________________________________
> Linuxarm mailing list -- linuxarm@openeuler.org
> To unsubscribe send an email to linuxarm-leave@openeuler.org

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd

[Thomas Monjalon]

12/03/2021 11:29, oulijun:
> 2021/3/10 15:59, Thomas Monjalon:
> > 10/03/2021 02:48, oulijun:
> >> Can we add an API such as rte_eth_get_device(pord_id)
> >>
> >> for example:
> >> struct rte_eth_dev *
> >> rte_eth_get_device(uint16_t port_id)
> >> {
> >>          return &rte_eth_devices[port_id];
> >> }
> > An application is not supposed to access the struct rte_eth_dev.
> > Which info do you need from this struct?
> 
> Applications cannot directly access the global variable 
> rte_eth_devices[]. To obtain information about rte_eth_dev, they need to 
> access the global variable through APIs instead of directly.

That's not the question.
Which device info do you need, which is not already provided by
one of the function rte_eth_*info* ?
	rte_eth_dev_get_dcb_info
	rte_eth_dev_get_reg_info
	rte_eth_dev_info_get
	rte_eth_rx_queue_info_get
	rte_eth_tx_queue_info_get
	rte_eth_dev_get_module_info

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd

[oulijun]

在 2021/3/12 19:21, Thomas Monjalon 写道:
> 12/03/2021 11:29, oulijun:
>> 2021/3/10 15:59, Thomas Monjalon:
>>> 10/03/2021 02:48, oulijun:
>>>> Can we add an API such as rte_eth_get_device(pord_id)
>>>>
>>>> for example:
>>>> struct rte_eth_dev *
>>>> rte_eth_get_device(uint16_t port_id)
>>>> {
>>>>           return &rte_eth_devices[port_id];
>>>> }
>>> An application is not supposed to access the struct rte_eth_dev.
>>> Which info do you need from this struct?
>>
>> Applications cannot directly access the global variable
>> rte_eth_devices[]. To obtain information about rte_eth_dev, they need to
>> access the global variable through APIs instead of directly.
> 
> That's not the question.
> Which device info do you need, which is not already provided by
> one of the function rte_eth_*info* ?
> 	rte_eth_dev_get_dcb_info
> 	rte_eth_dev_get_reg_info
> 	rte_eth_dev_info_get
> 	rte_eth_rx_queue_info_get
> 	rte_eth_tx_queue_info_get
> 	rte_eth_dev_get_module_info
> 
Hi, Thomas
   I think dev->data->nb_tx_queues can be obtained through 
rte_eth_info_get, but dev->data->tx_queue_state[queue_id] has nowhere to 
be obtained. I think a patch needs to be added to obtain 
tx_queue_state[queue_id] through rte_eth_tx_queue_info_get. What do you 
think?

Thanks
Lijun Ou
> 
> 
> .
> 

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd

[Thomas Monjalon]

17/03/2021 12:30, oulijun:
> 2021/3/12 19:21, Thomas Monjalon:
> > 12/03/2021 11:29, oulijun:
> >> 2021/3/10 15:59, Thomas Monjalon:
> >>> 10/03/2021 02:48, oulijun:
> >>>> Can we add an API such as rte_eth_get_device(pord_id)
> >>>>
> >>>> for example:
> >>>> struct rte_eth_dev *
> >>>> rte_eth_get_device(uint16_t port_id)
> >>>> {
> >>>>           return &rte_eth_devices[port_id];
> >>>> }
> >>> An application is not supposed to access the struct rte_eth_dev.
> >>> Which info do you need from this struct?
> >>
> >> Applications cannot directly access the global variable
> >> rte_eth_devices[]. To obtain information about rte_eth_dev, they need to
> >> access the global variable through APIs instead of directly.
> > 
> > That's not the question.
> > Which device info do you need, which is not already provided by
> > one of the function rte_eth_*info* ?
> > 	rte_eth_dev_get_dcb_info
> > 	rte_eth_dev_get_reg_info
> > 	rte_eth_dev_info_get
> > 	rte_eth_rx_queue_info_get
> > 	rte_eth_tx_queue_info_get
> > 	rte_eth_dev_get_module_info
> > 
> Hi, Thomas
>    I think dev->data->nb_tx_queues can be obtained through 
> rte_eth_info_get, but dev->data->tx_queue_state[queue_id] has nowhere to 
> be obtained. I think a patch needs to be added to obtain 
> tx_queue_state[queue_id] through rte_eth_tx_queue_info_get. What do you 
> think?

Yes it looks OK to add more queue info in rte_eth_*x_queue_info_get.

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd

[oulijun]

在 2021/3/17 20:07, Thomas Monjalon 写道:
> 17/03/2021 12:30, oulijun:
>> 2021/3/12 19:21, Thomas Monjalon:
>>> 12/03/2021 11:29, oulijun:
>>>> 2021/3/10 15:59, Thomas Monjalon:
>>>>> 10/03/2021 02:48, oulijun:
>>>>>> Can we add an API such as rte_eth_get_device(pord_id)
>>>>>>
>>>>>> for example:
>>>>>> struct rte_eth_dev *
>>>>>> rte_eth_get_device(uint16_t port_id)
>>>>>> {
>>>>>>            return &rte_eth_devices[port_id];
>>>>>> }
>>>>> An application is not supposed to access the struct rte_eth_dev.
>>>>> Which info do you need from this struct?
>>>>
>>>> Applications cannot directly access the global variable
>>>> rte_eth_devices[]. To obtain information about rte_eth_dev, they need to
>>>> access the global variable through APIs instead of directly.
>>>
>>> That's not the question.
>>> Which device info do you need, which is not already provided by
>>> one of the function rte_eth_*info* ?
>>> 	rte_eth_dev_get_dcb_info
>>> 	rte_eth_dev_get_reg_info
>>> 	rte_eth_dev_info_get
>>> 	rte_eth_rx_queue_info_get
>>> 	rte_eth_tx_queue_info_get
>>> 	rte_eth_dev_get_module_info
>>>
>> Hi, Thomas
>>     I think dev->data->nb_tx_queues can be obtained through
>> rte_eth_info_get, but dev->data->tx_queue_state[queue_id] has nowhere to
>> be obtained. I think a patch needs to be added to obtain
>> tx_queue_state[queue_id] through rte_eth_tx_queue_info_get. What do you
>> think?
> 
> Yes it looks OK to add more queue info in rte_eth_*x_queue_info_get.
Good, can I just catch up with this version?
> 
> 
> .
> 

Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd

[Thomas Monjalon]

18/03/2021 04:56, oulijun:
> 在 2021/3/17 20:07, Thomas Monjalon 写道:
> > 17/03/2021 12:30, oulijun:
> >> 2021/3/12 19:21, Thomas Monjalon:
> >>> 12/03/2021 11:29, oulijun:
> >>>> 2021/3/10 15:59, Thomas Monjalon:
> >>>>> 10/03/2021 02:48, oulijun:
> >>>>>> Can we add an API such as rte_eth_get_device(pord_id)
> >>>>>>
> >>>>>> for example:
> >>>>>> struct rte_eth_dev *
> >>>>>> rte_eth_get_device(uint16_t port_id)
> >>>>>> {
> >>>>>>            return &rte_eth_devices[port_id];
> >>>>>> }
> >>>>> An application is not supposed to access the struct rte_eth_dev.
> >>>>> Which info do you need from this struct?
> >>>>
> >>>> Applications cannot directly access the global variable
> >>>> rte_eth_devices[]. To obtain information about rte_eth_dev, they need to
> >>>> access the global variable through APIs instead of directly.
> >>>
> >>> That's not the question.
> >>> Which device info do you need, which is not already provided by
> >>> one of the function rte_eth_*info* ?
> >>> 	rte_eth_dev_get_dcb_info
> >>> 	rte_eth_dev_get_reg_info
> >>> 	rte_eth_dev_info_get
> >>> 	rte_eth_rx_queue_info_get
> >>> 	rte_eth_tx_queue_info_get
> >>> 	rte_eth_dev_get_module_info
> >>>
> >> Hi, Thomas
> >>     I think dev->data->nb_tx_queues can be obtained through
> >> rte_eth_info_get, but dev->data->tx_queue_state[queue_id] has nowhere to
> >> be obtained. I think a patch needs to be added to obtain
> >> tx_queue_state[queue_id] through rte_eth_tx_queue_info_get. What do you
> >> think?
> > 
> > Yes it looks OK to add more queue info in rte_eth_*x_queue_info_get.
> Good, can I just catch up with this version?

You can try.

[dpdk-dev] [PATCH V3] 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)

Users 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)
this command executed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V2->V3:
- The command implementation is changed so that the queuestate does
  not depend on the command execution.

V1->V2:
- use Tx instead of TX
- add note in doc
---
 app/test-pmd/cmdline.c                      | 85 +++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_21_05.rst      |  2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
 3 files changed, 98 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 56cf0bf..4daf9b2 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -675,6 +675,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"
@@ -16912,6 +16915,87 @@ 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;
+	uint16_t port_id = res->port_id;
+	uint16_t queue_id = res->queue_id;
+	uint32_t free_cnt = res->free_cnt;
+	struct rte_eth_txq_info qinfo;
+	int ret;
+
+	if (port_is_started(port_id) != 1) {
+		printf("Please start port %u first\n", port_id);
+		return;
+	}
+
+	/* Make sure the Tx queue is valid by called get tx queue info API */
+	if (rte_eth_tx_queue_info_get(port_id, queue_id, &qinfo)) {
+		printf("Failed to get port %u Tx queue %u info!\n",
+		       port_id, 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 */
@@ -17037,6 +17121,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 f1ccbb5..f43c4cf 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -155,6 +155,8 @@ New Features
     ``dpdk-testpmd -- --eth-link-speed N``
   * 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 36f0a32..0b5281e 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -272,6 +272,17 @@ 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)
+
+.. note::
+   This command is dangerous, users must make sure there are no cucurrent access to
+   the same Tx queue (link rte_eth_tx_burst, rte_eth_dev_tx_queue_stop and so on).
+
 show config
 ~~~~~~~~~~~
 
-- 
2.7.4

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

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Lijun Ou <oulijun@huawei.com>
> Sent: Monday, April 12, 2021 21:13
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
> linuxarm@openeuler.org
> Subject: [PATCH V3] app/testpmd: support Tx mbuf free on demand cmd
> 
> From: Chengwen Feng <fengchengwen@huawei.com>
> 
> This patch support tx_done_cleanup command:
> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
> 
> Users 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) this command
> executed.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>

As I remember, last comments (from Ferruh and Aaron) suggest you to do this in an unit test not testpmd.

> ---
> V2->V3:
> - The command implementation is changed so that the queuestate does
>   not depend on the command execution.
> 
> V1->V2:
> - use Tx instead of TX
> - add note in doc
> ---
>  app/test-pmd/cmdline.c                      | 85 +++++++++++++++++++++++++++++
>  doc/guides/rel_notes/release_21_05.rst      |  2 +
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>  3 files changed, 98 insertions(+)
<snip>
> +
>  show config
>  ~~~~~~~~~~~
> 
> --
> 2.7.4

[dpdk-dev] [PATCH V4] 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)

Users 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)
this command executed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V3->V4:
- revert the V3 scheme.

V2->V3:
- The command implementation is changed so that the queuestate does
  not depend on the command execution.

V1->V2:
- use Tx instead of TX
- add note in doc
---
 app/test-pmd/cmdline.c                      | 84 +++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_21_05.rst      |  2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
 3 files changed, 97 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 5bf1497..b43964d 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -675,6 +675,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"
@@ -16915,6 +16918,86 @@ 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;
+	uint16_t port_id = res->port_id;
+	uint16_t queue_id = res->queue_id;
+	uint32_t free_cnt = res->free_cnt;
+	struct rte_eth_txq_info qinfo;
+	int ret;
+
+	if (rte_eth_tx_queue_info_get(port_id, queue_id, &qinfo)) {
+		printf("Failed to get port %u Tx queue %u info!\n",
+		       port_id, queue_id);
+		return;
+	}
+
+	if (qinfo.queue_state != RTE_ETH_QUEUE_STATE_STARTED) {
+		printf("TX queue %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 */
@@ -17040,6 +17123,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 4538bb9..554499f 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -217,6 +217,8 @@ New Features
     ``show port (port_id) rxq (queue_id) desc used count``
   * Added command to dump internal representation information of single flow.
     ``flow dump (port_id) rule (rule_id)``
+  * Added command to cleanup a Tx queue's mbuf on a port.
+    ``tx_done_cleanup port <port_id> <queue_id> <free_cnt>``
 
 * **Updated ipsec-secgw sample application.**
 
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index a736e7d..6472647 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -270,6 +270,17 @@ 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)
+
+.. note::
+   This command is dangerous, users must make sure there are no cucurrent access to
+   the same Tx queue (link rte_eth_tx_burst, rte_eth_dev_tx_queue_stop and so on).
+
 show config
 ~~~~~~~~~~~
 
-- 
2.7.4

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

[oulijun]

在 2021/4/19 11:11, Li, Xiaoyun 写道:
> Hi
> 
>> -----Original Message-----
>> From: Lijun Ou <oulijun@huawei.com>
>> Sent: Monday, April 12, 2021 21:13
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
>> linuxarm@openeuler.org
>> Subject: [PATCH V3] app/testpmd: support Tx mbuf free on demand cmd
>>
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>
>> This patch support tx_done_cleanup command:
>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>
>> Users 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) this command
>> executed.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> 
> As I remember, last comments (from Ferruh and Aaron) suggest you to do this in an unit test not testpmd.
> 
Hi, Xiaoyun
Maybe it is. We've tested it in our local environment and he's OK.If I 
use the community's method of unit testing, I may have trouble doing 
unit testing. Would you consider someone else to help me?
I've already sent V4 and reverted to V3 and used the latest queue state 
solution.

>> ---
>> V2->V3:
>> - The command implementation is changed so that the queuestate does
>>    not depend on the command execution.
>>
>> V1->V2:
>> - use Tx instead of TX
>> - add note in doc
>> ---
>>   app/test-pmd/cmdline.c                      | 85 +++++++++++++++++++++++++++++
>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>   3 files changed, 98 insertions(+)
> <snip>
>> +
>>   show config
>>   ~~~~~~~~~~~
>>
>> --
>> 2.7.4
> 
> .
> 

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

[Ferruh Yigit]

On 4/19/2021 1:40 PM, oulijun wrote:
> 
> 
> 在 2021/4/19 11:11, Li, Xiaoyun 写道:
>> Hi
>>
>>> -----Original Message-----
>>> From: Lijun Ou <oulijun@huawei.com>
>>> Sent: Monday, April 12, 2021 21:13
>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org;
>>> linuxarm@openeuler.org
>>> Subject: [PATCH V3] app/testpmd: support Tx mbuf free on demand cmd
>>>
>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>
>>> This patch support tx_done_cleanup command:
>>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>>
>>> Users 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) this command
>>> executed.
>>>
>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>
>> As I remember, last comments (from Ferruh and Aaron) suggest you to do this in 
>> an unit test not testpmd.
>>
> Hi, Xiaoyun
> Maybe it is. We've tested it in our local environment and he's OK.If I use the 
> community's method of unit testing, I may have trouble doing unit testing. Would 
> you consider someone else to help me?
> I've already sent V4 and reverted to V3 and used the latest queue state solution.
> 

I think it is OK to have the testpmd command (this patch), at least makes the 
'rte_eth_tx_done_cleanup()' API used, but I believe this is not the best/easiest 
way to verify relevant PMD implementation.


For long term though we should have the 'app/test/test_ethdev.c' and improve it 
gradually and collectively.

>>> ---
>>> V2->V3:
>>> - The command implementation is changed so that the queuestate does
>>>    not depend on the command execution.
>>>
>>> V1->V2:
>>> - use Tx instead of TX
>>> - add note in doc
>>> ---
>>>   app/test-pmd/cmdline.c                      | 85 +++++++++++++++++++++++++++++
>>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>>   3 files changed, 98 insertions(+)
>> <snip>
>>> +
>>>   show config
>>>   ~~~~~~~~~~~
>>>
>>> -- 
>>> 2.7.4
>>
>> .
>>

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

[Ferruh Yigit]

On 4/19/2021 1:36 PM, Lijun Ou wrote:
> From: Chengwen Feng <fengchengwen@huawei.com>
> 
> This patch support tx_done_cleanup command:
> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
> 

Instead of creating a new root level, 'tx_done_cleanup' command, what do you 
think to use existing port command, something like:

"port cleanup <port_id> txq <queue_id> <free_cnt>"

> Users 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)
> this command executed.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V3->V4:
> - revert the V3 scheme.
> 
> V2->V3:
> - The command implementation is changed so that the queuestate does
>    not depend on the command execution.
> 
> V1->V2:
> - use Tx instead of TX
> - add note in doc
> ---
>   app/test-pmd/cmdline.c                      | 84 +++++++++++++++++++++++++++++
>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>   3 files changed, 97 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 5bf1497..b43964d 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -675,6 +675,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"
> +

If 'free_cnt' is '0', driver should free all possible mbufs, can you add this 
detail to help?

>   			"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"
> @@ -16915,6 +16918,86 @@ 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;
> +	uint16_t port_id = res->port_id;
> +	uint16_t queue_id = res->queue_id;
> +	uint32_t free_cnt = res->free_cnt;
> +	struct rte_eth_txq_info qinfo;
> +	int ret;
> +
> +	if (rte_eth_tx_queue_info_get(port_id, queue_id, &qinfo)) {
> +		printf("Failed to get port %u Tx queue %u info!\n",
> +		       port_id, queue_id);
> +		return;
> +	}
> +
> +	if (qinfo.queue_state != RTE_ETH_QUEUE_STATE_STARTED) {
> +		printf("TX queue %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.
> +	 */

Should there be a check to see if testpmd forwarding is stared or not?
This API shouldn't be allowed when forwarding is started.

> +	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 */
> @@ -17040,6 +17123,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 4538bb9..554499f 100644
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> @@ -217,6 +217,8 @@ New Features
>       ``show port (port_id) rxq (queue_id) desc used count``
>     * Added command to dump internal representation information of single flow.
>       ``flow dump (port_id) rule (rule_id)``
> +  * Added command to cleanup a Tx queue's mbuf on a port.
> +    ``tx_done_cleanup port <port_id> <queue_id> <free_cnt>``
>   
>   * **Updated ipsec-secgw sample application.**
>   
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index a736e7d..6472647 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -270,6 +270,17 @@ 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)
> +
> +.. note::
> +   This command is dangerous, users must make sure there are no cucurrent access to
> +   the same Tx queue (link rte_eth_tx_burst, rte_eth_dev_tx_queue_stop and so on).
> +
>   show config
>   ~~~~~~~~~~~
>   
> 

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

[oulijun]

在 2021/4/19 23:28, Ferruh Yigit 写道:
> On 4/19/2021 1:36 PM, Lijun Ou wrote:
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>
>> This patch support tx_done_cleanup command:
>> tx_done_cleanup port (port_id) (queue_id) (free_cnt)
>>
> 
> Instead of creating a new root level, 'tx_done_cleanup' command, what do 
> you think to use existing port command, something like:
> 
> "port cleanup <port_id> txq <queue_id> <free_cnt>"
> 
Agree.
>> Users 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)
>> this command executed.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V3->V4:
>> - revert the V3 scheme.
>>
>> V2->V3:
>> - The command implementation is changed so that the queuestate does
>>    not depend on the command execution.
>>
>> V1->V2:
>> - use Tx instead of TX
>> - add note in doc
>> ---
>>   app/test-pmd/cmdline.c                      | 84 
>> +++++++++++++++++++++++++++++
>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
>>   3 files changed, 97 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 5bf1497..b43964d 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -675,6 +675,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"
>> +
> 
> If 'free_cnt' is '0', driver should free all possible mbufs, can you add 
> this detail to help?
Agree. I will fix it.
> 
>>               "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"
>> @@ -16915,6 +16918,86 @@ 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;
>> +    uint16_t port_id = res->port_id;
>> +    uint16_t queue_id = res->queue_id;
>> +    uint32_t free_cnt = res->free_cnt;
>> +    struct rte_eth_txq_info qinfo;
>> +    int ret;
>> +
>> +    if (rte_eth_tx_queue_info_get(port_id, queue_id, &qinfo)) {
>> +        printf("Failed to get port %u Tx queue %u info!\n",
>> +               port_id, queue_id);
>> +        return;
>> +    }
>> +
>> +    if (qinfo.queue_state != RTE_ETH_QUEUE_STATE_STARTED) {
>> +        printf("TX queue %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.
>> +     */
> 
> Should there be a check to see if testpmd forwarding is stared or not?
> This API shouldn't be allowed when forwarding is started.
> 
Yes. I will fix in the new version.
>> +    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 */
>> @@ -17040,6 +17123,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 4538bb9..554499f 100644
>> --- a/doc/guides/rel_notes/release_21_05.rst
>> +++ b/doc/guides/rel_notes/release_21_05.rst
>> @@ -217,6 +217,8 @@ New Features
>>       ``show port (port_id) rxq (queue_id) desc used count``
>>     * Added command to dump internal representation information of 
>> single flow.
>>       ``flow dump (port_id) rule (rule_id)``
>> +  * Added command to cleanup a Tx queue's mbuf on a port.
>> +    ``tx_done_cleanup port <port_id> <queue_id> <free_cnt>``
>>   * **Updated ipsec-secgw sample application.**
>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst 
>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> index a736e7d..6472647 100644
>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>> @@ -270,6 +270,17 @@ 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)
>> +
>> +.. note::
>> +   This command is dangerous, users must make sure there are no 
>> cucurrent access to
>> +   the same Tx queue (link rte_eth_tx_burst, 
>> rte_eth_dev_tx_queue_stop and so on).
>> +
>>   show config
>>   ~~~~~~~~~~~
>>
> 
> .
> 

[dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command

[Lijun Ou]

From: Chengwen Feng <fengchengwen@huawei.com>

This patch supports cleanup txq mbufs command:
port cleanup (port_id) txq (queue_id) (free_cnt)

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V4->V5:
- rewrite patch title
- define the new cmd.
- Fix the comments given by Ferruh.yigit

V3->V4:
- revert the V3 scheme.

V2->V3:
- The command implementation is changed so that the queuestate does
  not depend on the command execution.

V1->V2:
- use Tx instead of TX
- add note in doc
---
 app/test-pmd/cmdline.c                      | 85 +++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_21_05.rst      |  2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  9 +++
 3 files changed, 96 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index ccaeefa..40389f3 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2455,6 +2455,90 @@ cmdline_parse_inst_t cmd_config_rss_hash_key = {
 	},
 };
 
+/* *** cleanup txq mbufs *** */
+struct cmd_cleanup_txq_mbufs_result {
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t keyword;
+	cmdline_fixed_string_t name;
+	uint16_t port_id;
+	uint16_t queue_id;
+	uint32_t free_cnt;
+};
+
+static void
+cmd_cleanup_txq_mbufs_parsed(void *parsed_result,
+			     __rte_unused struct cmdline *cl,
+			     __rte_unused void *data)
+{
+	struct cmd_cleanup_txq_mbufs_result *res = parsed_result;
+	uint16_t port_id = res->port_id;
+	uint16_t queue_id = res->queue_id;
+	uint32_t free_cnt = res->free_cnt;
+	struct rte_eth_txq_info qinfo;
+	int ret;
+
+	if (test_done == 0) {
+		printf("Please stop forwarding first\n");
+		return;
+	}
+
+	if (rte_eth_tx_queue_info_get(port_id, queue_id, &qinfo)) {
+		printf("Failed to get port %u TX queue %u info\n",
+		       port_id, queue_id);
+		return;
+	}
+
+	if (qinfo.queue_state != RTE_ETH_QUEUE_STATE_STARTED) {
+		printf("TX queue %u not started\n", queue_id);
+		return;
+	}
+
+	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_cleanup_txq_mbufs_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, port,
+				 "port");
+cmdline_parse_token_string_t cmd_cleanup_txq_mbufs_cleanup =
+	TOKEN_STRING_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, keyword,
+				 "cleanup");
+cmdline_parse_token_num_t cmd_cleanup_txq_mbufs_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, port_id,
+			      RTE_UINT16);
+cmdline_parse_token_string_t cmd_cleanup_txq_mbufs_txq =
+	TOKEN_STRING_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, name,
+				 "txq");
+cmdline_parse_token_num_t cmd_cleanup_txq_mbufs_queue_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, queue_id,
+			      RTE_UINT16);
+cmdline_parse_token_num_t cmd_cleanup_txq_mbufs_free_cnt =
+	TOKEN_NUM_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, free_cnt,
+			      RTE_UINT32);
+
+cmdline_parse_inst_t cmd_cleanup_txq_mbufs = {
+	.f = cmd_cleanup_txq_mbufs_parsed,
+	.data = NULL,
+	.help_str = "port cleanup <port_id> txq <queue_id> <free_cnt>",
+	.tokens = {
+		(void *)&cmd_cleanup_txq_mbufs_port,
+		(void *)&cmd_cleanup_txq_mbufs_cleanup,
+		(void *)&cmd_cleanup_txq_mbufs_port_id,
+		(void *)&cmd_cleanup_txq_mbufs_txq,
+		(void *)&cmd_cleanup_txq_mbufs_queue_id,
+		(void *)&cmd_cleanup_txq_mbufs_free_cnt,
+		NULL,
+	},
+};
+
 /* *** configure port rxq/txq ring size *** */
 struct cmd_config_rxtx_ring_size {
 	cmdline_fixed_string_t port;
@@ -17490,6 +17574,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_showport_rss_hash,
 	(cmdline_parse_inst_t *)&cmd_showport_rss_hash_key,
 	(cmdline_parse_inst_t *)&cmd_config_rss_hash_key,
+	(cmdline_parse_inst_t *)&cmd_cleanup_txq_mbufs,
 	(cmdline_parse_inst_t *)&cmd_dump,
 	(cmdline_parse_inst_t *)&cmd_dump_one,
 #ifdef RTE_NET_I40E
diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 668fca8..70f10ba 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -230,6 +230,8 @@ New Features
   * Added commands to construct conntrack context and relevant indirect
     action handle creation, update for conntrack action as well as conntrack
     item matching.
+  * Added command to cleanup a Tx queue's mbuf on a port.
+    ``port cleanup (port_id) txq (queue_id) (free_cnt)``
 
 * **Updated ipsec-secgw sample application.**
 
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 5dd98c2..dc7d9ae 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -2443,6 +2443,15 @@ hash of input [IP] packets received on port::
                      ipv6-udp-ex <string of hex digits \
                      (variable length, NIC dependent)>)
 
+port cleanup txq mbufs
+~~~~~~~~~~~~~~~~~~~~~~
+
+To cleanup txq mbufs currently cached by driver::
+
+   testpmd> port cleanup (port_id) txq (queue_id) (free_cnt)
+
+If the value of ``free_cnt`` is 0, driver should free all cached mbufs.
+
 Device Functions
 ----------------
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command

[Ferruh Yigit]

On 4/21/2021 9:09 AM, Lijun Ou wrote:
> From: Chengwen Feng <fengchengwen@huawei.com>
> 
> This patch supports cleanup txq mbufs command:
> port cleanup (port_id) txq (queue_id) (free_cnt)
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V4->V5:
> - rewrite patch title
> - define the new cmd.
> - Fix the comments given by Ferruh.yigit
> 
> V3->V4:
> - revert the V3 scheme.
> 
> V2->V3:
> - The command implementation is changed so that the queuestate does
>    not depend on the command execution.
> 
> V1->V2:
> - use Tx instead of TX
> - add note in doc
> ---
>   app/test-pmd/cmdline.c                      | 85 +++++++++++++++++++++++++++++
>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  9 +++
>   3 files changed, 96 insertions(+)
> 

Can you please update 'cmd_help_long_parsed' for the help string, this was in v4 
and seems dropped during change.

Re: [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command

[oulijun]

在 2021/4/21 16:15, Ferruh Yigit 写道:
> On 4/21/2021 9:09 AM, Lijun Ou wrote:
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>
>> This patch supports cleanup txq mbufs command:
>> port cleanup (port_id) txq (queue_id) (free_cnt)
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V4->V5:
>> - rewrite patch title
>> - define the new cmd.
>> - Fix the comments given by Ferruh.yigit
>>
>> V3->V4:
>> - revert the V3 scheme.
>>
>> V2->V3:
>> - The command implementation is changed so that the queuestate does
>>    not depend on the command execution.
>>
>> V1->V2:
>> - use Tx instead of TX
>> - add note in doc
>> ---
>>   app/test-pmd/cmdline.c                      | 85 
>> +++++++++++++++++++++++++++++
>>   doc/guides/rel_notes/release_21_05.rst      |  2 +
>>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  9 +++
>>   3 files changed, 96 insertions(+)
>>
> 
> Can you please update 'cmd_help_long_parsed' for the help string, this 
> was in v4 and seems dropped during change.
Yes, I do. Thanks
> .
> 

[dpdk-dev] [PATCH V6] app/test-pmd: support cleanup txq mbufs command

[Lijun Ou]

From: Chengwen Feng <fengchengwen@huawei.com>

This patch supports cleanup txq mbufs command:
port cleanup (port_id) txq (queue_id) (free_cnt)

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V5->V6:
- use Tx/Rx instead of RX/TX
- update 'cmd_help_long_parsed'

V4->V5:
- rewrite patch title
- define the new cmd.
- Fix the comments given by Ferruh.yigit

V3->V4:
- revert the V3 scheme.

V2->V3:
- The command implementation is changed so that the queuestate does
  not depend on the command execution.

V1->V2:
- use Tx instead of TX
- add note in doc
---
 app/test-pmd/cmdline.c                      | 88 +++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_21_05.rst      |  2 +
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  9 +++
 3 files changed, 99 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index ccaeefa..d63cd49 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -905,6 +905,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"    Register a dynf and Set/clear this flag on Tx. "
 			"Testpmd will set this value to any Tx packet "
 			"sent from this port\n\n"
+
+			"port cleanup (port_id) txq (queue_id) (free_cnt)\n"
+			"    Cleanup txq mbufs for a specific Tx queue\n\n"
 		);
 	}
 
@@ -2455,6 +2458,90 @@ cmdline_parse_inst_t cmd_config_rss_hash_key = {
 	},
 };
 
+/* *** cleanup txq mbufs *** */
+struct cmd_cleanup_txq_mbufs_result {
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t keyword;
+	cmdline_fixed_string_t name;
+	uint16_t port_id;
+	uint16_t queue_id;
+	uint32_t free_cnt;
+};
+
+static void
+cmd_cleanup_txq_mbufs_parsed(void *parsed_result,
+			     __rte_unused struct cmdline *cl,
+			     __rte_unused void *data)
+{
+	struct cmd_cleanup_txq_mbufs_result *res = parsed_result;
+	uint16_t port_id = res->port_id;
+	uint16_t queue_id = res->queue_id;
+	uint32_t free_cnt = res->free_cnt;
+	struct rte_eth_txq_info qinfo;
+	int ret;
+
+	if (test_done == 0) {
+		printf("Please stop forwarding first\n");
+		return;
+	}
+
+	if (rte_eth_tx_queue_info_get(port_id, queue_id, &qinfo)) {
+		printf("Failed to get port %u Tx queue %u info\n",
+		       port_id, queue_id);
+		return;
+	}
+
+	if (qinfo.queue_state != RTE_ETH_QUEUE_STATE_STARTED) {
+		printf("Tx queue %u not started\n", queue_id);
+		return;
+	}
+
+	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_cleanup_txq_mbufs_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, port,
+				 "port");
+cmdline_parse_token_string_t cmd_cleanup_txq_mbufs_cleanup =
+	TOKEN_STRING_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, keyword,
+				 "cleanup");
+cmdline_parse_token_num_t cmd_cleanup_txq_mbufs_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, port_id,
+			      RTE_UINT16);
+cmdline_parse_token_string_t cmd_cleanup_txq_mbufs_txq =
+	TOKEN_STRING_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, name,
+				 "txq");
+cmdline_parse_token_num_t cmd_cleanup_txq_mbufs_queue_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, queue_id,
+			      RTE_UINT16);
+cmdline_parse_token_num_t cmd_cleanup_txq_mbufs_free_cnt =
+	TOKEN_NUM_INITIALIZER(struct cmd_cleanup_txq_mbufs_result, free_cnt,
+			      RTE_UINT32);
+
+cmdline_parse_inst_t cmd_cleanup_txq_mbufs = {
+	.f = cmd_cleanup_txq_mbufs_parsed,
+	.data = NULL,
+	.help_str = "port cleanup <port_id> txq <queue_id> <free_cnt>",
+	.tokens = {
+		(void *)&cmd_cleanup_txq_mbufs_port,
+		(void *)&cmd_cleanup_txq_mbufs_cleanup,
+		(void *)&cmd_cleanup_txq_mbufs_port_id,
+		(void *)&cmd_cleanup_txq_mbufs_txq,
+		(void *)&cmd_cleanup_txq_mbufs_queue_id,
+		(void *)&cmd_cleanup_txq_mbufs_free_cnt,
+		NULL,
+	},
+};
+
 /* *** configure port rxq/txq ring size *** */
 struct cmd_config_rxtx_ring_size {
 	cmdline_fixed_string_t port;
@@ -17490,6 +17577,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_showport_rss_hash,
 	(cmdline_parse_inst_t *)&cmd_showport_rss_hash_key,
 	(cmdline_parse_inst_t *)&cmd_config_rss_hash_key,
+	(cmdline_parse_inst_t *)&cmd_cleanup_txq_mbufs,
 	(cmdline_parse_inst_t *)&cmd_dump,
 	(cmdline_parse_inst_t *)&cmd_dump_one,
 #ifdef RTE_NET_I40E
diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 668fca8..70f10ba 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -230,6 +230,8 @@ New Features
   * Added commands to construct conntrack context and relevant indirect
     action handle creation, update for conntrack action as well as conntrack
     item matching.
+  * Added command to cleanup a Tx queue's mbuf on a port.
+    ``port cleanup (port_id) txq (queue_id) (free_cnt)``
 
 * **Updated ipsec-secgw sample application.**
 
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 5dd98c2..dc7d9ae 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -2443,6 +2443,15 @@ hash of input [IP] packets received on port::
                      ipv6-udp-ex <string of hex digits \
                      (variable length, NIC dependent)>)
 
+port cleanup txq mbufs
+~~~~~~~~~~~~~~~~~~~~~~
+
+To cleanup txq mbufs currently cached by driver::
+
+   testpmd> port cleanup (port_id) txq (queue_id) (free_cnt)
+
+If the value of ``free_cnt`` is 0, driver should free all cached mbufs.
+
 Device Functions
 ----------------
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH V6] app/test-pmd: support cleanup txq mbufs command

[Ferruh Yigit]

On 4/21/2021 9:45 AM, Lijun Ou wrote:
> From: Chengwen Feng <fengchengwen@huawei.com>
> 
> This patch supports cleanup txq mbufs command:
> port cleanup (port_id) txq (queue_id) (free_cnt)
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.