Re: [dpdk-dev] [PATCH v4 4/6] ethdev: add mark flow item to flow item types

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

On Wed, Apr 18, 2018 at 10:04:21PM +0100, Declan Doherty wrote:
> Introduces a new action type RTE_FLOW_ITEM_TYPE_MARK which enables
> flow patterns to specify arbitrary integer values to match aginst

Typo on "aginst".

> which are set by the RTE_FLOW_ACTION_TYPE_MARK action in a 
> previously matched flow from a higher prioriry group.

prioriry => priority, however this last addition is unnecessary, it could be
any prior flow rule that happens to use PASSTHRU.

> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst | 28 ++++++++++++++++++++++++++++
>  lib/librte_ether/rte_flow.h        | 24 ++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 325010544..6f23ad909 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -598,6 +598,34 @@ associated with a port_id should be retrieved by other means.
>     | ``mask`` | ``index`` | zeroed to match any port index |
>     +----------+-----------+--------------------------------+
>  
> +Item: ``MARK``
> +^^^^^^^^^^^^^^
> +
> +Matches packets coming from a previously matched flow in a higher priority group

See above re "higher priority group".

> +with an arbitrary integer value which was set using the ``MARK`` action in the
> +previously matched rule.
> +
> +This item can only specified once as a match criteria as the ``MARK`` action can
> +only be specified once in a flow action.
> +
> +Note the value of MARK field is arbitrary and application defined.
> +
> +- Default ``mask`` matches any integer value.
> +
> +.. _table_rte_flow_item_mark:
> +
> +.. table:: MARK
> +
> +   +----------+----------+---------------------------+
> +   | Field    | Subfield | Value                     |
> +   +==========+==========+===========================+
> +   | ``spec`` | ``id``   | integer value             |
> +   +----------+--------------------------------------+
> +   | ``last`` | ``id``   | upper range value         |
> +   +----------+----------+---------------------------+
> +   | ``mask`` | ``id``   | zeroed to match any value |
> +   +----------+------- --+---------------------------+
> +
>  Data matching item types
>  ~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 91544f62b..56e262a23 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -323,6 +323,13 @@ enum rte_flow_item_type {
>  	 * See struct rte_flow_item_geneve.
>  	 */
>  	RTE_FLOW_ITEM_TYPE_GENEVE,
> +
> +	/**
> +	 * Matches specified mark field.
> +	 *
> +	 * See struct rte_flow_item_mark.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_MARK

Remember to add a trailing comma.

>  };
>  
>  /**
> @@ -814,6 +821,23 @@ static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
>  };
>  #endif
>  
> +/**
> + * RTE_FLOW_ITEM_TYPE_MARK
> + *
> + * Allow arbitrary integer value set using MARK action in a flow rule in a
> + * higher priority group to be matched against in flow rule in a lower
> + * priority group.
> + *
> + * This value is arbitrary and application-defined. Maximum allowed value
> + * depends on the underlying implementation.
> + *
> + * Depending on the underlying implementation the MARK item may be supported in
> + * the physical device, virtually in logical groups in the PMD or not at all.

See above regarding groups. Also please synchronize this with the contents
of rte_flow.rst. They should be the same.

> + */
> +struct rte_flow_item_mark {
> +	uint32_t id; /**< Integer value to match against. */
> +};
> +
>  /**
>   * Matching pattern item definition.
>   *
> -- 
> 2.14.3
> 

Otherwise this patch looks good.

-- 
Adrien Mazarguil
6WIND