From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E7E96A0567; Tue, 9 Mar 2021 15:13:24 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5CDEF4069D; Tue, 9 Mar 2021 15:13:24 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id E74074068A for ; Tue, 9 Mar 2021 15:13:22 +0100 (CET) IronPort-SDR: 5EOSMtABmGeaUwc2ZUaHG8ROgB5HHmw1xp0bzBrtVvBomqBiFyFd2i2mZeAayparlalCR4lWys Pg2+89UXrQ8Q== X-IronPort-AV: E=McAfee;i="6000,8403,9917"; a="249616888" X-IronPort-AV: E=Sophos;i="5.81,234,1610438400"; d="scan'208";a="249616888" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Mar 2021 06:13:21 -0800 IronPort-SDR: Ujb4A6Vy6YxyXzb4qReuwDpwYOoBMDQemVAlRvJAu1yVWMxCEQ1nlYDsSYgivWraqFsXe8dKsC gr0Zjv5nDY9Q== X-IronPort-AV: E=Sophos;i="5.81,234,1610438400"; d="scan'208";a="409747430" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.196.3]) ([10.213.196.3]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Mar 2021 06:13:19 -0800 To: Aaron Conole Cc: oulijun , xiaoyun.li@intel.com, dev@dpdk.org, linuxarm@openeuler.org, Andrew Rybchenko , Thomas Monjalon , Honnappa Nagarahalli References: <1614929583-37727-1-git-send-email-oulijun@huawei.com> <1614938252-62955-1-git-send-email-oulijun@huawei.com> <35583d9b-0782-74a7-95df-aef1ca35f819@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: Date: Tue, 9 Mar 2021 14:13:15 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 3/9/2021 2:00 PM, Aaron Conole wrote: > Ferruh Yigit 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 >>>>> >>>>> 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 >>>>> Signed-off-by: Lijun Ou >>>>> --- >>>>> 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 >>>>>   #include >>>>>   #include >>>>> +#include >>>> >>>> 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 >>>>>   #include >>>>>   #include >>>>> @@ -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. >