From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id 656041BC36 for ; Thu, 12 Apr 2018 12:23:10 +0200 (CEST) Received: by mail-wr0-f193.google.com with SMTP id y7so4557482wrh.10 for ; Thu, 12 Apr 2018 03:23:10 -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=+4yK04yJaEI8lMhpPJp/wuE8cX/5jpGknJqOI60wDOc=; b=fvhgVZTK1KhlxQX3NAw/dQqGDUy5qqyTcbBt5PUSag51fRIgDxHhtI413VM7rVLikP od6/QX5SpfLxvtd87gj094OwTG8ZQdbmTMlJ8uGNUZ/fnV4SNMediTvj+Hq2k/xPydBJ GPO/DjrY9TCj3tmptgLg1xgNZeeKDs4SCps0EnGC4z0qQuGLb1OfeidYPRscO8VkMYWp tB46B7XXSKEGhMvEXYwimFYYCaVBDnMOFCQpoc2aiiJb17khq1Q3ehQq7LWYt3LIV15d DTpyS20HakPEYt2joVIzvA/Ta7bxPBFQaKbEFgCRsa2c+hVFsXhlPe9fN6czKygEFMeJ 1NHw== 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=+4yK04yJaEI8lMhpPJp/wuE8cX/5jpGknJqOI60wDOc=; b=gCQbHgBmE8rgdn4ukUzfQ2zq7tJThSD/M//q53DhbxTGQ6rgELCGULK6Xa/iRn8QxZ 1TYFPdKPxnsQgi/7KHt1qafmzNNVbjDTKqepXruxfNs4rrDkd4YGNAXvMK4A7lVUeVGw xAgtCISIWdSNcGiDO+LM8ladgn4qyhH40B6NHY5wY9IVeuayHRhxvqBpXqgydS/USmYx k7Cx/qaeQrLhQzIVIZzlhVyAH7M5OCMq/GOl2ZZVz2EAx+W/wc4yghQ+eLuGbVfsL9SX Q1EiceSKh3IZqlAzl2hzIDPbkC07Mo3ZckjYP2zA89Cm5mvtmRbdIVJdAIYkE3eKl9ML KCqA== X-Gm-Message-State: ALQs6tCZYJWArBTPVgvWVwjzZ2ZEbCjDFza9b84ZyCHnPQ+/eOEA1sWx hYGvDaZDUPlcJNbrc08XX44FPg== X-Google-Smtp-Source: AIpwx48aHuOqbs6meLUUeboMno8JUH0VuBzOmp/aJkffk8LbMhp1x4s3M5HBeP7lmd36FtT6oVFp8Q== X-Received: by 10.223.188.12 with SMTP id s12mr326734wrg.266.1523528589971; Thu, 12 Apr 2018 03:23:09 -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 m4sm2215869wrj.47.2018.04.12.03.23.08 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 12 Apr 2018 03:23:09 -0700 (PDT) Date: Thu, 12 Apr 2018 12:22:55 +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: <20180412102255.GQ4957@6wind.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-5-git-send-email-qi.z.zhang@intel.com> <20180412070343.GO4957@6wind.com> <039ED4275CED7440929022BC67E70611531903F5@SHSMSX103.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <039ED4275CED7440929022BC67E70611531903F5@SHSMSX103.ccr.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH v2 4/4] ether: add packet modification 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: Thu, 12 Apr 2018 10:23:10 -0000 On Thu, Apr 12, 2018 at 08:50:14AM +0000, Zhang, Qi Z wrote: > Hi Adrien > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Thursday, April 12, 2018 3:04 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 v2 4/4] ether: add packet modification aciton in flow API > > > > On Sun, Apr 01, 2018 at 05:19:22PM -0400, Qi Zhang wrote: > > > Add new actions that be used to modify packet content with generic > > > semantic: > > > > > > RTE_FLOW_ACTION_TYPE_FIELD_UPDATE: > > > - update specific field of packet > > > RTE_FLWO_ACTION_TYPE_FIELD_INCREMENT: > > > - increament specific field of packet > > > RTE_FLWO_ACTION_TYPE_FIELD_DECREMENT: > > > - decreament specific field of packet > > > RTE_FLWO_ACTION_TYPE_FIELD_COPY: > > > - copy data from one field to another in packet. > > > > > > All action use struct rte_flow_item parameter to match the pattern > > > that going to be modified, if no pattern match, the action just be > > > skipped. > > > > That's not good. It must result in undefined behavior, more about that below. > > I may not get your point, see my below comment. > > > > > > These action are non-terminating action. they will not impact the fate > > > of the packets. > > > > > > Signed-off-by: Qi Zhang > > > > Noticed a few typos above and in subject line ("aciton", "FLWO", "increament", > > "decreament"). > > > > Note that I'm usually against using rte_flow_item structures and associated > > enum values inside action lists because it could be seen as inconsistent from > > an API standpoint. On the other hand, reusing existing types is a good thing so > > let's go with that for now. > > > > Please see inline comments. > > > > > --- > > > doc/guides/prog_guide/rte_flow.rst | 89 > > ++++++++++++++++++++++++++++++++++++++ > > > lib/librte_ether/rte_flow.h | 57 ++++++++++++++++++++++++ > > > 2 files changed, 146 insertions(+) > > > > > > diff --git a/doc/guides/prog_guide/rte_flow.rst > > > b/doc/guides/prog_guide/rte_flow.rst > > > index aa5c818..6628964 100644 > > > --- a/doc/guides/prog_guide/rte_flow.rst > > > +++ b/doc/guides/prog_guide/rte_flow.rst > > > @@ -1508,6 +1508,95 @@ Representor. > > > | ``port_id`` | identification of the destination | > > > +--------------+-----------------------------------+ > > > > > > +Action: ``FILED_UPDATE`` > > > +^^^^^^^^^^^^^^^^^^^^^^^ > > > > FILED => FIELD > > > > Underline is also shorter than title and might cause documentation warnings. > > > > > + > > > +Update specific field of the packet. > > > + > > > +- Non-terminating by default. > > > > These statements are not needed since "ethdev: alter behavior of flow API > > actions" [1]. > > > > [1] http://dpdk.org/ml/archives/dev/2018-April/096527.html > > > > > + > > > +.. _table_rte_flow_action_field_update: > > > + > > > +.. table:: FIELD_UPDATE > > > + > > > + +-----------+---------------------------------------------------------+ > > > + | Field | Value > > | > > > + > > +===========+==================================================== > > =====+ > > > + | ``item`` | item->type: specify the pattern to modify > > | > > > + | | item->spec: specify the new value to update > > | > > > + | | item->mask: specify which part of the pattern to update > > | > > > + | | item->last: ignored > > | > > > > This table needs to be divided a bit more with one cell per field for better > > clarity. See other pattern item definitions such as "Item: ``RAW``" for an > > example. > > > > > + +-----------+---------------------------------------------------------+ > > > + | ``layer`` | 0 means outermost matched pattern, > > | > > > + | | 1 means next-to-outermost and so on ... > > | > > > + > > > + +-----------+------------------------------------------------------- > > > + --+ > > > > What does "layer" refer to by the way? The layer described on the pattern side > > of the flow rule, the actual protocol layer matched inside traffic, or is "item" > > actually an END-terminated list of items (as suggested by "pattern" in above > > documentation)? > > > > I suspect the intent is for layer to have the same definition as RSS encapulation > > level ("ethdev: add encap level to RSS flow API action" [2]), and item points to > > a single item, correct? > > Yes > > > > In that case, it's misleading, please rename it "level". Also keep in mind you > > can't make an action rely on anything found on the pattern side of a flow rule. > > > OK, "Level" looks better. > Also I may not get your point here. please correct me, > My understanding is, all the modification action of a flow is independent of patterns of the same flow, > For example when define a flow with pattern = eth/ipv4 and with a TCP modification action. > all ipv4 packets will hit that flow, and go to the same destination, but only TCP packet will be modified > otherwise, the action is just skipped, > > > What happens when this action is attempted on non-matching traffic must be > > documented here as well. Refer to discussion re "ethdev: Add tunnel > > encap/decap actions" [3]. To be on the safe side, it must be documented as > > resulting in undefined behavior. > > so what is "undefined behavior" you means? > The rule is: > If a packet matched pattern in action, it will be modified, otherwise the action just take no effect > is this idea acceptable? Not really, what happens will depend on the underlying device. It's better to document it as undefined because you can't predict the result. Some devices will cause packets to be lost, others will let them through unchanged, others will crash the system after formatting the hard drive, no one knows. It's an application's responsibility to properly match traffic or otherwise make sure only matching traffic goes through before performing any actions on it, otherwise they'll encounter such undefined behavior. > > Based the same thread, I also suggest here to define "last" as reserved and > > therefore an error if set to anything other than NULL, however it might prove > > useful, see below. > > > > [2] http://dpdk.org/ml/archives/dev/2018-April/096531.html > > [3] http://dpdk.org/ml/archives/dev/2018-April/096418.html > > > > > + > > > +Action: ``FILED_INCREMENT`` > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > FILED => FIELD > > > > > + > > > +Increment 1 on specific field of the packet. > > > > All right, but what for? FIELD_UPDATE overwrites a specific value at some > > specific place after matching something rather specific. > > > > In my opinion to get predictable results with FIELD_INCREMENT, applications > > also need to have a pretty good idea of what's about to be incremented. > > That's because you can't put conditionals in flow rules (yet). So if you need to > > match an exact IPv4 address in order to increment it, why wouldn't you just > > provide its replacement directly? > > The typical usage is for TTL or similar count that are not be matched in flow pattern. Incrementing TTL? Let's assume it's automatically decremented. What happens when its value is already 0 in the packet? > > I'm not saying there are no use cases for this, but you know, a couple of real > > world example scenarios can't hurt. This should answer what an application > > could possibly want to unconditionally increment in ingress/egress traffic and > > for what purpose. > > > > > > + > > > +- Non-terminating by default. > > > > Same comment as in FIELD_UPDATE. > > > > > + > > > +.. _table_rte_flow_action_field_update: > > > + > > > +.. table:: FIELD_INCREMENT > > > + > > > + +-----------+---------------------------------------------------------+ > > > + | Field | Value > > | > > > + > > +===========+==================================================== > > =====+ > > > + | ``item`` | item->type: specify the pattern to modify > > | > > > + | | item->spec: ignored > > | > > > + | | item->mask: specify which part of the pattern to update > > | > > > + | | item->last: ignored > > | > > > + +-----------+---------------------------------------------------------+ > > > + | ``layer`` | 0 means outermost matched pattern, > > | > > > + | | 1 means next-to-outermost and so on ... > > | > > > + > > > + +-----------+------------------------------------------------------- > > > + --+ > > > > Ditto. > > > > With another comment regarding "last". When specified it could be used to > > provide a stride, i.e. the amount to increment instead of 1 (increment = last - > > spec). > > But the initial value is not predicable like TTL. Not sure I understand this comment. The action part of a flow rule blindly affects traffic as described in the action, it doesn't even look at the original value. The pattern part is supposed to select traffic on which actions must be performed. Decrementing TTL is a possibility but I don't see TTL as something predictable on the pattern side. Are we both talking about Time-to-Live? > > > > > > + > > > +Action: ``FIELD_DECREMENT`` > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > + > > > +Decrement 1 on specific field of the packet. > > > > Same comment as in FIELD_INCREMENT. > > > > > + > > > +Paramenter is same is FIELD_INCREMENT. > > > > Paramenter => Parameter > > > > > + > > > +-Non-terminating by default. > > > > Same comment as in FIELD_UPDATE. > > > > A table is missing in this section and must be included. Keep in mind section > > titles are used as anchors in the documentation and readers may reach this > > point without going through the previous sections. > > > > OK, will add. > > > > + > > > +ACTION: ``FIELD_COPY`` > > > +^^^^^^^^^^^^^^^^^^^^^^ > > > + > > > +Copy data of one field to another of the packet. > > > + > > > +-Non-terminating by default. > > > > Same comment as in FIELD_UPDATE. > > > > > + > > > +.. _table_rte_flow_action_field_copy: > > > + > > > +.. table:: FIELD_COPY > > > + > > > + +-----------------+-----------------------------------------------------------------+ > > > + | Field | Value > > | > > > + > > +=================+============================================== > > ===================+ > > > + | ``src_item`` | src_item->type: match the pattern that data will be > > copy from | > > > + | | src_item->spec: ignored > > | > > > + | | src_item->mask: specify which part of the > > pattern to copy | > > > + | | src_item->last: ignored > > | > > > + +-----------------+-----------------------------------------------------------------+ > > > + | ``src_layer`` | layer of src_item > > | > > > + | | 0 means outermost matched pattern, > > | > > > + | | 1 means next-to-outermost and so on ... > > | > > > + +-----------------+-----------------------------------------------------------------+ > > > + | ``dst_item`` | dst_item->type: match the pattern that data will be > > copy to | > > > + | | dst_item->spec: ignored > > | > > > + | | dst_item->mask: specify which part of the > > pattern to be update | > > > + | | it must match > > src_item->mask. | > > > + | | dst_item->last: ignored > > | > > > + +-----------------+-----------------------------------------------------------------+ > > > + | ``dst_layer`` | layer of dst_item > > | > > > + | | 0 means outermost matched pattern, > > | > > > + | | 1 means next-to-outermost and so on ... > > | > > > + > > > + +-----------------+------------------------------------------------- > > > + ----------------+ > > > > Same comments as in FIELD_UPDATE regarding table format, "last", etc. > > > > A couple of real world use cases would be a nice addition here as well. > > > > For TTL copy-in/copy-out :) > Sorry, I should add typical usage in document also Then please elaborate. > > > > + > > > Negative types > > > ~~~~~~~~~~~~~~ > > > > > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > > > index a8ec780..d81ac4c 100644 > > > --- a/lib/librte_ether/rte_flow.h > > > +++ b/lib/librte_ether/rte_flow.h > > > @@ -1178,6 +1178,34 @@ enum rte_flow_action_type { > > > * See struct rte_flow_action_port. > > > */ > > > RTE_FLOW_ACTION_TYPE_PORT, > > > + > > > + /** > > > + * Update specific field of the packet. > > > > Update => Updates (to keep the style) > > > > > + * > > > + * See struct rte_flow_item_field_update. > > > + */ > > > + RTE_FLOW_ACTION_TYPE_FILED_UPDATE, > > > > FILED => FIELD > > > > > + > > > + /** > > > + * Increment specific field of the packet. > > > > Increment => Increments > > > > > + * > > > + * See struct rte_flow_item_field_update. > > > + */ > > > + RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT, > > > + > > > + /** > > > + * Decrement specific field of the packet. > > > > Decrement => Decrements > > > > > + * > > > + * See struct rte_flow_item_field_update. > > > + */ > > > + RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT, > > > + > > > + /** > > > + * Copy data of one field to another of the packet. > > > > Copy => Copies > > > > > + * > > > + * See struct rte_flow_item_type_field_copy. > > > + */ > > > + RTE_FLOW_ACTION_TYPE_FIELD_COPY, > > > }; > > > > > > /** > > > @@ -1327,6 +1355,35 @@ struct rte_flow_action_port { > > > uint16_t port_id; /**< identification of the forward destination. */ > > > }; > > > > > > +/** RTE_FLOW_ACTION_TYPE_FIELD_UPDATE > > > > Here "/**" should be on its own line like the rest of the file. You can safely > > ignore checkpatch nonsense (if any). > > > > > + * RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT > > > + * RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT > > > > One shared structure for everything? How about a single UPDATE action to > > cover everything instead? > > > > - mask && last is NULL or last - spec is 0 => update > > - mask && last - spec is positive => increment by that amount > > - mask && last - spec is negative => decrement by that amount > > So, only care about delta?, last=4 spec=3 is the same thing as last=34 spec=33? > It looks a little bit confuse to me. Anything's fine as long as it's properly documented and convincing enough :) This approach allows decrementing by a large amount using unsigned values or whatever types spec/last fields use according to a mask. One can decrement or increment something as small as a single bit and as large as an IPv6 address: spec = 0x8000, last = 0 => decrement by 0x8000 spec = 0, last = 0x8000 => increment by 0x8000 spec = 0x8000, last = 0x8000 => set to 0x8000 All the above *only* for masked fields and when last is non-NULL. Think of the possibilities! > > > > > This would be easier to document, would result in a smaller patch, implicitly > > gives meaning to "last" and provides the ability to simultaneously increment, > > decrement and update several fields at once. > > > > > + * > > > + * Update specific field of the packet. > > > + * > > > + * Typical usage: update mac/ip address. > > > > Documentation is a bit weak and needs improvement. In any case, except for > > tables and examples, whatever is written in this comment should be word for > > word the same as what is written in rte_flow.rst. > > > > > + */ > > > +struct rte_flow_action_field_update { > > > + const struct rte_flow_item *item; /**< specify the data to modify. > > > +*/ > > > > Since there is a single item that contains its own pointers to spec/last/mask, > > "item" shouldn't be a pointer. It's unnecessary and misleading. > > OK, will change to structure. > > > > Documentation needs improvement. > > > > > + uint8_t layer; > > > + /**< 0 means outermost matched pattern, 1 means next-to-outermost... > > > +*/ > > > > uint8_t layer => uint32_t level (we're not constrained by space) > > > > Ditto re documentation. See RSS encap level patch [2] for ideas. > > Thanks for heads up, will check that. > > > > > > +}; > > > + > > > +/** RTE_FLOW_ACTION_TYPE_FIELD_COPY > > > > Ditto for "/**". > > > > > + * > > > + * Copy data from one field to another of the packet. > > > + * > > > + * Typical usage: TTL copy-in / copy-out > > > > This typical usage doesn't look that obvious to me. Copy TTL from what part of > > the packet to where? What happens to the overwritten TTL value? Why should > > they be synchronized? > > I think this is standard flow action from OpenFlow, and it is supported by our hardware > This is the generic way we implemented, let me know if you have other better idea. OK, after looking it up [4], I see OpenFlow defines specific "Change-TTL" actions that only work with specific protocols (IPv4 TTL, IPv6 hops, MPLS TTL). The above makes so much sense now. This document describes that packets with invalid TTLs are rejected, particularly on the decrement side. If this is what HW supports, I think it would be wiser to define specific TTL-manipulating actions as well. As defined, increment/decrement actions are rather dumb and cannot care about what's being incremented/decremented. Since they can be used with any header field, they don't have special provisions when the original or resulting value is 0. I think a dedicated set of OpenFlow actions is needed if OF is considered a HW capability. They would be easy to document by pointing to their original specification. Based on [1] this should result in: RTE_FLOW_ACTION_TYPE_OF_COPY_TTL_INWARDS RTE_FLOW_ACTION_TYPE_OF_POP RTE_FLOW_ACTION_TYPE_OF_PUSH_MPLS RTE_FLOW_ACTION_TYPE_OF_PUSH_PBB RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN RTE_FLOW_ACTION_TYPE_OF_COPY_TTL_OUTWARDS RTE_FLOW_ACTION_TYPE_OF_DECREMENT_TTL ... Many of them wouldn't even need a configuration structure. [4] https://www.opennetworking.org/images/stories/downloads/sdn-resources/onf-specifications/openflow/openflow-spec-v1.3.0.pdf > > Thanks > Qi > > > > > > + */ > > > +struct rte_flow_action_field_copy { > > > + const struct rte_flow_item *src_item; /**< Specify the data copy from */ > > > + uint8_t src_layer; > > > + /**< 0 means outermost matched pattern, 1 means next-to-outermost... > > */ > > > + const struct rte_flow_item *dst_item; /**< Specify the data copy to */ > > > + uint8_t dst_layer; > > > + /**< 0 means outermost matched pattern, 1 means next-to-outermost... > > > +*/ }; > > > > Same comments as for struct rte_flow_action_field_update. > > > > > + > > > /** > > > * Definition of a single action. > > > * > > > -- > > > 2.7.4 > > > > > > > -- > > Adrien Mazarguil > > 6WIND -- Adrien Mazarguil 6WIND