From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id E660F1BC5C for ; Thu, 12 Apr 2018 10:50:50 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Apr 2018 01:50:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,440,1517904000"; d="scan'208";a="32803232" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga007.jf.intel.com with ESMTP; 12 Apr 2018 01:50:48 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 12 Apr 2018 01:50:43 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.151]) by shsmsx102.ccr.corp.intel.com ([169.254.2.184]) with mapi id 14.03.0319.002; Thu, 12 Apr 2018 16:50:15 +0800 From: "Zhang, Qi Z" To: Adrien Mazarguil CC: "dev@dpdk.org" , "Doherty, Declan" , "Chandran, Sugesh" , "Glynn, Michael J" , "Liu, Yu Y" , "Ananyev, Konstantin" , "Richardson, Bruce" Thread-Topic: [PATCH v2 4/4] ether: add packet modification aciton in flow API Thread-Index: AQHTyjsqo1vGbY6TbEaiN6r6q7JhJqP8PjaAgACOqsA= Date: Thu, 12 Apr 2018 08:50:14 +0000 Message-ID: <039ED4275CED7440929022BC67E70611531903F5@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> In-Reply-To: <20180412070343.GO4957@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 08:50:51 -0000 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 >=20 > 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. >=20 > That's not good. It must result in undefined behavior, more about that be= low. I may not get your point, see my below comment. >=20 > > These action are non-terminating action. they will not impact the fate > > of the packets. > > > > Signed-off-by: Qi Zhang >=20 > Noticed a few typos above and in subject line ("aciton", "FLWO", "incream= ent", > "decreament"). >=20 > Note that I'm usually against using rte_flow_item structures and associat= ed > 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 th= ing so > let's go with that for now. >=20 > Please see inline comments. >=20 > > --- > > 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`` > > +^^^^^^^^^^^^^^^^^^^^^^^ >=20 > FILED =3D> FIELD >=20 > Underline is also shorter than title and might cause documentation warnin= gs. >=20 > > + > > +Update specific field of the packet. > > + > > +- Non-terminating by default. >=20 > These statements are not needed since "ethdev: alter behavior of flow API > actions" [1]. >=20 > [1] http://dpdk.org/ml/archives/dev/2018-April/096527.html >=20 > > + > > +.. _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 upda= te > | > > + | | item->last: ignored > | >=20 > This table needs to be divided a bit more with one cell per field for bet= ter > clarity. See other pattern item definitions such as "Item: ``RAW``" for a= n > example. >=20 > > + +-----------+------------------------------------------------------= ---+ > > + | ``layer`` | 0 means outermost matched pattern, > | > > + | | 1 means next-to-outermost and so on ... > | > > + > > + +-----------+------------------------------------------------------- > > + --+ >=20 > 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 ab= ove > documentation)? >=20 > I suspect the intent is for layer to have the same definition as RSS enca= pulation > level ("ethdev: add encap level to RSS flow API action" [2]), and item po= ints to > a single item, correct? Yes >=20 > In that case, it's misleading, please rename it "level". Also keep in min= d you > can't make an action rely on anything found on the pattern side of a flow= rule. >=20 OK, "Level" looks better. Also I may not get your point here. please correct me,=20 My understanding is, all the modification action of a flow is independent o= f patterns of the same flow,=20 For example when define a flow with pattern =3D eth/ipv4 and with a TCP mod= ification action. all ipv4 packets will hit that flow, and go to the same destination, but on= ly TCP packet will be modified otherwise, the action is just skipped, > What happens when this action is attempted on non-matching traffic must b= e > 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 a= s > 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 a= ction just take no effect is this idea acceptable? >=20 > Based the same thread, I also suggest here to define "last" as reserved a= nd > therefore an error if set to anything other than NULL, however it might p= rove > useful, see below. >=20 > [2] http://dpdk.org/ml/archives/dev/2018-April/096531.html > [3] http://dpdk.org/ml/archives/dev/2018-April/096418.html >=20 > > + > > +Action: ``FILED_INCREMENT`` > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ >=20 > FILED =3D> FIELD >=20 > > + > > +Increment 1 on specific field of the packet. >=20 > All right, but what for? FIELD_UPDATE overwrites a specific value at some > specific place after matching something rather specific. >=20 > In my opinion to get predictable results with FIELD_INCREMENT, applicatio= ns > 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 ju= st > provide its replacement directly? The typical usage is for TTL or similar count that are not be matched in fl= ow pattern.=20 >=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 applicatio= n > could possibly want to unconditionally increment in ingress/egress traffi= c and > for what purpose. >=20 > > + > > +- Non-terminating by default. >=20 > Same comment as in FIELD_UPDATE. >=20 > > + > > +.. _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 upda= te > | > > + | | item->last: ignored > | > > + +-----------+------------------------------------------------------= ---+ > > + | ``layer`` | 0 means outermost matched pattern, > | > > + | | 1 means next-to-outermost and so on ... > | > > + > > + +-----------+------------------------------------------------------- > > + --+ >=20 > Ditto. >=20 > 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 > > + > > +Action: ``FIELD_DECREMENT`` > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Decrement 1 on specific field of the packet. >=20 > Same comment as in FIELD_INCREMENT. >=20 > > + > > +Paramenter is same is FIELD_INCREMENT. >=20 > Paramenter =3D> Parameter >=20 > > + > > +-Non-terminating by default. >=20 > Same comment as in FIELD_UPDATE. >=20 > A table is missing in this section and must be included. Keep in mind sec= tion > titles are used as anchors in the documentation and readers may reach thi= s > point without going through the previous sections. >=20 OK, will add. > > + > > +ACTION: ``FIELD_COPY`` > > +^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Copy data of one field to another of the packet. > > + > > +-Non-terminating by default. >=20 > Same comment as in FIELD_UPDATE. >=20 > > + > > +.. _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 wil= l 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 wil= l 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 ... > | > > + > > + +-----------------+------------------------------------------------- > > + ----------------+ >=20 > Same comments as in FIELD_UPDATE regarding table format, "last", etc. >=20 > A couple of real world use cases would be a nice addition here as well. >=20 For TTL copy-in/copy-out :) Sorry, I should add typical usage in document also > > + > > 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. >=20 > Update =3D> Updates (to keep the style) >=20 > > + * > > + * See struct rte_flow_item_field_update. > > + */ > > + RTE_FLOW_ACTION_TYPE_FILED_UPDATE, >=20 > FILED =3D> FIELD >=20 > > + > > + /** > > + * Increment specific field of the packet. >=20 > Increment =3D> Increments >=20 > > + * > > + * See struct rte_flow_item_field_update. > > + */ > > + RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT, > > + > > + /** > > + * Decrement specific field of the packet. >=20 > Decrement =3D> Decrements >=20 > > + * > > + * See struct rte_flow_item_field_update. > > + */ > > + RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT, > > + > > + /** > > + * Copy data of one field to another of the packet. >=20 > Copy =3D> Copies >=20 > > + * > > + * 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 >=20 > Here "/**" should be on its own line like the rest of the file. You can s= afely > ignore checkpatch nonsense (if any). >=20 > > + * RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT > > + * RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT >=20 > One shared structure for everything? How about a single UPDATE action to > cover everything instead? >=20 > - 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=3D3= 4 spec=3D33? It looks a little bit confuse to me.=20 >=20 > This would be easier to document, would result in a smaller patch, implic= itly > gives meaning to "last" and provides the ability to simultaneously increm= ent, > decrement and update several fields at once. >=20 > > + * > > + * Update specific field of the packet. > > + * > > + * Typical usage: update mac/ip address. >=20 > Documentation is a bit weak and needs improvement. In any case, except fo= r > tables and examples, whatever is written in this comment should be word f= or > word the same as what is written in rte_flow.rst. >=20 > > + */ > > +struct rte_flow_action_field_update { > > + const struct rte_flow_item *item; /**< specify the data to modify. > > +*/ >=20 > 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. >=20 > Documentation needs improvement. >=20 > > + uint8_t layer; > > + /**< 0 means outermost matched pattern, 1 means next-to-outermost... > > +*/ >=20 > uint8_t layer =3D> uint32_t level (we're not constrained by space) >=20 > Ditto re documentation. See RSS encap level patch [2] for ideas. Thanks for heads up, will check that. >=20 > > +}; > > + > > +/** RTE_FLOW_ACTION_TYPE_FIELD_COPY >=20 > Ditto for "/**". >=20 > > + * > > + * Copy data from one field to another of the packet. > > + * > > + * Typical usage: TTL copy-in / copy-out >=20 > This typical usage doesn't look that obvious to me. Copy TTL from what pa= rt of > the packet to where? What happens to the overwritten TTL value? Why shoul= d > 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 bette= r idea. Thanks Qi >=20 > > + */ > > +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... > > +*/ }; >=20 > Same comments as for struct rte_flow_action_field_update. >=20 > > + > > /** > > * Definition of a single action. > > * > > -- > > 2.7.4 > > >=20 > -- > Adrien Mazarguil > 6WIND