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 AF622A0562; Tue, 23 Mar 2021 11:20:10 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 34DA5140D47; Tue, 23 Mar 2021 11:20:07 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id CC54D140D36 for ; Tue, 23 Mar 2021 11:20:05 +0100 (CET) IronPort-SDR: UNbk1QA/WfYTzn7nk3R56K8z6DioyMRg8ynaHcRJx1fEKEk44kV+wLkIBFRzvHSZpfD3rTDTFi c/Mw1eSi+xPg== X-IronPort-AV: E=McAfee;i="6000,8403,9931"; a="190475147" X-IronPort-AV: E=Sophos;i="5.81,271,1610438400"; d="scan'208";a="190475147" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2021 03:20:04 -0700 IronPort-SDR: Btv4Q26qCSh6iXwSfLz76o5g8PNwj/9MrwOKYq1mJRHRY6HWfY+0lJ3qiMk/ym+gxsWuSvj3X3 w9ij5jdXmUbA== X-IronPort-AV: E=Sophos;i="5.81,271,1610438400"; d="scan'208";a="604255481" 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:20:01 -0700 From: Ferruh Yigit 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 , "jerinj@marvell.com" , Bruce Richardson , Olivier Matz , Anatoly Burakov References: <1616070332-63414-1-git-send-email-oulijun@huawei.com> <74bd828c-9f2c-17ff-9f8e-d06643db04e5@oktetlabs.ru> <4fa64451-b647-c946-733e-a95297cdfa4a@intel.com> X-User: ferruhy Message-ID: <5a386889-e562-7439-7101-a88d91da9413@intel.com> Date: Tue, 23 Mar 2021 10:19:57 +0000 MIME-Version: 1.0 In-Reply-To: <4fa64451-b647-c946-733e-a95297cdfa4a@intel.com> 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/23/2021 10:13 AM, Ferruh Yigit wrote: > 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. > Another high-level discussion is, testpmd keeps lots of config/state itself, I assume that is because it is not possible to get all DPDK config/state from DPDK library, but not sure if this is a design decision. Should we try to provide all config/state information via DPDK APIs, or should we push this responsibility to the application level?