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 9E74842C09; Fri, 2 Jun 2023 03:36:35 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 28754406B8; Fri, 2 Jun 2023 03:36:35 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 7437B40693 for ; Fri, 2 Jun 2023 03:36:33 +0200 (CEST) Received: from dggpeml500011.china.huawei.com (unknown [172.30.72.56]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4QXQWH3Z2hzLqKs; Fri, 2 Jun 2023 09:33:31 +0800 (CST) Received: from [10.67.101.191] (10.67.101.191) by dggpeml500011.china.huawei.com (7.185.36.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Fri, 2 Jun 2023 09:36:31 +0800 Message-ID: <72e9dda5-edf9-7cf6-a03f-3b46b2e9ccd7@huawei.com> Date: Fri, 2 Jun 2023 09:36:30 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v2 1/2] ethdev: add API to check queue ID validity To: Ferruh Yigit , Andrew Rybchenko , CC: , , , , , , , References: <20230516110021.1801443-1-huangdengdui@huawei.com> <20230522130947.345390-1-huangdengdui@huawei.com> <20230522130947.345390-2-huangdengdui@huawei.com> Content-Language: en-US From: huangdengdui In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.101.191] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpeml500011.china.huawei.com (7.185.36.84) X-CFilter-Loop: Reflected 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 On 2023/6/2 6:13, Ferruh Yigit wrote: > On 5/31/2023 5:31 PM, Ferruh Yigit wrote: >> On 5/22/2023 2:58 PM, Andrew Rybchenko wrote: >>> On 5/22/23 16:09, Dengdui Huang wrote: >>>> The API rte_eth_dev_is_valid_rxq/txq checks >>>> the port ID validity and then the Rx/Tx queue ID is valid. >>> >>> What is valid Tx/Rx queue? It depends on on caller >>> expectations. Some functions are satisfied with just >>> check vs configured number of queues. Some require >>> the queue to be setup. May be some should require >>> the queue to be started. >>> >>> So, I suggest to avoid term "valid" and be more precise >>> here and API naming. >>> >> >> I understand the concern 'valid' keyword, but we already have an API as >> 'rte_eth_dev_is_valid_port()', which does similar checks, >> >> so 'rte_eth_dev_is_valid_rxq()' & 'rte_eth_dev_is_valid_txq()' looks >> consistent with it. >> >> v3 has API names, 'rte_eth_dev_rxq_avail()' & 'rte_eth_dev_txq_avail()', >> I am not sure about these naming too, it feels like queues are valid but >> it maybe in available and not available states. >> >> >> @Andrew, do you have any suggestion on the API naming? >> If not I am for going with rte_eth_dev_is_valid_rxq()' & >> 'rte_eth_dev_is_valid_txq()' mainly because of existing >> 'rte_eth_dev_is_valid_port()' API. >> >> Perhaps we can elaborate what 'valid' means in API documentation to help >> users. >> > > Hi Dengdui, > > It looks like there is no better suggestion, lets not block this patch > more and continue with > 'rte_eth_dev_is_valid_rxq()' & 'rte_eth_dev_is_valid_txq()' API names. > > Can you please send a v4, with changes in v3 but API names as above, and > more description in the API documentation for what 'valid' means? > Hi Ferruh, Thanks for your review, I will do in v4. > >>>> >>>> Signed-off-by: Dengdui Huang >>>> --- >>>>   doc/guides/rel_notes/release_23_07.rst |  5 ++++ >>>>   lib/ethdev/rte_ethdev.c                | 30 +++++++++++++++++++++ >>>>   lib/ethdev/rte_ethdev.h                | 36 ++++++++++++++++++++++++++ >>>>   lib/ethdev/version.map                 |  4 +++ >>>>   4 files changed, 75 insertions(+) >>>> >>>> diff --git a/doc/guides/rel_notes/release_23_07.rst >>>> b/doc/guides/rel_notes/release_23_07.rst >>>> index a9b1293689..19e645156f 100644 >>>> --- a/doc/guides/rel_notes/release_23_07.rst >>>> +++ b/doc/guides/rel_notes/release_23_07.rst >>>> @@ -56,6 +56,11 @@ New Features >>>>        ======================================================= >>>>     +* **Added ethdev Rx/Tx queue id check API.** >>>> + >>>> +  Added ethdev Rx/Tx queue id check API which provides functions >>> >>> id -> ID >>> >>>> +  for check if Rx/Tx queue id is valid. >>> >>> id -> ID >>> >>>> + >>> >>> It should be two empty lines here and just one above. >>> >>>>   Removed Items >>>>   ------------- >>>>   diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >>>> index 4d03255683..3d85218127 100644 >>>> --- a/lib/ethdev/rte_ethdev.c >>>> +++ b/lib/ethdev/rte_ethdev.c >>>> @@ -407,6 +407,36 @@ rte_eth_dev_is_valid_port(uint16_t port_id) >>>>       return is_valid; >>>>   } >>>>   +int >>>> +rte_eth_dev_is_valid_rxq(uint16_t port_id, uint16_t queue_id) >>>> +{ >>>> +    struct rte_eth_dev *dev; >>>> + >>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>>> +    dev = &rte_eth_devices[port_id]; >>>> + >>>> +    if (queue_id >= dev->data->nb_rx_queues || >>>> +            dev->data->rx_queues[queue_id] == NULL) >>> >>> We already have internal eth_dev_validate_tx_queue(). Shouldn't >>> it be used here? >>> >>> Also, some functions check that queues array is not NULL. >>> If the the is excessive after queue number check, it >>> should be consistent everywhere and corresponding check >>> of the array pointer vs NULL should be removed in a separate >>> cleanup patch. If the check is required in some corner cases >>> (I hope no), it should be here as well. >>> >>>> +        return -EINVAL; >>>> + >>>> +    return 0; >>>> +} >>> >>> [snip] >>> >> >