[dpdk-dev] [PATCH v2 4/4] ether: add packet modification aciton in flow API
Zhang, Qi Z
qi.z.zhang at intel.com
Fri Apr 13 15:47:15 CEST 2018
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Thursday, April 12, 2018 6:23 PM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>
> Cc: dev at dpdk.org; Doherty, Declan <declan.doherty at intel.com>; Chandran,
> Sugesh <sugesh.chandran at intel.com>; Glynn, Michael J
> <michael.j.glynn at intel.com>; Liu, Yu Y <yu.y.liu at intel.com>; Ananyev,
> Konstantin <konstantin.ananyev at intel.com>; Richardson, Bruce
> <bruce.richardson at intel.com>
> Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in flow API
>
> On Thu, Apr 12, 2018 at 08:50:14AM +0000, Zhang, Qi Z wrote:
> > Hi Adrien
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> > > Sent: Thursday, April 12, 2018 3:04 PM
> > > To: Zhang, Qi Z <qi.z.zhang at intel.com>
> > > Cc: dev at dpdk.org; Doherty, Declan <declan.doherty at intel.com>;
> > > Chandran, Sugesh <sugesh.chandran at intel.com>; Glynn, Michael J
> > > <michael.j.glynn at intel.com>; Liu, Yu Y <yu.y.liu at intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev at intel.com>; Richardson,
> > > Bruce <bruce.richardson at intel.com>
> > > 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 <qi.z.zhang at intel.com>
> > >
> > > 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.
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 accept ETH/IPV4/TCP with TCP action base on
its capability.
>
> 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
> ...
>
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.
>
> [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
More information about the dev
mailing list