[dpdk-dev] [PATCH v2 4/4] ether: add packet modification aciton in flow API

[Zhang, Qi Z]

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?

> 
> 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. 

> 
> 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.

> 
> > +
> > +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

> > +
> >  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. 

> 
> 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.

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