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 13C89A0567; Tue, 9 Mar 2021 10:53:58 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C984C22A493; Tue, 9 Mar 2021 10:53:57 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id 156EF22A443 for ; Tue, 9 Mar 2021 10:53:55 +0100 (CET) IronPort-SDR: bi5N3BzO0NcYRsBicnrpX4yCdf/XfZTQlorFf/Ne37uCRHUdXgqh1RW0Btsd27zMJ1mKk+LbrX mLtSi8toi7KA== X-IronPort-AV: E=McAfee;i="6000,8403,9917"; a="249575157" X-IronPort-AV: E=Sophos;i="5.81,234,1610438400"; d="scan'208";a="249575157" 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 01:53:54 -0800 IronPort-SDR: vjU84rj5K07KyFSeTxUnjMHizn1uBMe4hTkI+0eSBBT9+8NuRnyIjnuxDn4vAa4aXsF4e662m2 Y2+6Nx3HzTag== X-IronPort-AV: E=Sophos;i="5.81,234,1610438400"; d="scan'208";a="409669986" 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 01:53:52 -0800 To: oulijun Cc: xiaoyun.li@intel.com, dev@dpdk.org, linuxarm@openeuler.org, Andrew Rybchenko , Thomas Monjalon , Aaron Conole , Honnappa Nagarahalli References: <1614929583-37727-1-git-send-email-oulijun@huawei.com> <1614938252-62955-1-git-send-email-oulijun@huawei.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <35583d9b-0782-74a7-95df-aef1ca35f819@intel.com> Date: Tue, 9 Mar 2021 09:53:49 +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 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. 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 >>> 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.