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 D5AB6A0562; Tue, 23 Mar 2021 11:13:54 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C09304014D; Tue, 23 Mar 2021 11:13:54 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id 0B84C40143 for ; Tue, 23 Mar 2021 11:13:52 +0100 (CET) IronPort-SDR: /JM0d4OegLUcjriEoPzhKzjwL8yk1GQCZRpOTqdIW/86kfw1wOoEHMsMGMIdMw2ewODIcMCF7a GFNLtEN3ioIw== X-IronPort-AV: E=McAfee;i="6000,8403,9931"; a="177572926" X-IronPort-AV: E=Sophos;i="5.81,271,1610438400"; d="scan'208";a="177572926" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2021 03:13:51 -0700 IronPort-SDR: +kWnvuQQOfddh20D3tTSjagZDgeSJczzI/6lmJoC38wUdYtJux6oPBIP5Bmw7ATLTQAN0IecaF e8G2zXxV/2DA== X-IronPort-AV: E=Sophos;i="5.81,271,1610438400"; d="scan'208";a="604254385" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.14.45]) ([10.252.14.45]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2021 03:13:49 -0700 To: "Ananyev, Konstantin" , Andrew Rybchenko , Lijun Ou , "thomas@monjalon.net" Cc: "dev@dpdk.org" , "linuxarm@openeuler.org" , Andrew Rybchenko , David Marchand , Ray Kinsella , Luca Boccassi , Ori Kam References: <1616070332-63414-1-git-send-email-oulijun@huawei.com> <74bd828c-9f2c-17ff-9f8e-d06643db04e5@oktetlabs.ru> From: Ferruh Yigit X-User: ferruhy Message-ID: <4fa64451-b647-c946-733e-a95297cdfa4a@intel.com> Date: Tue, 23 Mar 2021 10:13:45 +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] ethdev: add queue state when retrieve queue information 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/22/2021 6:53 PM, Ananyev, Konstantin wrote: > > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Monday, March 22, 2021 5:08 PM >> To: Ananyev, Konstantin ; Yigit, Ferruh ; Lijun Ou ; >> thomas@monjalon.net >> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko ; David Marchand >> ; Ray Kinsella ; Luca Boccassi >> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information >> >> On 3/22/21 7:53 PM, Ananyev, Konstantin wrote: >>> >>> >>>> -----Original Message----- >>>> From: Andrew Rybchenko >>>> Sent: Monday, March 22, 2021 4:02 PM >>>> To: Ananyev, Konstantin ; Yigit, Ferruh ; Lijun Ou ; >>>> thomas@monjalon.net >>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko ; David Marchand >>>> ; Ray Kinsella ; Luca Boccassi >>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information >>>> >>>> On 3/22/21 6:45 PM, Ananyev, Konstantin wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: dev On Behalf Of Andrew Rybchenko >>>>>> Sent: Monday, March 22, 2021 2:49 PM >>>>>> To: Yigit, Ferruh ; Lijun Ou ; thomas@monjalon.net >>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org; Andrew Rybchenko ; David Marchand >>>>>> ; Ray Kinsella ; Luca Boccassi >>>>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: add queue state when retrieve queue information >>>>>> >>>>>> On 3/22/21 12:22 PM, Ferruh Yigit wrote: >>>>>>> On 3/18/2021 12:25 PM, Lijun Ou wrote: >>>>>>>> Currently, upper-layer application could get queue state only >>>>>>>> through pointers such as dev->data->tx_queue_state[queue_id], >>>>>>>> this is not the recommended way to access it. So this patch >>>>>>>> add get queue state when call rte_eth_rx_queue_info_get and >>>>>>>> rte_eth_tx_queue_info_get API. >>>>>>>> >>>>>>>> Note: The hairpin queue is not supported with above >>>>>>>> rte_eth_*x_queue_info_get, so the queue state could be >>>>>>>> RTE_ETH_QUEUE_STATE_STARTED or RTE_ETH_QUEUE_STATE_STOPPED. >>>>>>>> Note: After add queue_state field, the 'struct rte_eth_rxq_info' size >>>>>>>> remains 128B, and the 'struct rte_eth_txq_info' size remains 64B, so >>>>>>>> it could be ABI compatible. >>>>>>>> >>>>>>>> Signed-off-by: Chengwen Feng >>>>>>>> Signed-off-by: Lijun Ou >>>>>>> >>>>>>> <...> >>>>>>> >>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h >>>>>>>> b/lib/librte_ethdev/rte_ethdev.h >>>>>>>> index efda313..3b83c5a 100644 >>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h >>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h >>>>>>>> @@ -1591,6 +1591,8 @@ struct rte_eth_rxq_info { >>>>>>>> uint8_t scattered_rx; /**< scattered packets RX supported. */ >>>>>>>> uint16_t nb_desc; /**< configured number of RXDs. */ >>>>>>>> uint16_t rx_buf_size; /**< hardware receive buffer size. */ >>>>>>>> + /**< Queues state: STARTED(1) / STOPPED(0). */ >>>>>>>> + uint8_t queue_state; >>>>>>>> } __rte_cache_min_aligned; >>>>>>>> /** >>>>>>>> @@ -1600,6 +1602,8 @@ struct rte_eth_rxq_info { >>>>>>>> struct rte_eth_txq_info { >>>>>>>> struct rte_eth_txconf conf; /**< queue config parameters. */ >>>>>>>> uint16_t nb_desc; /**< configured number of TXDs. */ >>>>>>>> + /**< Queues state: STARTED(1) / STOPPED(0). */ >>>>>>>> + uint8_t queue_state; >>>>>>>> } __rte_cache_min_aligned; >>>>>>>> /* Generic Burst mode flag definition, values can be ORed. */ >>>>>>>> >>>>>>> >>>>>>> This is causing an ABI warning [1], but I guess it is safe since the >>>>>>> size of the struct is not changing (cache align). Adding a few more >>>>>>> people to comment. >>>>>>> >>>>>>> >>>>>>> [1] >>>>>>> https://travis-ci.com/github/ovsrobot/dpdk/builds/220497651 >>>>>> >>>>>> Frankly speaking I dislike addition of queue_state as uint8_t. >>>>>> IMHO it should be either 'bool started' or enum to support more >>>>>> states in the future if we need. >>>>> >>>>> I think we already have set of defines for it: >>>>> lib/librte_ethdev/rte_ethdev_driver.h:925:#define RTE_ETH_QUEUE_STATE_STOPPED 0 >>>>> lib/librte_ethdev/rte_ethdev_driver.h:926:#define RTE_ETH_QUEUE_STATE_STARTED 1 >>>>> lib/librte_ethdev/rte_ethdev_driver.h:927:#define RTE_ETH_QUEUE_STATE_HAIRPIN 2 >>>>> >>>>> If we want to publish it, then might be enough just move these macros to rte_ethdev.h or so. >>>>> >>>>> About uint8_t vs enum - yes, in principle enum would be a bit nicer, >>>>> but right now rte_eth_dev_data.(rx|tx)_queue_state[] itself is an array of uint8_t. >>>>> So probably not much point to waste extra 3B in rte_eth_(rxq|txq)_info. >>>>> Unless in future will want to change it in struct rte_eth_dev_data too >>>>> (or even hide it inside dev private queue data). >>>> >>>> I forgot about hairpin and bitmask... If so, I think it is >>>> sufficient to fix absolutely misleading comment, say >>>> that it is a bit mask and think about removal of >>>> RTE_ETH_QUEUE_STATE_STOPPED (since it could be >>>> stopped+hairpin). May be consider to use uin16_t, >>>> since 8 bit is really small bitmask. It still fits in >>>> available hole. >>> >>> Hmm, as I can read the code - hairpin queue can't be started/stopped by SW, >>> and each of the states (stopped/started/hairpin) is mutually exclusive. >>> Is that not what was intended when hairpin queues were introduced? >>> >> >> Thanks, yes, you're right. My memory lies to me. If queue state >> is not a bit mask, it should be an enum from API point of view. >> Rx/Tx queue info structures are control path. I see no point to >> save bits here. Clear API is more important on control path. >> The only reason here to use uint8_t is to avoid ABI breakage. >> I can't judge if it is critical to wait or not. > > As alternate thought - introduce new API function, > something like: > int rte_eth_get_rxq_state(portid, queue_id); > Then rte_eth_dev_is_rx_hairpin_queue() probably can be deprecated > in favour of this new one. > > The 'rte_eth_dev_is_rx_hairpin_queue()' is internal function, and it is not visible to the application, it should be OK to keep it. But 'STATE_HAIRPIN' should be kept internal, or should be available to the application? The actual need is to know the start/stop state of the queue. That is for app to decide if 'rte_eth_tx_done_cleanup()' can be done or not an a queue: https://patches.dpdk.org/project/dpdk/patch/1614938252-62955-1-git-send-email-oulijun@huawei.com/ And normally I also prefer APIs with simple & clear responsibility, but this one seems very related to the existing '_queue_info_get()' ones, so I am fine with both options.