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 B49E2A0C45; Thu, 25 Nov 2021 12:12:56 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4128940DF5; Thu, 25 Nov 2021 12:12:56 +0100 (CET) Received: from mail-io1-f45.google.com (mail-io1-f45.google.com [209.85.166.45]) by mails.dpdk.org (Postfix) with ESMTP id 82F7340140 for ; Thu, 25 Nov 2021 12:12:54 +0100 (CET) Received: by mail-io1-f45.google.com with SMTP id x10so6986640ioj.9 for ; Thu, 25 Nov 2021 03:12:54 -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=0M8ZsOuCQ/Yb+KmTEi7HLTtz8vRKNDjc0RkyEi10A7I=; b=hpNGEwZhksjgPscc/3/swxJfSmACWbVBCbHQUa0HjKyCbLnm9mXgOAbWYh2k//seTM VQRJ+gcUXpFWr7ddoZ99FyC9d58FWO/Euysi2LyLtDA/E2M+J0DgjPdK4+4P3GqTlCvp ui1NQjPLbAFrsx5G+TKpCxr6ZnRuwG9O817EPAFVui6dNd27sWl+vTnnCxZeGKbxPWe9 PPHfCvxDFUcpirPskEX3AovUGcGVOIBct31zk+Bp/pc+zrRpBZLA/m8XqFvkO9eRuup9 bJJrRU0klkr3I5kufqeJ4yqtkxKm21y/WgNrC7cQJCA+BvPwAmYJNuk5/puJ1E3UsrBQ bJTQ== 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=0M8ZsOuCQ/Yb+KmTEi7HLTtz8vRKNDjc0RkyEi10A7I=; b=nbNqibwXuZcp2WFBwJbcZTO3VuX3H7aEtSJTTDxsIHzOPsXfFRITmAWRozo9Ac6CSC QYrZzdELK/MIHT5Nilu2ar265ei7Y5NUqIbH3F2hcRLC794gzedpO1mvQh4TNHh6Ep1e JXZSUwfVtlQSoHzs+eAhDKNtxFMd/bgNkRjt0buBCSF5E6ZnTvGJobsNtlpKNaS8P6ht iA+Eh1NPiAk/3JK474uuSWnXB3vqwDgOrlBHBjk43QKuxMf1CTxAR1NihuJnd7U9bjOF C5lUwo28loYD6RpocLFMI1ULxfFFrYJkRma8RMs6Z0re75dqrMu8VoWDAza3Q3hxjDGr Ma9Q== X-Gm-Message-State: AOAM531SgcYXn5yffcbL78pDL/iSHHYsyoQfCv4SJXjkwfwIz7xFwF3H eOmzQWka5ZUu5GHapMto93GhwbKVopzHRLayN9I= X-Google-Smtp-Source: ABdhPJyvBvtk6YBeE7lSYoFJe6voKrIZGcO0E4nOuj/rsDL+GrC3lbH5m/D6ZpnhIkutpmpIbdRX1eOYenpc9oIvoB8= X-Received: by 2002:a02:c60b:: with SMTP id i11mr29229911jan.15.1637838773711; Thu, 25 Nov 2021 03:12:53 -0800 (PST) MIME-Version: 1.0 References: <20211005125923.2651449-1-jerinj@marvell.com> In-Reply-To: From: Jerin Jacob Date: Thu, 25 Nov 2021 16:42:27 +0530 Message-ID: Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: support priority based flow control To: Ori Kam Cc: Jerin Jacob , dpdk-dev , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , Ferruh Yigit , 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 , "NBU-Contact-longli (EXTERNAL)" , 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 , Slava 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 Wed, Nov 24, 2021 at 9:30 PM Ori Kam wrote: > > > > > -----Original Message----- > > From: Jerin Jacob > > Sent: Wednesday, November 24, 2021 12:48 PM > > > > On Wed, Nov 24, 2021 at 3:01 PM Ori Kam wrote: > > > > > > Hi Jerin, > > > > > > > -----Original Message----- > > > > From: Jerin Jacob > > > > Sent: Tuesday, November 23, 2021 12:58 PM > > > > To: Ori Kam > > > > > > > > On Sun, Nov 21, 2021 at 3:20 PM Ori Kam wrote: > > > > > > > > > > Hi Jerin, > > > > > > > > Hi Ori, > > > > > > > > > > > > > > Sorry for my late response, > > > > > > > > Thanks for the review. > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Jerin Jacob > > > > > > Sent: Wednesday, November 17, 2021 11:49 AM > > > > > > To: Jerin Jacob > > > > > > Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: support priority based flow control > > > > > > > > > > > > On Tue, Oct 5, 2021 at 6:32 PM wrote: > > > > > > > > > > > > > > From: Jerin Jacob > > > > > > > > > > > > > > rte_eth_dev_priority_flow_ctrl_set() based API is not generic as it > > > > > > > can not support other than VLAN priority mapping to PFC traffic class. > > > > > > > > > > > > > > Introducing RTE_FLOW_ACTION_TYPE_PFC_SET_TC rte_flow action to > > > > > > > set the traffic class as per 802.1Qbb specification. This will enable, > > > > > > > Traffic class(8bit) to be selected based on any packet field like DSCP. > > > > > > > > > > > > > > Also, making it as rte_flow action will enable fine control on > > > > > > > traffic class selection to a specific queue or VF etc. > > > > > > > > > > > > > > Signed-off-by: Jerin Jacob > > > > > > > > > > > > Ping. If there are no comments on RFC, Planning to send v1 for 22.02. > > > > > > > > > > All the set type of functions are going to be deprecated. > > > > > you should use RTE_FLOW_ACTION_TYPE_MODIFY_FIELD. > > > > > > > > > > What is the item that you are matching on when using rte_flow? Is it part of the tci in the vlan > > item? > > > > > > > > TC can be VLAN TCI field or DSCP field in IP header or any other field > > > > in packet. > > > > We need to set the traffic class as per 802.1Qbb specification, May I > > > > know how the "modify" > > > > attribute helps here. It should be a "set" operation. Right? > > > > > > > > > > Yes, in the rte_flow_action_modify_field there is what operation you want to do, > > > in this case the action should be set. > > > > But RTE_FLOW_ACTION_TYPE_MODIFY_FIELD used for modify the packet > > content[1]. RIght? > > In this case, it is more of sideband data not anything on packet > > content. If so, explicit action > > makes sense. Right? > > > > It looks like I'm missing something, > If you don't want to change the packet and this is just data, > why not use tag/mark/flag/metadata? > > Who should get this data? > If the packet is hairpined and the packet is sent to wire this info should be part > of the packet right? No. Here is what I envisioned for working this, User add riles like this. Patten: VLAN TCI is value X or DSCP value Y Action: RTE_FLOW_ACTION_TYPE_PFC_SET_TC with an value for TC(8bit defined in 802.1Qbb) Driver use this rule to enable TC (flow control) with that value for the given VLAN TCI == X tag/mark/flag/metadata used to embed something in mbuf. Here, This action establishes, For a given flow what TC value needs to be enabled(it does not need to be given in mbuf or packet for application to use). It just establishes the TC wiring for flow control enablement for a given pattern. Is it adding up? > > > > > [1] > > * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD > > * > > * Modify a destination header field according to the specified > > * operation. Another field of the packet can be used as a source as well > > * as tag, mark, metadata, immediate value or a pointer to it. > > > > > > > > I assume that you are trying to set the VLAN tag priority field right? > > > > Both VLAN tag and/or DSCP field. > > > > Going back to the above comment so you are changing something in the packet. No. See above. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > Planning to submit the testpmd and cnxk ethdev driver changes after receiving > > > > > > > the feedback on this. > > > > > > > > > > > > > > > > > > > > > doc/guides/prog_guide/rte_flow.rst | 24 ++++++++++++++++++++++++ > > > > > > > lib/ethdev/rte_flow.c | 1 + > > > > > > > lib/ethdev/rte_flow.h | 27 +++++++++++++++++++++++++++ > > > > > > > 3 files changed, 52 insertions(+) > > > > > > > > > > > > > > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > > > > > > > index 2b42d5ec8c..e59f8a2902 100644 > > > > > > > --- a/doc/guides/prog_guide/rte_flow.rst > > > > > > > +++ b/doc/guides/prog_guide/rte_flow.rst > > > > > > > @@ -2999,6 +2999,30 @@ which is set in the packet meta-data (i.e. struct > > > > ``rte_mbuf::sched::color``) > > > > > > > | ``meter_color`` | Packet color | > > > > > > > +-----------------+--------------+ > > > > > > > > > > > > > > +Action: ``PFC_SET_TC`` > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^ > > > > > > > + > > > > > > > +Set traffic class as per PFC (802.1Qbb) specification. > > > > > > > + > > > > > > > +This action must be used with any of the following action. > > > > > > > + > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_QUEUE`` > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_RSS`` > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_PF`` > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_VF`` > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_PHY_PORT`` > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_PORT_ID`` > > > > > > > + > > > > > > > > > > Why? All the above actions are terminating actions so if I want ot match on the value > > > > > it doesn't make sense to have it only on the last rule. > > > > > > > > In PFC, we are specifying, Given TC needs to steer to specific Queue, > > > > RSS, PF etc. > > > > Not sure how other actions are relevant for SET_TC action. Do you have any > > > > specific action in mind where SET_TC valid in addition to above actions > > > > > > > First what happens in case of egress traffic? There is no dest action. > > > > It will be invalid. I can change the documentation to specify egress > > direction is not valid. > > Thoughts? > > > > Why not? Isn't it possible that application will want to send some packet with this value? This is Rx Flow control(8bit TC value defined in 802.1Qbb), Not relevant when using on Tx. > > > > Second what happens if for example the priority is based on the outer tunnel > > > which I want decap and at a latter stage I want to do connection tracking and only > > > if everything is correct I want to send this packet to a queue? > > > > Which is fine with the current scheme of things as per the documentation, > > "This action must be used with any of the following action." it does > > not preclude to > > use of any other action. If it is not clear, we can reword like below, > > --- > > This action must be used with any of the following action and not limited to > > using any of other actions in conjunction with the following action. > > --- > > Thoughts? > > > > Like stated above I can see use case where you want to set this value at the start > of the pipe and then based on this value act. > > For example: > 1. decap the packet and based on the tunnel set this value and jump to connection tracking group. > 2. run connection tracking and jump to next table > 3. Based on the connection tracking and the TC value send to some queue. Yes. It is possible to have decap + connection tracking + RTE_FLOW_ACTION_TYPE_PFC_SET_TC + [RTE_FLOW_ACTION_TYPE_QUEUE or RTE_FLOW_ACTION_TYPE_RSS or RTE_FLOW_ACTION_TYPE_PF or RTE_FLOW_ACTION_TYPE_VF or RTE_FLOW_ACTION_TYPE_PHY_PORT or RTE_FLOW_ACTION_TYPE_PORT_ID] cascaded actions. > > Best, > Ori > > > > > > > > > > > > > > > > > > > > > > > +.. _table_rte_flow_action_pfc_set_tc: > > > > > > > + > > > > > > > +.. table:: PFC_SET_PRIORITY > > > > > > > + > > > > > > > + +-----------------+-------------------------------------+ > > > > > > > + | Field | Value | > > > > > > > + +=================+=====================================+ > > > > > > > + | ``tc`` | Traffic class as per PFC (802.1Qbb) | > > > > > > > + +-----------------+-------------------------------------+ > > > > > > > + > > > > > > > Negative types > > > > > > > ~~~~~~~~~~~~~~ > > > > > > > > > > > > > > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c > > > > > > > index 8cb7a069c8..75c661159e 100644 > > > > > > > --- a/lib/ethdev/rte_flow.c > > > > > > > +++ b/lib/ethdev/rte_flow.c > > > > > > > @@ -189,6 +189,7 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = { > > > > > > > */ > > > > > > > MK_FLOW_ACTION(INDIRECT, 0), > > > > > > > MK_FLOW_ACTION(CONNTRACK, sizeof(struct rte_flow_action_conntrack)), > > > > > > > + MK_FLOW_ACTION(PFC_SET_TC, sizeof(struct rte_flow_action_pfc_set_tc)), > > > > > > > }; > > > > > > > > > > > > > > int > > > > > > > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h > > > > > > > index 7b1ed7f110..5298418e9e 100644 > > > > > > > --- a/lib/ethdev/rte_flow.h > > > > > > > +++ b/lib/ethdev/rte_flow.h > > > > > > > @@ -2409,6 +2409,13 @@ enum rte_flow_action_type { > > > > > > > * See struct rte_flow_action_meter_color. > > > > > > > */ > > > > > > > RTE_FLOW_ACTION_TYPE_METER_COLOR, > > > > > > > + > > > > > > > + /** > > > > > > > + * Set traffic class as per PFC (802.1Qbb) specification. > > > > > > > + * > > > > > > > + * See struct rte_flow_action_pfc_set_tc. > > > > > > > + */ > > > > > > > + RTE_FLOW_ACTION_TYPE_PFC_SET_TC, > > > > > > > }; > > > > > > > > > > > > > > /** > > > > > > > @@ -3168,6 +3175,26 @@ struct rte_flow_action_meter_color { > > > > > > > enum rte_color color; /**< Packet color. */ > > > > > > > }; > > > > > > > > > > > > > > +/** > > > > > > > + * @warning > > > > > > > + * @b EXPERIMENTAL: this structure may change without prior notice > > > > > > > + * > > > > > > > + * RTE_FLOW_ACTION_TYPE_PFC_SET_TC > > > > > > > + * > > > > > > > + * Set traffic class as per PFC (802.1Qbb) specification. > > > > > > > + * > > > > > > > + * This action must be used any of the following action. > > > > > > > + * - RTE_FLOW_ACTION_TYPE_QUEUE, > > > > > > > + * - RTE_FLOW_ACTION_TYPE_RSS, > > > > > > > + * - RTE_FLOW_ACTION_TYPE_PF, > > > > > > > + * - RTE_FLOW_ACTION_TYPE_VF, > > > > > > > + * - RTE_FLOW_ACTION_TYPE_PHY_PORT, > > > > > > > + * - RTE_FLOW_ACTION_TYPE_PORT_ID > > > > > > > + */ > > > > > > > > > > What does it mean? I must use it only on rules that have one of the above actions? > > > > > > > > See above. > > > > > > > > > > > > > > > > +struct rte_flow_action_pfc_set_tc { > > > > > > > + uint8_t tc; /**< Traffic class as per PFC (802.1Qbb) specification */ > > > > > > > +}; > > > > > > > + > > > > > > > /** > > > > > > > * Field IDs for MODIFY_FIELD action. > > > > > > > */ > > > > > > > -- > > > > > > > 2.33.0 > > > > > > > > > > > > > > > > > Best, > > > > > Ori