From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Zhang, Qi Z" Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in flow API Date: Fri, 13 Apr 2018 13:47:15 +0000 Message-ID: <039ED4275CED7440929022BC67E7061153191023@SHSMSX103.ccr.corp.intel.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> <20180412102255.GQ4957@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Doherty, Declan" , "Chandran, Sugesh" , "Glynn, Michael J" , "Liu, Yu Y" , "Ananyev, Konstantin" , "Richardson, Bruce" To: Adrien Mazarguil Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 988B01C4A9 for ; Fri, 13 Apr 2018 15:47:20 +0200 (CEST) In-Reply-To: <20180412102255.GQ4957@6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Thursday, April 12, 2018 6:23 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 >=20 > 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 tha= t > 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 =3D> 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 > > > | > > > > + > > > > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D > > > =3D=3D=3D=3D=3D+ > > > > + | ``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 insi= de > 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 patter= n > 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 patte= rn > =3D 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? >=20 > Not really, what happens will depend on the underlying device. It's bette= r to > document it as undefined because you can't predict the result. Some devic= es > will cause packets to be lost, others will let them through unchanged, ot= hers > will crash the system after formatting the hard drive, no one knows. OK, basically I think "undefined behavior" is not friendly to application, = we should avoid. But you are right, we need to consider device with different behavior for "= modification on non-matched pattern" I'm thinking why driver can't avoid non-matched pattern modification if the= device does not support? For example, driver can reject a flow ETH/IPV4 with TCP action, but may acc= ept ETH/IPV4/TCP with TCP action base on=20 its capability. >=20 > It's an application's responsibility to properly match traffic or otherwi= se make > sure only matching traffic goes through before performing any actions on = it, > otherwise they'll encounter such undefined behavior. >=20 > > > 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 =3D> 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 i= n flow > pattern. >=20 > Incrementing TTL? Let's assume it's automatically decremented. What > happens when its value is already 0 in the packet? >=20 > > > 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 > > > | > > > > + > > > > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D > > > =3D=3D=3D=3D=3D+ > > > > + | ``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 =3D last - spec). > > > > But the initial value is not predicable like TTL. >=20 > Not sure I understand this comment. The action part of a flow rule blindl= y > affects traffic as described in the action, it doesn't even look at the o= riginal > value. The pattern part is supposed to select traffic on which actions mu= st be > performed. >=20 > Decrementing TTL is a possibility but I don't see TTL as something predic= table > on the pattern side. Are we both talking about Time-to-Live? >=20 > > > > > > > > > + > > > > +Action: ``FIELD_DECREMENT`` > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > + > > > > +Decrement 1 on specific field of the packet. > > > > > > Same comment as in FIELD_INCREMENT. > > > > > > > + > > > > +Paramenter is same is FIELD_INCREMENT. > > > > > > Paramenter =3D> 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 secti= ons. > > > > > > > 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 > > > | > > > > + > > > > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+ > > > > + | ``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 wel= l. > > > > > > > For TTL copy-in/copy-out :) > > Sorry, I should add typical usage in document also >=20 > Then please elaborate. >=20 > > > > > > + > > > > 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 =3D> Updates (to keep the style) > > > > > > > + * > > > > + * See struct rte_flow_item_field_update. > > > > + */ > > > > + RTE_FLOW_ACTION_TYPE_FILED_UPDATE, > > > > > > FILED =3D> FIELD > > > > > > > + > > > > + /** > > > > + * Increment specific field of the packet. > > > > > > Increment =3D> Increments > > > > > > > + * > > > > + * See struct rte_flow_item_field_update. > > > > + */ > > > > + RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT, > > > > + > > > > + /** > > > > + * Decrement specific field of the packet. > > > > > > Decrement =3D> 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 =3D> 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 =3D> update > > > - mask && last - spec is positive =3D> increment by that amount > > > - mask && last - spec is negative =3D> decrement by that amount > > > > So, only care about delta?, last=3D4 spec=3D3 is the same thing as last= =3D34 > spec=3D33? > > It looks a little bit confuse to me. >=20 > Anything's fine as long as it's properly documented and convincing enough= :) >=20 > 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: >=20 > spec =3D 0x8000, last =3D 0 =3D> decrement by 0x8000 >=20 > spec =3D 0, last =3D 0x8000 =3D> increment by 0x8000 >=20 > spec =3D 0x8000, last =3D 0x8000 =3D> set to 0x8000 >=20 > All the above *only* for masked fields and when last is non-NULL. Think o= f > the possibilities! >=20 > > > > > > > > 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 =3D> 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 m= e > know if you have other better idea. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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 origin= al > specification. Based on [1] this should result in: >=20 > 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 > ... >=20 Yes, actually this is another option during design :) Since you suggested, I'd like to change to this style. Thanks Qi > Many of them wouldn't even need a configuration structure. >=20 > [4] > https://www.opennetworking.org/images/stories/downloads/sdn-resources/ > onf-specifications/openflow/openflow-spec-v1.3.0.pdf >=20 > > > > 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 >=20 > -- > Adrien Mazarguil > 6WIND