[V2] app/testpmd: support Tx mbuf free on demand cmd
Checks
Commit Message
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(+)
Comments
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.
@@ -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,
@@ -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
@@ -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
~~~~~~~~~~~