Re: [dpdk-dev] [PATCH v4 2/6] ethdev: Add jump action type to rte_flow

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Wed, Apr 18, 2018 at 10:04:19PM +0100, Declan Doherty wrote:
> Add jump action type which defines an action which allows a matched
> flow to be redirect to the specified group. This allows physical and
> logical flow table/group hierarchies to be managed through rte_flow.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

You should have rebased this series including this patch on top of mine
([1] followed by [2]) to avoid some mistakes such as documenting
"terminating actions".

This patch could also update documentation for flow rules [3] groups [4] and
priorities [5] to make clear that groups aren't linked by default, their
order doesn't matter and explicit JUMP actions are needed to reach them
(actually look for every instance of the word "group" in this
documentation).

Also the addition of an action in the middle of the enum has an ABI impact,
please put a reminder in the commit log as in [6].

More below.

[1] "Bunch of flow API-related fixes"
    http://dpdk.org/ml/archives/dev/2018-April/098035.html
[2] "Flow API overhaul for switch offloads"
    http://dpdk.org/ml/archives/dev/2018-April/098047.html
[3] "9.2.1. Description"
     https://dpdk.org/doc/guides/prog_guide/rte_flow.html#description
[3] "9.2.2.1. Attribute: Group"
    https://dpdk.org/doc/guides/prog_guide/rte_flow.html#attribute-group
[5] "9.2.2.2. Attribute: Priority"
    https://dpdk.org/doc/guides/prog_guide/rte_flow.html#attribute-priority
[6] "ethdev: add physical port action to flow API"
    http://dpdk.org/ml/archives/dev/2018-April/098062.html

> ---
>  doc/guides/prog_guide/rte_flow.rst | 26 ++++++++++++++++++++++++--
>  lib/librte_ether/rte_flow.h        | 27 +++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 672473d33..325010544 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1188,6 +1188,28 @@ flow rules:
>     | 2     | END                        |
>     +-------+----------------------------+
>  
> +

Extraneous empty line.

> +Action: ``JUMP``
> +^^^^^^^^^^^^^^^^
> +
> +Redirects packets to a group on the current device.
> +
> +In a hierarchy of groups, which can be used to represent physical or logical
> +flow group/tables on the device, this action allows the terminating action to
> +be a group on that device.
> +
> +- Terminating by default.

Since there are no more "terminating" actions, you should document that it
will cause traffic to be processed by flow rules found in the target group.

Also I think you should put emphasis on undefined behavior:

- Targeting a group that doesn't include any flow rule.
- Triggering loops.

> +
> +.. _table_rte_flow_action_jump:
> +
> +.. table:: JUMP
> +
> +   +-----------+---------------------------------+
> +   | Field     | Value                           |
> +   +===========+=================================+
> +   | ``group`` | Group ID to redirect packets to |
> +   +-----------+---------------------------------+

Nit: "Group ID" => "group ID" to keep the style.

> +
>  Action: ``MARK``
>  ^^^^^^^^^^^^^^^^
>  
> @@ -1512,7 +1534,7 @@ the RTE_FLOW_ITEM_TYPE_END item type.
>     | ``definition`` | Tunnel end-point overlay definition |
>     +----------------+-------------------------------------+
>  
> -.. _table_rte_flow_action_tunnel_encap_example:
> +.. _table_rte_flow_action_tunnel_encap_vxlan_example:
>  
>  .. table:: IPv4 VxLAN flow pattern example.
>  
> @@ -1551,7 +1573,7 @@ NVGRE network overlay which conforms with RFC 7637 (NVGRE: Network Virtualizatio
>  Using Generic Routing Encapsulation). The pattern must be terminated with
>  the RTE_FLOW_ITEM_TYPE_END item type.
>  
> -.. _table_rte_flow_action_tunnel_encap_example:
> +.. _table_rte_flow_action_tunnel_encap_nvgre_example:

The above two hunks do not seem relevant.

>
>  .. table:: IPv4 NVGRE flow pattern example.
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 537e1bfba..91544f62b 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -913,6 +913,20 @@ enum rte_flow_action_type {
>  	 */
>  	RTE_FLOW_ACTION_TYPE_PASSTHRU,
>  
> +	/**
> +	 * RTE_FLOW_ACTION_TYPE_JUMP
> +	 *
> +	 * Redirects packets to a group on the current device.
> +	 *
> +	 * In a hierarchy of groups, which can be used to represent
> +	 * physical or logical flow groups/tables on a device, this
> +	 * action allows the terminating action to be a group on
> +	 * that device.

Since struct rte_flow_action_jump provides complete documentation, no need
to repeat this paragraph here. A single line is enough (see other actions).

> +	 *
> +	 * See struct rte_flow_action_jump

Nit: missing "."

> +	 */
> +	RTE_FLOW_ACTION_TYPE_JUMP,
> +
>  	/**
>  	 * [META]
>  	 *
> @@ -1213,6 +1227,19 @@ struct rte_flow_action_tunnel_encap {
>  	 */
>  };
>  
> +/**
> + * RTE_FLOW_ACTION_TYPE_JUMP
> + *
> + * Redirects packets to a group on the current device.
> + *
> + * In a hierarchy of groups, which can be used to represent physical or logical
> + * flow tables on the device, this action allows the action to be a redirect to
> + * a group on that device.

Remember to synchronize this paragraph after making changes to rte_flow.rst.

Here you can also provide more info regarding undefined behavior (useful in
Doxygen format for application writers).

> + */
> +struct rte_flow_action_jump {
> +	uint32_t group;
> +};
> +

You should move this definition before "struct rte_flow_action_mark" to keep
the same definition order as in the enum.

>  /**
>   * Definition of a single action.
>   *
> -- 
> 2.14.3
> 

-- 
Adrien Mazarguil
6WIND