From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 14E2BA04BE; Mon, 7 Sep 2020 09:47:25 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 005732BAB; Mon, 7 Sep 2020 09:47:24 +0200 (CEST) Received: from huawei.com (szxga05-in.huawei.com [45.249.212.191]) by dpdk.org (Postfix) with ESMTP id 3520C29AC for ; Mon, 7 Sep 2020 09:47:22 +0200 (CEST) Received: from DGGEMS404-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id C32DDFF9FBE44A38FB43; Mon, 7 Sep 2020 15:47:20 +0800 (CST) Received: from [127.0.0.1] (10.65.81.238) by DGGEMS404-HUB.china.huawei.com (10.3.19.204) with Microsoft SMTP Server id 14.3.487.0; Mon, 7 Sep 2020 15:47:10 +0800 To: Matan Azrad , "dev@dpdk.org" References: <1592483709-7076-1-git-send-email-tangchengchang@huawei.com> <1598685199-1630-1-git-send-email-tangchengchang@huawei.com> <1598685199-1630-2-git-send-email-tangchengchang@huawei.com> <1a4dc7d6-5596-34cb-9eb1-adcd2adef2fb@huawei.com> CC: "maryam.tahhan@intel.com" , "linuxarm@huawei.com" , "ferruh.yigit@intel.com" , "wenzhuo.lu@intel.com" , NBU-Contact-Thomas Monjalon , "arybchenko@solarflare.com" From: Chengchang Tang Message-ID: <66ecf741-ac5b-cf61-66f3-c5df59c1bd49@huawei.com> Date: Mon, 7 Sep 2020 15:47:10 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.65.81.238] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Hi Matan On 2020/9/6 21:45, Matan Azrad wrote: > > Hi Chengchang > > From: Chengchang Tang: >> Hi, Matan >> >> On 2020/9/2 18:30, Matan Azrad wrote: >>> Hi Chengchang >>> >>> From: Chengchang Tang >>>> Hi, Matan >>>> >>>> On 2020/9/2 15:19, Matan Azrad wrote: >>>>> >>>>> Hi Chengchang >>>>> >>>>> From: Chengchang Tang >>>>>> Hi, Matan >>>>>> >>>>>> On 2020/9/1 23:33, Matan Azrad wrote: >>>>>>> >>>>>>> Hi Chengchang >>>>>>> >>>>>>> Please see some question below. >>>>>>> >>>>>>> From: Chengchang Tang >>>>>>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the >>>>>>>> buffer size used in receiving packets for HW. >>>>>>>> >>>>>>>> In this way, upper-layer users can get this information by >>>>>>>> calling rte_eth_rx_queue_info_get. >>>>>>>> >>>>>>>> Signed-off-by: Chengchang Tang >>>>>>>> Reviewed-by: Wei Hu (Xavier) >>>>>>>> Acked-by: Andrew Rybchenko >>>>>>>> --- >>>>>>>> lib/librte_ethdev/rte_ethdev.h | 2 ++ >>>>>>>> 1 file changed, 2 insertions(+) >>>>>>>> >>>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h >>>>>>>> b/lib/librte_ethdev/rte_ethdev.h index 70295d7..9fed5cb 100644 >>>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h >>>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h >>>>>>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info { >>>>>>>> struct rte_eth_rxconf conf; /**< queue config parameters. */ >>>>>>>> uint8_t scattered_rx; /**< scattered packets RX supported. >> */ >>>>>>>> uint16_t nb_desc; /**< configured number of RXDs. */ >>>>>>>> + /**< buffer size used for hardware when receive packets. */ >>>>>>>> + uint16_t rx_buf_size; >>>>>>> >>>>>>> Is it the maximum supported Rx buffer by the HW? >>>>>>> If yes, maybe max_rx_buf_size is better name? >>>>>> >>>>>> No, it is the Rx buffer size currently used by HW. >>> >>>>> Doesn't it defined by the user? Using Rx queue mem-pool mbuf room >> size? >>>>> >>>>> And it may be different per Rx queue.... >>>> >>>> Yes, it is defined by user using the Rx queue mem-pool mbuf room size. >>>> When different queues are bound to different mempools, different >>>> queues may have different value. >>>>> >>>>>> IMHO, the structure rte_eth_rxq_info and associated query API are >>>>>> mainly used to query HW configurations at runtime or after queue is >>>>>> configured/setup. Therefore, the content of this structure should >>>>>> be the current HW configuration. >>>>> >>>>> It looks me more like capabilities... >>>>> The one which define the current configuration is the user by the >>>> configuration APIs(after reading the capabilities). >>>> >>>> I prefer to consider the structure rte_eth_dev_info as the capabilities. >>> >>> Yes. >>> >>>> Because rxq_info and associated APIs are not available until the >>>> queue is configured. And the max rx_buf_size is already exists in >> dev_info. >>>>> >>>>> I don't think we have here all the current configurations, so what >>>>> is special >>>> in this one? >>>> >>>> I think the structure is used to store the queue-related >>>> configuration, especially the final HW configuration that may be >>>> different from user configuration and some configurations that are >>>> not mandatory for the user(PMDs will use a default configuration). >>>> Such as the scatterred_rx and rx_drop_en in rte_eth_rxconf, some PMDs >>>> will adjust it in some cases based on their HW constraints. >>> >>> Ok, this struct(struct rte_eth_rxq_info) is new for me. >>> Thanks for the explanation. >>> >>>> This configuration item meets the above criteria. The value range of >>>> rx_buf_size varies according to HW. Some HW may require 1k-alignment, >>>> while others may require several fixed values. So, the PMDs will >>>> configure it based on their HW constraints. This results in >>>> difference between the user configuration and the actual >>>> configuration and this value affects the memory usage and performance. >>>> I think there's a need for a way to expose that information. >>> >>> So the user can configure X and the driver will use Y!=X? >> >> Yes, it depends on the HW. In the queue setup API, it just checks whether >> the input is greater than the required minimum value. But HW usually has >> requirements for alignment and so on. >> So when X does not meet these requirements, PMDs will calculate a new >> value Y that meets these requirements to configure the hardware (Y <= X, to >> ensure no memory overflow occurs). >>> Should the application validate its own configurations after setting them >> successfully? >> >> It depends on their own needs. The application should not be forced to >> verify it to avoid affecting the ease of use of PMDs. For some applications, >> they don't care about this value. > > I understand, > It looks me like a bad ping-pong between app and PMD (for all the fields in the struct), > And we should avoid adding fields to this structure if we can. > > What's about adding field in rte_eth_dev_info to expose the rx buffer alignment supported by the PMD? > Then, application has all the knowledge you want to expose before the configuration. This may not work because there may be other restrictions besides alignment, which are related to the hardware design. Therefore, it is difficult to describe all constraints in a single field. Moreover, this approach seems to constrain the PMDs and HW to some extent. > >>> >>>>> >>>>>>> Maybe document that 0 means - no limitation by HW? >>>>>> >>>>>> Yes, there is no need to fill this filed for HW that has no restrictions on >> it. >>>>>> I'll add it in v4. >>>>>> >>>>>>> Must application read it in order to know if its datapath should >>>>>>> handle >>>>>> multi-segment buffers? >>>>>> >>>>>> I think it's more appropriate to use scattered_rx to determine if >>>>>> multi- segment buffers should be handled. >>>>>> >>>>>>> >>>>>>> Maybe it will be good to force application to configure scatter >>>>>>> when this >>>>>> field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room >> size)... >>>>> >>>>> Can you explain more what is the issue you came to solve? >>>> >>>> This HW information may be useful when there is some problems running >>>> a application. This structure and related APIs can be used to expose >>>> it at run time. >>>>> >>> OK >>> >>> . >>> > > > . >