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

[Adrien Mazarguil]

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.

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

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.

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.

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?

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

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

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

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

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.

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.

> +};
> +
> +/** 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?

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