[dpdk-dev,v2,4/4] ether: add packet modification aciton in flow API
Checks
Commit Message
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. These action are non-terminating action. they will not
impact the fate of the packets.
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
doc/guides/prog_guide/rte_flow.rst | 89 ++++++++++++++++++++++++++++++++++++++
lib/librte_ether/rte_flow.h | 57 ++++++++++++++++++++++++
2 files changed, 146 insertions(+)
Comments
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@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
>
Hi Adrien
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, April 12, 2018 3:04 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@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@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
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 <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> > Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> > <bruce.richardson@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@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.
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
...
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
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, April 12, 2018 6:23 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@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@6wind.com]
> > > Sent: Thursday, April 12, 2018 3:04 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > Bruce <bruce.richardson@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@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
On Fri, Apr 13, 2018 at 01:47:15PM +0000, Zhang, Qi Z wrote:
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Thursday, April 12, 2018 6:23 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> > Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> > <bruce.richardson@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@6wind.com]
> > > > Sent: Thursday, April 12, 2018 3:04 PM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > > Bruce <bruce.richardson@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.
<snip>
> > > > 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.
Drivers are free to accept an action or not depending on what is guaranteed
to be matched on the pattern side. It's fine as long as the resulting flow
rule works exactly as documented. Consistency is much more important to
applications than offloads proper.
Depending on device capabilities and the importance given to offload
specific use cases by vendors, PMD support may range from a basic 1:1
translation attempt between rte_flow and device format, to an all out
processing effort resulting in multiple device flow rules and whatnot to
satisfy the request by any means necessary (see mlx5 RSS support on empty
patterns in case you're curious).
Whichever approach you choose (basic or complex), my recommendation is
simply to make sure the PMD reports an error whenever a flow rule is
ambiguous and could result in unexpected behavior if applied as is to the
device.
The error message should also be helpful. A message such as "unable to apply
flow rule" is pretty useless, while "this action is not supported when X
pattern item is not present" actually gives useful information.
"Undefined behavior" is for application writers. It means that if a PMD
happens to accept the rule in question, what happens isn't covered by
documentation. Ideally a PMD shouldn't accept it in the first place
though.
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Monday, April 16, 2018 9:31 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in flow API
>
> On Fri, Apr 13, 2018 at 01:47:15PM +0000, Zhang, Qi Z wrote:
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > Sent: Thursday, April 12, 2018 6:23 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > Bruce <bruce.richardson@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@6wind.com]
> > > > > Sent: Thursday, April 12, 2018 3:04 PM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > > > Bruce <bruce.richardson@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.
> <snip>
> > > > > 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.
>
> Drivers are free to accept an action or not depending on what is guaranteed
> to be matched on the pattern side. It's fine as long as the resulting flow rule
> works exactly as documented. Consistency is much more important to
> applications than offloads proper.
>
> Depending on device capabilities and the importance given to offload specific
> use cases by vendors, PMD support may range from a basic 1:1 translation
> attempt between rte_flow and device format, to an all out processing effort
> resulting in multiple device flow rules and whatnot to satisfy the request by
> any means necessary (see mlx5 RSS support on empty patterns in case you're
> curious).
>
> Whichever approach you choose (basic or complex), my recommendation is
> simply to make sure the PMD reports an error whenever a flow rule is
> ambiguous and could result in unexpected behavior if applied as is to the
> device.
>
> The error message should also be helpful. A message such as "unable to
> apply flow rule" is pretty useless, while "this action is not supported when X
> pattern item is not present" actually gives useful information.
>
> "Undefined behavior" is for application writers. It means that if a PMD
> happens to accept the rule in question, what happens isn't covered by
> documentation. Ideally a PMD shouldn't accept it in the first place though.
OK, I'm trying to understand why "Undefined behavior" is necessary for this action.
For example, we have a device that can offload TCP layer modification, for packet that contain TCP, packet be modified, for no-tcp packet, the packet will be dropped
Also the device does not support TCP filter, so we are not able to create a flow to filter TCP packet only.
Then without "Undefined behavior", the driver has to reject any flow with TCP packet modification action, since it can't guarantee no-tcp packet not be impacted,
while with " Undefined behavior", a flow with tcp action is actually is a "rule in question" and it can "happens to" be accepted by the driver?
Regards
Qi
>
> --
> Adrien Mazarguil
> 6WIND
On Mon, Apr 16, 2018 at 03:03:39PM +0000, Zhang, Qi Z wrote:
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Monday, April 16, 2018 9:31 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> > Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in flow API
> >
> > On Fri, Apr 13, 2018 at 01:47:15PM +0000, Zhang, Qi Z wrote:
> > > > -----Original Message-----
> > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > Sent: Thursday, April 12, 2018 6:23 PM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > > Bruce <bruce.richardson@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@6wind.com]
> > > > > > Sent: Thursday, April 12, 2018 3:04 PM
> > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > > > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > > > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > > > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > > > > Bruce <bruce.richardson@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.
> > <snip>
> > > > > > 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.
> >
> > Drivers are free to accept an action or not depending on what is guaranteed
> > to be matched on the pattern side. It's fine as long as the resulting flow rule
> > works exactly as documented. Consistency is much more important to
> > applications than offloads proper.
> >
> > Depending on device capabilities and the importance given to offload specific
> > use cases by vendors, PMD support may range from a basic 1:1 translation
> > attempt between rte_flow and device format, to an all out processing effort
> > resulting in multiple device flow rules and whatnot to satisfy the request by
> > any means necessary (see mlx5 RSS support on empty patterns in case you're
> > curious).
> >
> > Whichever approach you choose (basic or complex), my recommendation is
> > simply to make sure the PMD reports an error whenever a flow rule is
> > ambiguous and could result in unexpected behavior if applied as is to the
> > device.
> >
> > The error message should also be helpful. A message such as "unable to
> > apply flow rule" is pretty useless, while "this action is not supported when X
> > pattern item is not present" actually gives useful information.
>
> >
> > "Undefined behavior" is for application writers. It means that if a PMD
> > happens to accept the rule in question, what happens isn't covered by
> > documentation. Ideally a PMD shouldn't accept it in the first place though.
>
> OK, I'm trying to understand why "Undefined behavior" is necessary for this action.
> For example, we have a device that can offload TCP layer modification, for packet that contain TCP, packet be modified, for no-tcp packet, the packet will be dropped
Well, this is how rte_flow is defined at a higher level than just the TCP
action we're talking about: all actions of a flow rule must happen on
matched traffic regardless, as guaranteed by the API. Actions do not perform
a second round of traffic matching, they mindlessly affect it according to
their documentation.
With that in mind, what should be the end result of a TCP-specific action on
a packet without a TCP header?
In your case, the device happens to let traffic through. On mine, traffic is
dropped. On another, a deadlock occurs in the device. This is why I think
"undefined behavior" is appropriate here.
> Also the device does not support TCP filter, so we are not able to create a flow to filter TCP packet only.
OK, now I'm starting to understand your concern.
> Then without "Undefined behavior", the driver has to reject any flow with TCP packet modification action, since it can't guarantee no-tcp packet not be impacted,
> while with " Undefined behavior", a flow with tcp action is actually is a "rule in question" and it can "happens to" be accepted by the driver?
I guess expecting applications to rely on PMD-specific (undefined) behavior
is out of the question? :)
The simplest solution to this problem is obviously to document it on an
action basis like you suggested. However doing so puts such actions at odds
with the rest of the API and is not recommended.
So far we took TCP as an example here, but before going further, is there an
actual scenario in this series where the device is unable to match the
protocol an action will affect?
For instance, I assume your device supports IPv4/IPv6 matching before
requesting a TTL-decrementing action. If so, can we stay on "undefined
behavior" for when an application doesn't match IPv4/IPv6 first?
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, April 17, 2018 5:55 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in flow API
>
> On Mon, Apr 16, 2018 at 03:03:39PM +0000, Zhang, Qi Z wrote:
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > Sent: Monday, April 16, 2018 9:31 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > Bruce <bruce.richardson@intel.com>
> > > Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in
> > > flow API
> > >
> > > On Fri, Apr 13, 2018 at 01:47:15PM +0000, Zhang, Qi Z wrote:
> > > > > -----Original Message-----
> > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > > > Sent: Thursday, April 12, 2018 6:23 PM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > > > Bruce <bruce.richardson@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@6wind.com]
> > > > > > > Sent: Thursday, April 12, 2018 3:04 PM
> > > > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > > > Cc: dev@dpdk.org; Doherty, Declan
> > > > > > > <declan.doherty@intel.com>; Chandran, Sugesh
> > > > > > > <sugesh.chandran@intel.com>; Glynn, Michael J
> > > > > > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > > > > > Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > > > > Richardson, Bruce <bruce.richardson@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.
> > > <snip>
> > > > > > > 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.
> > >
> > > Drivers are free to accept an action or not depending on what is
> > > guaranteed to be matched on the pattern side. It's fine as long as
> > > the resulting flow rule works exactly as documented. Consistency is
> > > much more important to applications than offloads proper.
> > >
> > > Depending on device capabilities and the importance given to offload
> > > specific use cases by vendors, PMD support may range from a basic
> > > 1:1 translation attempt between rte_flow and device format, to an
> > > all out processing effort resulting in multiple device flow rules
> > > and whatnot to satisfy the request by any means necessary (see mlx5
> > > RSS support on empty patterns in case you're curious).
> > >
> > > Whichever approach you choose (basic or complex), my recommendation
> > > is simply to make sure the PMD reports an error whenever a flow rule
> > > is ambiguous and could result in unexpected behavior if applied as
> > > is to the device.
> > >
> > > The error message should also be helpful. A message such as "unable
> > > to apply flow rule" is pretty useless, while "this action is not
> > > supported when X pattern item is not present" actually gives useful
> information.
> >
> > >
> > > "Undefined behavior" is for application writers. It means that if a
> > > PMD happens to accept the rule in question, what happens isn't
> > > covered by documentation. Ideally a PMD shouldn't accept it in the first
> place though.
> >
> > OK, I'm trying to understand why "Undefined behavior" is necessary for this
> action.
> > For example, we have a device that can offload TCP layer modification,
> > for packet that contain TCP, packet be modified, for no-tcp packet,
> > the packet will be dropped
>
> Well, this is how rte_flow is defined at a higher level than just the TCP action
> we're talking about: all actions of a flow rule must happen on matched traffic
> regardless, as guaranteed by the API. Actions do not perform a second round
> of traffic matching, they mindlessly affect it according to their
> documentation.
>
> With that in mind, what should be the end result of a TCP-specific action on a
> packet without a TCP header?
>
> In your case, the device happens to let traffic through. On mine, traffic is
> dropped. On another, a deadlock occurs in the device. This is why I think
> "undefined behavior" is appropriate here.
OK, I'm fine to add "undefined behavior" into document.
But besides this, would you help to review my v3 patches
Thanks
Qi
>
> > Also the device does not support TCP filter, so we are not able to create a
> flow to filter TCP packet only.
>
> OK, now I'm starting to understand your concern.
>
> > Then without "Undefined behavior", the driver has to reject any flow
> > with TCP packet modification action, since it can't guarantee no-tcp packet
> not be impacted, while with " Undefined behavior", a flow with tcp action is
> actually is a "rule in question" and it can "happens to" be accepted by the
> driver?
>
> I guess expecting applications to rely on PMD-specific (undefined) behavior is
> out of the question? :)
>
> The simplest solution to this problem is obviously to document it on an
> action basis like you suggested. However doing so puts such actions at odds
> with the rest of the API and is not recommended.
>
> So far we took TCP as an example here, but before going further, is there an
> actual scenario in this series where the device is unable to match the
> protocol an action will affect?
>
> For instance, I assume your device supports IPv4/IPv6 matching before
> requesting a TTL-decrementing action. If so, can we stay on "undefined
> behavior" for when an application doesn't match IPv4/IPv6 first?
>
> --
> Adrien Mazarguil
> 6WIND
@@ -1508,6 +1508,95 @@ Representor.
| ``port_id`` | identification of the destination |
+--------------+-----------------------------------+
+Action: ``FILED_UPDATE``
+^^^^^^^^^^^^^^^^^^^^^^^
+
+Update specific field of the packet.
+
+- Non-terminating by default.
+
+.. _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 |
+ +-----------+---------------------------------------------------------+
+ | ``layer`` | 0 means outermost matched pattern, |
+ | | 1 means next-to-outermost and so on ... |
+ +-----------+---------------------------------------------------------+
+
+Action: ``FILED_INCREMENT``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Increment 1 on specific field of the packet.
+
+- Non-terminating by default.
+
+.. _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 ... |
+ +-----------+---------------------------------------------------------+
+
+Action: ``FIELD_DECREMENT``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Decrement 1 on specific field of the packet.
+
+Paramenter is same is FIELD_INCREMENT.
+
+-Non-terminating by default.
+
+ACTION: ``FIELD_COPY``
+^^^^^^^^^^^^^^^^^^^^^^
+
+Copy data of one field to another of the packet.
+
+-Non-terminating by default.
+
+.. _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 ... |
+ +-----------------+-----------------------------------------------------------------+
+
Negative types
~~~~~~~~~~~~~~
@@ -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.
+ *
+ * See struct rte_flow_item_field_update.
+ */
+ RTE_FLOW_ACTION_TYPE_FILED_UPDATE,
+
+ /**
+ * Increment specific field of the packet.
+ *
+ * See struct rte_flow_item_field_update.
+ */
+ RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT,
+
+ /**
+ * Decrement specific field of the packet.
+ *
+ * See struct rte_flow_item_field_update.
+ */
+ RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT,
+
+ /**
+ * Copy data of one field to another of the packet.
+ *
+ * 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
+ * RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT
+ * RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT
+ *
+ * Update specific field of the packet.
+ *
+ * Typical usage: update mac/ip address.
+ */
+struct rte_flow_action_field_update {
+ const struct rte_flow_item *item; /**< specify the data to modify. */
+ uint8_t layer;
+ /**< 0 means outermost matched pattern, 1 means next-to-outermost... */
+};
+
+/** RTE_FLOW_ACTION_TYPE_FIELD_COPY
+ *
+ * Copy data from one field to another of the packet.
+ *
+ * Typical usage: TTL copy-in / copy-out
+ */
+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... */
+};
+
/**
* Definition of a single action.
*