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

[oulijun <oulijun@huawei.com>]

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