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

[Ferruh Yigit]

On 3/9/2021 8:49 AM, oulijun wrote:
> 
> 
> 在 2021/3/9 1:33, Ferruh Yigit 写道:
>> On 3/5/2021 9:57 AM, Lijun Ou wrote:
>>> From: Chengwen Feng <fengchengwen at 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 at huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun at 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.