From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f174.google.com (mail-wr0-f174.google.com [209.85.128.174]) by dpdk.org (Postfix) with ESMTP id 823647CF5 for ; Fri, 20 Apr 2018 10:55:11 +0200 (CEST) Received: by mail-wr0-f174.google.com with SMTP id h3-v6so20890292wrh.5 for ; Fri, 20 Apr 2018 01:55:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=b+Q1404UlZRC49/RSDKMToNXnyHForpyQ8f5Dht7RFk=; b=d7yicsJkmVzIuoau0WVLaToDdSrZkA2mSbPapplhYBAAulcTOcDXHHQb8k+1jdPkT1 I39f8ZR/3l+/fds32s4MicHLGZFqeMI64IQwiVfEj3/2z6dcd+PxKPzGa8FH+3/FqWi8 2mK8XjubWT1dNLwiPfO5LkfLryo0De1TG6PtG2Z6x6V2a+4sa9iKQ1ocJ03iRQZPt+wd xOqNM2EcGi9z1XeSGXBbCukR2zm+Hvrz1Xg8F5Xwv8kBrl0jmkuZKrQ7eKcitfevsYao YMJN6qZVOvdNAya9giaXl55ZvrElIynRVXdY/3JCGexARYxU5jnMIxo8TXAAiufnsCwG 1Jrw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=b+Q1404UlZRC49/RSDKMToNXnyHForpyQ8f5Dht7RFk=; b=Q4FPibaDmDABkmdd5M64UkEwAzBs5XBLgyYuGQtbh5A+/HhHCNDWtKHyuaxwKi8SUL kGNvWXShj5Djy8RX3rHuUOrqMZgnRgXiKSidFi5cx5UHcWQwQMJsr9G/8VuQ9cxqGy/L C5TPaRvCJUtlS6xUXdWut459ptFYlQYz7+9Et/RANUYLexsYt5yBAHdQUXkJZ4fBX9AE JiBfStvRilzY10NL5k1o/KDkEFLk66eifzs0un7kYAt8xNhSM11BUptNEU64KLPABYcp K8GLQFjniubOF1RGG9428af+CKUOS9+71L79AinRkA3m16QDJ5WNlPAQPIe6FihEY6Lw y7Eg== X-Gm-Message-State: ALQs6tCLjGBDsERJW+XrqDAiGCy3brNTGwPdHqqBNPagu9tdyC85zSpj ZTI+YFLGp4nNArkNBEfFI9SBNw== X-Google-Smtp-Source: AIpwx48iFlYnxPjXuduEX9X5AXG8PNRDG+caTYZlpGnptItEUC6PHG5esp0ln3AaqBZXh+k3iPea5Q== X-Received: by 10.28.91.65 with SMTP id p62mr1375336wmb.140.1524214511214; Fri, 20 Apr 2018 01:55:11 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id t76sm1635694wme.17.2018.04.20.01.55.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 20 Apr 2018 01:55:10 -0700 (PDT) Date: Fri, 20 Apr 2018 10:54:56 +0200 From: Adrien Mazarguil To: "Zhang, Qi Z" Cc: "dev@dpdk.org" , "Doherty, Declan" , "Chandran, Sugesh" , "Glynn, Michael J" , "Liu, Yu Y" , "Ananyev, Konstantin" , "Richardson, Bruce" Message-ID: <20180420085456.GK4957@6wind.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <20180416061042.785-1-qi.z.zhang@intel.com> <20180416061042.785-3-qi.z.zhang@intel.com> <20180419144843.GF4957@6wind.com> <039ED4275CED7440929022BC67E70611531A3BE2@SHSMSX103.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <039ED4275CED7440929022BC67E70611531A3BE2@SHSMSX103.ccr.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: add packet field set aciton in flow API 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: , X-List-Received-Date: Fri, 20 Apr 2018 08:55:11 -0000 On Fri, Apr 20, 2018 at 02:24:20AM +0000, Zhang, Qi Z wrote: > Hi Adrien: > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Thursday, April 19, 2018 10:49 PM > > To: Zhang, Qi Z > > Cc: dev@dpdk.org; Doherty, Declan ; Chandran, > > Sugesh ; Glynn, Michael J > > ; Liu, Yu Y ; Ananyev, > > Konstantin ; Richardson, Bruce > > > > Subject: Re: [PATCH v3 2/4] ethdev: add packet field set aciton in flow API > > > > Typo in commit title: aciton => action > > > > On Mon, Apr 16, 2018 at 02:10:40PM +0800, Qi Zhang wrote: > > > Add new action RTE_FLOW_ACTION_TYPE_FIELD_SET, it is used to modify > > > fields of specific protocol layer of the packet, the action only apply > > > on packets that contain the requireds protocol layer. > > > > requireds => required > > > > > > > > Signed-off-by: Qi Zhang > > > > (more below) > > > > > --- > > > doc/guides/prog_guide/rte_flow.rst | 30 > > +++++++++++++++++++++++++++ > > > lib/librte_ether/rte_flow.h | 42 > > +++++++++++++++++++++++++++++++++++++- > > > 2 files changed, 71 insertions(+), 1 deletion(-) > > > > > > diff --git a/doc/guides/prog_guide/rte_flow.rst > > > b/doc/guides/prog_guide/rte_flow.rst > > > index 99468bf60..68deb9812 100644 > > > --- a/doc/guides/prog_guide/rte_flow.rst > > > +++ b/doc/guides/prog_guide/rte_flow.rst > > > @@ -1574,6 +1574,36 @@ fields in the pattern items. > > > | 1 | END | > > > +-------+----------+ > > > > > > +Action: ``FILED_SET`` > > > +^^^^^^^^^^^^^^^^^^^^^ > > > > FILED_SET => FIELD_SET > > > > > + > > > +Modify the value of fields in a protocol layer, only applies to > > > +packets that contain respective protocol layer. > > > + > > > +.. _table_rte_flow_action_field_set: > > > + > > > +.. table:: FIELD_SET > > > + > > > + +---------------+-------------------------------------------------------------------+ > > > + | Field | Value > > | > > > + > > +===============+============================================== > > =====================+ > > > + | ``type`` | Specify the type of a protocol layer. (see > > RTE_FLOW_ITEM_TYPE_*) | > > > + +---------------+-------------------------------------------------------------------+ > > > + | ``dir_level`` | Specify the level of matched protocol layer. > > | > > > + | | direction (1b) > > | > > > + | | 0: match start from outermost. > > | > > > + | | 1: match start from innermost. > > | > > > > Please remove the direction part. What devices can match is always > > outermost up to the point where they can't recognize an inner header. > > "innermost" is almost guaranteed to never have the desired effect. > > > > > + | | level: (31b) > > | > > > + | | 0: outermost or innermost protocol layer that > > matched @type | > > > + | | 1: next to outmost or innermost protocol layer > > that matched @type | > > > + | | 2: and so on ... > > | > > > > Then you can remove any reference to dir_level from here. > > > > > + +---------------+-------------------------------------------------------------------+ > > > + | ``new_val`` | Pointer to specific data structure according to > > protocol type, | > > > + | | the content is the new value to updtae. > > | > > > > updtae => update > > > > > + +---------------+-------------------------------------------------------------------+ > > > + | ``mask`` | Bit-mask applied to new_val > > | > > > + > > > + +---------------+--------------------------------------------------- > > > + ----------------+ > > > + > > > Negative types > > > ~~~~~~~~~~~~~~ > > > > > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > > > index f84bbfda5..2dc95b6b8 100644 > > > --- a/lib/librte_ether/rte_flow.h > > > +++ b/lib/librte_ether/rte_flow.h > > > @@ -1245,7 +1245,15 @@ enum rte_flow_action_type { > > > * > > > * See struct rte_flow_action_security. > > > */ > > > - RTE_FLOW_ACTION_TYPE_SECURITY > > > + RTE_FLOW_ACTION_TYPE_SECURITY, > > > + > > > + /** > > > + * Modify the value of fields in a protocol layer, only applies to > > > + * packets that contain respective protocol layer. > > > + * > > > + * See struct rte_flow_action_field_set. > > > + */ > > > + RTE_FLOW_ACTION_TYPE_FIELD_SET, > > > }; > > > > > > /** > > > @@ -1384,6 +1392,38 @@ struct rte_flow_action_security { }; > > > > > > /** > > > + * RTE_FLOW_ACTION_TYPE_FIELD_SET > > > + * > > > + * Modify the value of fields in a protocol layer, only applies to > > > + * packets that contain respective protocol layer. > > > + */ > > > +struct rte_flow_action_field_set { > > > + /** > > > + * Specify the type of a protocol layer. > > > + */ > > > + enum rte_flow_item_type type; > > > + /** > > > + * Specify the level of matched protocol layer. > > > + * > > > + * direction (1b) > > > + * 0: match start from outermost. > > > + * 1: match start from innermost. > > > + * > > > + * level (31b) > > > + * 0: outermost|innermost protocol layer that matched @type. > > > + * 1: next to outermost|innermost protocol layer that matched @type. > > > + * 2: and so on ... > > > + */ > > > + uint32_t dir_level; > > > > See above regarding this field. > > > > > + /** > > > + * Pointer to specific data structure according to protocol type, > > > + * the content is the new value to update. > > > + */ > > > + const void *new_val; > > > + const void *mask; /**< Bit-mask applied to new_val. */ }; > > > + > > > +/** > > > * Definition of a single action. > > > * > > > * A list of actions is terminated by a END action. > > > -- > > > 2.13.6 > > > > > > > Testpmd implementation and documentation update are also missing, > > however > > I'm still not convinced by the definition of this new action, it seems too > > generic to be useful (e.g. compare this with a dedicated "update destination > > IPv4 address" action for instance). > > > > What existing HW capabilities do you intend to expose through this, what > > kind of fields can be updated at this point? > > For our device, there will be more than 20 actions if we create an action for each field like "RTE_FLOW_ACTION_TYPE_IPV4_ADDR_SET", > More detail, that will cover fields in IPV4/IPV6/Ether/ICMP/ND/ARP, so I think a generic field set action would be better. I see. You know, I think adding 20+ focused actions would be fine actually, easier to document, report as HW capabilities and to use by applications (e.g. see issue you raise about adding testpmd support below). In all the protocols you mention, is the device able to really update *all* fields or only the "usual" set? IPv4 src/dst probably, now what about ToS, packet ID, fragment offset and so on? Same for other protocols, should we care about protocol fields that applications rarely set (if at all), especially if no device can update them? > For testpmd support, Seems there is no reference to enable an action with void parameters, so it may take me time to figure out a solution, Dedicated actions on the other hand should be way easier to add :) > I'm not sure I can capture this on the 18.05, so is that possible we just deferred testpmd support for this action? since I saw action like rte_flow_action_security also don't have > testpmd support yet. I know, there are several problematic actions without testpmd support. However those are often associated with experimental APIs and may be modified without prior notice. Keep in mind there is no need to rush. Adding rte_flow actions doesn't hurt ABI and can be done for any DPDK release, even part of the same series as the first PMD implementation which must be validated somehow (no one ever submits code that can't be tested, right?), this is why testpmd support is mandatory. > > If it's still unclear, I suggest to remove this patch from the series or at > > the very least mark it as experimental. You can even provide a forward > > declaration without the contents of struct rte_flow_action_field_set to > > prevent applications from using it before it's finalized. -- Adrien Mazarguil 6WIND