DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] app/testpmd: support Tx mbuf free on demand cmd
@ 2021-03-05  7:33 Lijun Ou
  2021-03-05  7:46 ` Li, Xiaoyun
  2021-03-05  9:57 ` [dpdk-dev] [PATCH V2] " Lijun Ou
  0 siblings, 2 replies; 31+ messages in thread
From: Lijun Ou @ 2021-03-05  7:33 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: xiaoyun.li, dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-05  7:33 [dpdk-dev] [PATCH] app/testpmd: support Tx mbuf free on demand cmd Lijun Ou
@ 2021-03-05  7:46 ` Li, Xiaoyun
  2021-03-05  9:58   ` oulijun
  2021-03-05  9:57 ` [dpdk-dev] [PATCH V2] " Lijun Ou
  1 sibling, 1 reply; 31+ messages in thread
From: Li, Xiaoyun @ 2021-03-05  7:46 UTC (permalink / raw)
  To: Lijun Ou, Yigit, Ferruh; +Cc: dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-05  7:33 [dpdk-dev] [PATCH] app/testpmd: support Tx mbuf free on demand cmd Lijun Ou
  2021-03-05  7:46 ` Li, Xiaoyun
@ 2021-03-05  9:57 ` Lijun Ou
  2021-03-08 17:33   ` Ferruh Yigit
  2021-04-12 13:12   ` [dpdk-dev] [PATCH V3] " Lijun Ou
  1 sibling, 2 replies; 31+ messages in thread
From: Lijun Ou @ 2021-03-05  9:57 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: xiaoyun.li, dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-05  7:46 ` Li, Xiaoyun
@ 2021-03-05  9:58   ` oulijun
  0 siblings, 0 replies; 31+ messages in thread
From: oulijun @ 2021-03-05  9:58 UTC (permalink / raw)
  To: Li, Xiaoyun, Yigit, Ferruh; +Cc: dev, linuxarm



在 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
> .
>


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-05  9:57 ` [dpdk-dev] [PATCH V2] " Lijun Ou
@ 2021-03-08 17:33   ` Ferruh Yigit
  2021-03-09  8:49     ` oulijun
  2021-04-12 13:12   ` [dpdk-dev] [PATCH V3] " Lijun Ou
  1 sibling, 1 reply; 31+ messages in thread
From: Ferruh Yigit @ 2021-03-08 17:33 UTC (permalink / raw)
  To: Lijun Ou; +Cc: xiaoyun.li, dev, linuxarm, Andrew Rybchenko, Thomas Monjalon

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
>   ~~~~~~~~~~~
>   
> 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-08 17:33   ` Ferruh Yigit
@ 2021-03-09  8:49     ` oulijun
  2021-03-09  9:53       ` Ferruh Yigit
  0 siblings, 1 reply; 31+ messages in thread
From: oulijun @ 2021-03-09  8:49 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: xiaoyun.li, dev, linuxarm, Andrew Rybchenko, Thomas Monjalon



在 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
>>   ~~~~~~~~~~~
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-09  8:49     ` oulijun
@ 2021-03-09  9:53       ` Ferruh Yigit
  2021-03-09  9:57         ` Thomas Monjalon
                           ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Ferruh Yigit @ 2021-03-09  9:53 UTC (permalink / raw)
  To: oulijun
  Cc: xiaoyun.li, dev, linuxarm, Andrew Rybchenko, Thomas Monjalon,
	Aaron Conole, Honnappa Nagarahalli

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-09  9:53       ` Ferruh Yigit
@ 2021-03-09  9:57         ` Thomas Monjalon
  2021-03-09 10:18           ` Andrew Rybchenko
  2021-03-09 14:00         ` Aaron Conole
  2021-03-10  1:48         ` oulijun
  2 siblings, 1 reply; 31+ messages in thread
From: Thomas Monjalon @ 2021-03-09  9:57 UTC (permalink / raw)
  To: oulijun, Ferruh Yigit
  Cc: xiaoyun.li, dev, linuxarm, Andrew Rybchenko, Aaron Conole,
	Honnappa Nagarahalli

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.



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-09  9:57         ` Thomas Monjalon
@ 2021-03-09 10:18           ` Andrew Rybchenko
  0 siblings, 0 replies; 31+ messages in thread
