[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 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
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
在 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 .
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
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 ~~~~~~~~~~~
在 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 ~~~~~~~~~~~
.
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.
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.
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
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.
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.
在 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?
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?
在 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
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
在 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
.
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.
在 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?
.
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.
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
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
在 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
.
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
.
participants (7)
-
Aaron Conole -
Andrew Rybchenko -
Ferruh Yigit -
Li, Xiaoyun -
Lijun Ou -
oulijun
-
Thomas Monjalon