DPDK patches and discussions
 help / color / mirror / Atom feed
From: oulijun <oulijun@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: <xiaoyun.li@intel.com>, <dev@dpdk.org>, <linuxarm@openeuler.org>,
	"Andrew Rybchenko" <arybchenko@solarflare.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	Aaron Conole <aconole@redhat.com>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
Subject: Re: [dpdk-dev] [PATCH V2] app/testpmd: support Tx mbuf free on demand cmd
Date: Wed, 10 Mar 2021 09:48:00 +0800
Message-ID: <f9de9d21-15ae-58d3-a051-e65649770f60@huawei.com> (raw)
In-Reply-To: <35583d9b-0782-74a7-95df-aef1ca35f819@intel.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?
> 

  parent reply	other threads:[~2021-03-10  1:48 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-05  7:33 [dpdk-dev] [PATCH] " Lijun Ou
2021-03-05  7:46 ` Li, Xiaoyun
2021-03-05  9:58   ` oulijun
2021-03-05  9:57 ` [dpdk-dev] [PATCH V2] " Lijun Ou
2021-03-08 17:33   ` Ferruh Yigit
2021-03-09  8:49     ` oulijun
2021-03-09  9:53       ` Ferruh Yigit
2021-03-09  9:57         ` Thomas Monjalon
2021-03-09 10:18           ` Andrew Rybchenko
2021-03-09 14:00         ` Aaron Conole
2021-03-09 14:13           ` Ferruh Yigit
2021-03-10  1:48         ` oulijun [this message]
2021-03-10  7:59           ` Thomas Monjalon
2021-03-12 10:29             ` [dpdk-dev] [Linuxarm] " oulijun
2021-03-12 11:21               ` Thomas Monjalon
2021-03-17 11:30                 ` oulijun
2021-03-17 12:07                   ` Thomas Monjalon
2021-03-18  3:56                     ` oulijun
2021-03-18  7:51                       ` Thomas Monjalon
2021-04-12 13:12   ` [dpdk-dev] [PATCH V3] " Lijun Ou
2021-04-19  3:11     ` Li, Xiaoyun
2021-04-19 12:40       ` oulijun
2021-04-19 14:56         ` Ferruh Yigit
2021-04-19 12:36     ` [dpdk-dev] [PATCH V4] " Lijun Ou
2021-04-19 15:28       ` Ferruh Yigit
2021-04-21  1:44         ` oulijun
2021-04-21  8:09       ` [dpdk-dev] [PATCH V5] app/test-pmd: support cleanup txq mbufs command Lijun Ou
2021-04-21  8:15         ` Ferruh Yigit
2021-04-21  8:32           ` oulijun
2021-04-21  8:45         ` [dpdk-dev] [PATCH V6] " Lijun Ou
2021-04-21 11:26           ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f9de9d21-15ae-58d3-a051-e65649770f60@huawei.com \
    --to=oulijun@huawei.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=aconole@redhat.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=linuxarm@openeuler.org \
    --cc=thomas@monjalon.net \
    --cc=xiaoyun.li@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git