From: Andrew Rybchenko @ 2021-03-09 10:18 UTC (permalink / raw)
  To: Thomas Monjalon, oulijun, Ferruh Yigit
  Cc: xiaoyun.li, dev, linuxarm, Andrew Rybchenko, Aaron Conole,
	Honnappa Nagarahalli

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-09  9:53       ` Ferruh Yigit
  2021-03-09  9:57         ` Thomas Monjalon
@ 2021-03-09 14:00         ` Aaron Conole
  2021-03-09 14:13           ` Ferruh Yigit
  2021-03-10  1:48         ` oulijun
  2 siblings, 1 reply; 31+ messages in thread
From: Aaron Conole @ 2021-03-09 14:00 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: oulijun, xiaoyun.li, dev, linuxarm, Andrew Rybchenko,
	Thomas Monjalon, Honnappa Nagarahalli

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.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-09 14:00         ` Aaron Conole
@ 2021-03-09 14:13           ` Ferruh Yigit
  0 siblings, 0 replies; 31+ messages in thread
From: Ferruh Yigit @ 2021-03-09 14:13 UTC (permalink / raw)
  To: Aaron Conole
  Cc: oulijun, xiaoyun.li, dev, linuxarm, Andrew Rybchenko,
	Thomas Monjalon, Honnappa Nagarahalli

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.
> 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-09  9:53       ` Ferruh Yigit
  2021-03-09  9:57         ` Thomas Monjalon
  2021-03-09 14:00         ` Aaron Conole
@ 2021-03-10  1:48         ` oulijun
  2021-03-10  7:59           ` Thomas Monjalon
  2 siblings, 1 reply; 31+ messages in thread
From: oulijun @ 2021-03-10  1:48 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: xiaoyun.li, dev, linuxarm, Andrew Rybchenko, Thomas Monjalon,
	Aaron Conole, Honnappa Nagarahalli



在 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?
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-10  1:48         ` oulijun
@ 2021-03-10  7:59           ` Thomas Monjalon
  2021-03-12 10:29             ` [dpdk-dev] [Linuxarm] " oulijun
  0 siblings, 1 reply; 31+ messages in thread
From: Thomas Monjalon @ 2021-03-10  7:59 UTC (permalink / raw)
  To: oulijun
  Cc: Ferruh Yigit, xiaoyun.li, dev, linuxarm, Andrew Rybchenko,
	Aaron Conole, Honnappa Nagarahalli

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?



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-10  7:59           ` Thomas Monjalon
@ 2021-03-12 10:29             ` oulijun
  2021-03-12 11:21               ` Thomas Monjalon
  0 siblings, 1 reply; 31+ messages in thread
From: oulijun @ 2021-03-12 10:29 UTC (permalink / raw)
  To: linuxarm, dev, Thomas Monjalon



在 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-12 10:29             ` [dpdk-dev] [Linuxarm] " oulijun
@ 2021-03-12 11:21               ` Thomas Monjalon
  2021-03-17 11:30                 ` oulijun
  0 siblings, 1 reply; 31+ messages in thread
From: Thomas Monjalon @ 2021-03-12 11:21 UTC (permalink / raw)
  To: oulijun; +Cc: linuxarm, dev

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




^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-12 11:21               ` Thomas Monjalon
@ 2021-03-17 11:30                 ` oulijun
  2021-03-17 12:07                   ` Thomas Monjalon
  0 siblings, 1 reply; 31+ messages in thread
From: oulijun @ 2021-03-17 11:30 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: linuxarm, dev



在 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
> 
> 
> .
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-17 11:30                 ` oulijun
@ 2021-03-17 12:07                   ` Thomas Monjalon
  2021-03-18  3:56                     ` oulijun
  0 siblings, 1 reply; 31+ messages in thread
From: Thomas Monjalon @ 2021-03-17 12:07 UTC (permalink / raw)
  To: oulijun; +Cc: linuxarm, dev, ferruh.yigit, andrew.rybchenko

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.



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-17 12:07                   ` Thomas Monjalon
@ 2021-03-18  3:56                     ` oulijun
  2021-03-18  7:51                       ` Thomas Monjalon
  0 siblings, 1 reply; 31+ messages in thread
From: oulijun @ 2021-03-18  3:56 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: linuxarm, dev, ferruh.yigit, andrew.rybchenko



