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 1D308A04A5; Tue, 25 Jan 2022 19:53:26 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 99C644117D; Tue, 25 Jan 2022 19:53:25 +0100 (CET) Received: from mail-il1-f172.google.com (mail-il1-f172.google.com [209.85.166.172]) by mails.dpdk.org (Postfix) with ESMTP id B8EC941161 for ; Tue, 25 Jan 2022 19:53:23 +0100 (CET) Received: by mail-il1-f172.google.com with SMTP id d3so17636442ilr.10 for ; Tue, 25 Jan 2022 10:53:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9AeW9W+xT/qaAiwUstLvcFLGjrn/CJ4gjS5vWg7rirs=; b=F5UB0pGl6U29QMAhBrmtN81uvYx0WJQiWHwPZ5Ha8RLxlKoyNcFyzXrcengSSZoucG CNQL+CftIwTVFEVmdi46EOsVTzj6DmZPc7ZgIbjt0ta+GrAoEo8fP2ft1DUEGb4zuxxO vnXj27Fwx0Bf0uHxxfc1+wdu5s39ZRJ8kI7Dc0Rd0BAjbOptkGzzyCA2L9NR6jUuTD9v pQuuUBUDb7kfbOcyeNVd2dPeDJp2i/AYA112BOzfATPqM4pXkBvLBko02Egk7b5hPW38 Ao2UbtsWZy3Ro3kuTtn+/NyIyJ88bnpsgbq+s56MZnnEKkPB7gTyHIUHh2btk26EWKYq KP4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9AeW9W+xT/qaAiwUstLvcFLGjrn/CJ4gjS5vWg7rirs=; b=dqYF/5OCDBx1d0PgMM4tEtmyvL0398G9oCIQ99QiHst93IOcMkUOKUig0EGTwC+mls hwQWhYepLYey/dhvasjn34c3iIm/LY5DfhLia7V5ldm2L8wkZKMoYv2AZhLR01TD4IyX bji5Xm0PlTa3gn3rJJRhPE3BbgO+dVX1v9zO9QkXZLqKr0MW+/fECK0vONYMpMMnZ7SR Fh3APuOYxHdTpO2AC4srJ/4NHzpfBxYrfnQQp3FpMiKsFJYP+UHzAv0iKMJuCnpp+HNW o6sMo1D5QwaShZo5TEfOZLK9a5u7W4CIqgFprcM7DhUoyElRx9NUdlf/F7/trVl4Wk9y hflg== X-Gm-Message-State: AOAM531zuEsS396wpG0XXCoYR+Wjc184zGDCGrA4cdb1y0I0DNNe2kJn N58pOsPgwRelEsCEVdh9isazC672D4ZI8rtMjsc= X-Google-Smtp-Source: ABdhPJzel9/7c7y4gsoPquvnTOkmSyxb5+Xycf2ZegrKFj3WMcj2M/1g3IEfzT8MHap0L2eTt/v3/dLs3ummGRN+uwU= X-Received: by 2002:a05:6e02:1809:: with SMTP id a9mr12699989ilv.318.1643136802977; Tue, 25 Jan 2022 10:53:22 -0800 (PST) MIME-Version: 1.0 References: <20220109105851.734687-1-skori@marvell.com> <20220113102718.3167282-1-jerinj@marvell.com> <000cbfe1-9d91-3011-4cf9-56dbeebcebb6@intel.com> In-Reply-To: <000cbfe1-9d91-3011-4cf9-56dbeebcebb6@intel.com> From: Jerin Jacob Date: Wed, 26 Jan 2022 00:22:56 +0530 Message-ID: Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: support queue-based priority flow control To: Ferruh Yigit Cc: Jerin Jacob , dpdk-dev , Ray Kinsella , Thomas Monjalon , Andrew Rybchenko , Ajit Khaparde , Andrew Boyer , Beilei Xing , "Richardson, Bruce" , Chas Williams , "Xia, Chenbo" , Ciara Loftus , Devendra Singh Rawat , Ed Czeck , Evgeny Schemeilin , Gaetan Rivet , Gagandeep Singh , Guoyang Zhou , Haiyue Wang , Harman Kalra , heinrich.kuhn@corigine.com, Hemant Agrawal , Hyong Youb Kim , Igor Chauskin , Igor Russkikh , Jakub Grajciar , Jasvinder Singh , Jian Wang , Jiawen Wu , Jingjing Wu , John Daley , John Miller , "John W. Linville" , "Wiles, Keith" , Kiran Kumar K , Lijun Ou , Liron Himi , Long Li , Marcin Wojtas , Martin Spinler , Matan Azrad , Matt Peters , Maxime Coquelin , Michal Krawczyk , "Min Hu (Connor" , Pradeep Kumar Nalla , Nithin Dabilpuram , Qiming Yang , Qi Zhang , Radha Mohan Chintakuntla , Rahul Lakkireddy , Rasesh Mody , Rosen Xu , Sachin Saxena , Satha Koteswara Rao Kottidi , Shahed Shaikh , Shai Brandes , Shepard Siegel , Somalapuram Amaranath , Somnath Kotur , Stephen Hemminger , Steven Webster , Sunil Kumar Kori , Tetsuya Mukawa , Veerasenareddy Burru , Viacheslav Ovsiienko , Xiao Wang , Xiaoyun Wang , Yisen Zhuang , Yong Wang , Ziyang Xuan Content-Type: text/plain; charset="UTF-8" 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 Tue, Jan 25, 2022 at 11:05 PM Ferruh Yigit wrote: > > On 1/13/2022 10:27 AM, jerinj@marvell.com wrote: > > From: Jerin Jacob > > > > Based on device support and use-case need, there are two different ways > > to enable PFC. The first case is the port level PFC configuration, in > > this case, rte_eth_dev_priority_flow_ctrl_set() API shall be used to > > configure the PFC, and PFC frames will be generated using based on VLAN > > TC value. > > > > The second case is the queue level PFC configuration, in this > > case, Any packet field content can be used to steer the packet to the > > specific queue using rte_flow or RSS and then use > > rte_eth_dev_priority_flow_ctrl_queue_set() to set the TC mapping on each > > queue. Based on congestion selected on the specific queue, configured TC > > shall be used to generate PFC frames. > > > Operation of these modes are mutually exclusive, when driver sets > > non zero value for rte_eth_dev_info::pfc_queue_tc_max, > > application must use queue level PFC configuration via > > rte_eth_dev_priority_flow_ctrl_queue_set() API instead of port level > > PFC configuration via rte_eth_dev_priority_flow_ctrl_set() API to > > realize PFC configuration. > > > > This patch enables the configuration for second case a.k.a queue > > based PFC also updates rte_eth_dev_priority_flow_ctrl_set() > > implmentaion to adheher to rte_eth_dev_info::pfc_queue_tc_max > > handling. > > > > Also updated libabigail.abignore to ignore the update > > to reserved fields in rte_eth_dev_info. > > > > Signed-off-by: Jerin Jacob > > Signed-off-by: Sunil Kumar Kori > > --- > > > > A driver implemtion based on this API is at > > https://patches.dpdk.org/project/dpdk/patch/20220111081831.881374-1-skori@marvell.com/ > > > > RFC..v1 > > - Added queue based PFC config API instead port based > > > > v1..v2 > > + > > /** > > * Tunnel type for device-specific classifier configuration. > > * @see rte_eth_udp_tunnel > > @@ -1841,8 +1863,30 @@ struct rte_eth_dev_info { > > * embedded managed interconnect/switch. > > */ > > struct rte_eth_switch_info switch_info; > > - > > - uint64_t reserved_64s[2]; /**< Reserved for future fields */ > > + /** > > + * Maximum supported traffic class as per PFC (802.1Qbb) specification. > > + * > > + * Based on device support and use-case need, there are two different > > + * ways to enable PFC. The first case is the port level PFC > > + * configuration, in this case, rte_eth_dev_priority_flow_ctrl_set() > > + * API shall be used to configure the PFC, and PFC frames will be > > + * generated using based on VLAN TC value. > > + * The second case is the queue level PFC configuration, in this case, > > + * Any packet field content can be used to steer the packet to the > > + * specific queue using rte_flow or RSS and then use > > + * rte_eth_dev_priority_flow_ctrl_queue_set() to set the TC mapping > > + * on each queue. Based on congestion selected on the specific queue, > > + * configured TC shall be used to generate PFC frames. > > + * > > + * When set to non zero value, application must use queue level > > + * PFC configuration via rte_eth_dev_priority_flow_ctrl_queue_set() API > > + * instead of port level PFC configuration via > > + * rte_eth_dev_priority_flow_ctrl_set() API to realize > > + * PFC configuration. > > + */ > > + uint8_t pfc_queue_tc_max; > > > 'rte_eth_dev_info_get()' is one of the APIs that anyone using ethdev needs to use. > > Instead of expanding it with less used features, what do you think to have a > specific API to get the 'pfc_queue_tc_max'? OK. rte_eth_dev_info_get() was general theme used in ethdev API. Fine for new API specifically for this. > > It also can be used by application to detect queue based PFC is supported by > driver or not. OK. > Assume API is 'rte_eth_dev_priority_flow_ctrl_get()', if it returns '-ENOTSUP' > application can know that PMD doesn't support queue based PFC. OK. > > > > + uint8_t reserved_8s[7]; > > + uint64_t reserved_64s[1]; /**< Reserved for future fields */ > > void *reserved_ptrs[2]; /**< Reserved for future fields */ > > }; > > > > @@ -4109,6 +4153,9 @@ int rte_eth_dev_flow_ctrl_set(uint16_t port_id, > > * Configure the Ethernet priority flow control under DCB environment > > * for Ethernet device. > > * > > + * @see struct rte_eth_dev_info::pfc_queue_tc_max priority > > + * flow control usage models. > > + * > > * @param port_id > > * The port identifier of the Ethernet device. > > * @param pfc_conf > > @@ -4119,10 +4166,40 @@ int rte_eth_dev_flow_ctrl_set(uint16_t port_id, > > * - (-ENODEV) if *port_id* invalid. > > * - (-EINVAL) if bad parameter > > * - (-EIO) if flow control setup failure or device is removed. > > + * > > */ > > int rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id, > > - struct rte_eth_pfc_conf *pfc_conf); > > + struct rte_eth_pfc_conf *pfc_conf); > > Above syntax changes are not needed. > > DPDK coding convention is using tabs (mostly two) for multi line function > decleration/definition, please be consistant with usage, this patch has > multiple variations. Ack. > > > > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice. > > + * > > + * Configure the Ethernet priority flow control for a given queue > > + * for Ethernet device. > > + * > > + * @see struct rte_eth_dev_info::pfc_queue_tc_max priority flow control > > + * usage models. > > + * > > + * @note When an ethdev port switches to PFC mode, the unconfigured > > Doesit mean queue based PFC mode? Yes. I will change to PFC mode -> queue-based PFC mode. > > > + * queues shall be configured by the driver with default values such as > > + * lower priority value for TC etc. > > + * > > I assume there is no way for application to know what the defaults values are, > also not sure if application interested in this. Yes. I don't think the application cares. But added in the doc to clarify the state. > > > + * @param port_id > > + * The port identifier of the Ethernet device. > > + * @param pfc_queue_conf > > + * The pointer to the structure of the priority flow control parameters > > + * for the queue. > > + * @return > > + * - (0) if successful. > > + * - (-ENOTSUP) if hardware doesn't support priority flow control mode. > > + * - (-ENODEV) if *port_id* invalid. > > + * - (-EINVAL) if bad parameter > > + * - (-EIO) if flow control setup queue failure > > + */ > > +__rte_experimental > > +int rte_eth_dev_priority_flow_ctrl_queue_set(uint16_t port_id, > > + struct rte_eth_pfc_queue_conf *pfc_queue_conf); > > I wonder if Rx/Tx queue id should be API arguments, to be consistent > with some other APIs, and will it help application that configures queues > in a loop. > But I can see 'rx_pause' or 'tx_pause' (in config) can be valid or not > based on the 'mode', so I understand to have queue ids in the struct. > No strong opinion. Yes. I keep it as is.