在 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?
> 
> 
> .
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [Linuxarm] Re: [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-18  3:56                     ` oulijun
@ 2021-03-18  7:51                       ` Thomas Monjalon
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Monjalon @ 2021-03-18  7:51 UTC (permalink / raw)
  To: oulijun; +Cc: linuxarm, dev, ferruh.yigit, andrew.rybchenko

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.



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [dpdk-dev] [PATCH V3] app/testpmd: support Tx mbuf free on demand cmd
  2021-03-05  9:57 ` [dpdk-dev] [PATCH V2] " Lijun Ou
  2021-03-08 17:33   ` Ferruh Yigit
@ 2021-04-12 13:12   ` Lijun Ou
  2021-04-19  3:11     ` Li, Xiaoyun
  2021-04-19 12:36     ` [dpdk-dev] [PATCH V4] " Lijun Ou
  1 sibling, 2 replies; 31+ messages in thread
From: Lijun Ou @ 2021-04-12 13:12 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: xiaoyun.li, dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V3] app/testpmd: support Tx mbuf free on demand cmd
  2021-04-12 13:12   ` [dpdk-dev] [PATCH V3] " Lijun Ou
@ 2021-04-19  3:11     ` Li, Xiaoyun
  2021-04-19 12:40       ` oulijun
  2021-04-19 12:36     ` [dpdk-dev] [PATCH V4] " Lijun Ou
  1 sibling, 1 reply; 31+ messages in thread
From: Li, Xiaoyun @ 2021-04-19  3:11 UTC (permalink / raw)
  To: Lijun Ou, Yigit, Ferruh; +Cc: dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [dpdk-dev] [PATCH V4] app/testpmd: support Tx mbuf free on demand cmd
  2021-04-12 13:12   ` [dpdk-dev] [PATCH V3] " Lijun Ou
  2021-04-19  3:11     ` Li, Xiaoyun
@ 2021-04-19 12:36     ` Lijun Ou
  2021-04-19 15:28       ` Ferruh Yigit
  2021-04-21  8:09       ` [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command Lijun Ou
  1 sibling, 2 replies; 31+ messages in thread
From: Lijun Ou @ 2021-04-19 12:36 UTC (permalink / raw)
  To: ferruh.yigit; +Cc: xiaoyun.li, dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V3] app/testpmd: support Tx mbuf free on demand cmd
  2021-04-19  3:11     ` Li, Xiaoyun
@ 2021-04-19 12:40       ` oulijun
  2021-04-19 14:56         ` Ferruh Yigit
  0 siblings, 1 reply; 31+ messages in thread
From: oulijun @ 2021-04-19 12:40 UTC (permalink / raw)
  To: Li, Xiaoyun, Yigit, Ferruh; +Cc: dev, linuxarm



在 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
> 
> .
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V3] app/testpmd: support Tx mbuf free on demand cmd
  2021-04-19 12:40       ` oulijun
@ 2021-04-19 14:56         ` Ferruh Yigit
  0 siblings, 0 replies; 31+ messages in thread
From: Ferruh Yigit @ 2021-04-19 14:56 UTC (permalink / raw)
  To: oulijun, Li, Xiaoyun
  Cc: dev, linuxarm, Aaron Conole, Thomas Monjalon, Andrew Rybchenko

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
>>
>> .
>>


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V4] app/testpmd: support Tx mbuf free on demand cmd
  2021-04-19 12:36     ` [dpdk-dev] [PATCH V4] " Lijun Ou
@ 2021-04-19 15:28       ` Ferruh Yigit
  2021-04-21  1:44         ` oulijun
  2021-04-21  8:09       ` [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command Lijun Ou
  1 sibling, 1 reply; 31+ messages in thread
From: Ferruh Yigit @ 2021-04-19 15:28 UTC (permalink / raw)
  To: Lijun Ou; +Cc: xiaoyun.li, dev, linuxarm

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
>   ~~~~~~~~~~~
>   
> 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V4] app/testpmd: support Tx mbuf free on demand cmd
  2021-04-19 15:28       ` Ferruh Yigit
@ 2021-04-21  1:44         ` oulijun
  0 siblings, 0 replies; 31+ messages in thread
From: oulijun @ 2021-04-21  1:44 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: xiaoyun.li, dev, linuxarm



在 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
>>   ~~~~~~~~~~~
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command
  2021-04-19 12:36     ` [dpdk-dev] [PATCH V4] " Lijun Ou
  2021-04-19 15:28       ` Ferruh Yigit
@ 2021-04-21  8:09       ` Lijun Ou
  2021-04-21  8:15         ` Ferruh Yigit
  2021-04-21  8:45         ` [dpdk-dev] [PATCH V6] " Lijun Ou
  1 sibling, 2 replies; 31+ messages in thread
From: Lijun Ou @ 2021-04-21  8:09 UTC (permalink / raw)
  To: thomas, ferruh.yigit; +Cc: dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command
  2021-04-21  8:09       ` [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command Lijun Ou
@ 2021-04-21  8:15         ` Ferruh Yigit
  2021-04-21  8:32           ` oulijun
  2021-04-21  8:45         ` [dpdk-dev] [PATCH V6] " Lijun Ou
  1 sibling, 1 reply; 31+ messages in thread
From: Ferruh Yigit @ 2021-04-21  8:15 UTC (permalink / raw)
  To: Lijun Ou, thomas; +Cc: dev, linuxarm

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command
  2021-04-21  8:15         ` Ferruh Yigit
@ 2021-04-21  8:32           ` oulijun
  0 siblings, 0 replies; 31+ messages in thread
From: oulijun @ 2021-04-21  8:32 UTC (permalink / raw)
  To: Ferruh Yigit, thomas; +Cc: dev, linuxarm



在 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
> .
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [dpdk-dev] [PATCH V6] app/test-pmd: support cleanup txq mbufs command
  2021-04-21  8:09       ` [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command Lijun Ou
  2021-04-21  8:15         ` Ferruh Yigit
@ 2021-04-21  8:45         ` Lijun Ou
  2021-04-21 11:26           ` Ferruh Yigit
  1 sibling, 1 reply; 31+ messages in thread
From: Lijun Ou @ 2021-04-21  8:45 UTC (permalink / raw)
  To: thomas, ferruh.yigit; +Cc: dev, linuxarm

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [dpdk-dev] [PATCH V6] app/test-pmd: support cleanup txq mbufs command
  2021-04-21  8:45         ` [dpdk-dev] [PATCH V6] " Lijun Ou
@ 2021-04-21 11:26           ` Ferruh Yigit
  0 siblings, 0 replies; 31+ messages in thread
From: Ferruh Yigit @ 2021-04-21 11:26 UTC (permalink / raw)
  To: Lijun Ou, thomas; +Cc: dev, linuxarm

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.


^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2021-04-21 11:26 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05  7:33 [dpdk-dev] [PATCH] app/testpmd: support Tx mbuf free on demand cmd Lijun Ou
2021-03-05  7:46 ` Li, Xiaoyun
2021-03-05  9:58   ` oulijun
2021-03-05  9:57 ` [dpdk-dev] [PATCH V2] " Lijun Ou
2021-03-08 17:33   ` Ferruh Yigit
2021-03-09  8:49     ` oulijun
2021-03-09  9:53       ` Ferruh Yigit
2021-03-09  9:57         ` Thomas Monjalon
2021-03-09 10:18           ` Andrew Rybchenko
2021-03-09 14:00         ` Aaron Conole
2021-03-09 14:13           ` Ferruh Yigit
2021-03-10  1:48         ` oulijun
2021-03-10  7:59           ` Thomas Monjalon
2021-03-12 10:29             ` [dpdk-dev] [Linuxarm] " oulijun
2021-03-12 11:21               ` Thomas Monjalon
2021-03-17 11:30                 ` oulijun
2021-03-17 12:07                   ` Thomas Monjalon
2021-03-18  3:56                     ` oulijun
2021-03-18  7:51                       ` Thomas Monjalon
2021-04-12 13:12   ` [dpdk-dev] [PATCH V3] " Lijun Ou
2021-04-19  3:11     ` Li, Xiaoyun
2021-04-19 12:40       ` oulijun
2021-04-19 14:56         ` Ferruh Yigit
2021-04-19 12:36     ` [dpdk-dev] [PATCH V4] " Lijun Ou
2021-04-19 15:28       ` Ferruh Yigit
2021-04-21  1:44         ` oulijun
2021-04-21  8:09       ` [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command Lijun Ou
2021-04-21  8:15         ` Ferruh Yigit
2021-04-21  8:32           ` oulijun
2021-04-21  8:45         ` [dpdk-dev] [PATCH V6] " Lijun Ou
2021-04-21 11:26           ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git