Re: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action for rte flow

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 7/2/20 9:43 PM, Jiawei Wang wrote:
> When using full offload, all traffic will be handled by the HW, and
> directed to the requested vf or wire, the control application loses

vf->VF

> visibility on the traffic.
> So there's a need for an action that will enable the control application
> some visibility.
> 
> The solution is introduced a new action that will sample the incoming
> traffic and send a duplicated traffic in some predefined ratio to the
> application, while the original packet will continue to the target
> destination.
> 

May be 1 packet per second is a better sampling approach?
Or just different.

> The packets sampled equals is '1/ratio', if the ratio value be set to 1
> , means that the packets would be completely mirrored. The sample packet

Comma on the next line looks bad.

> can be assigned with different set of actions from the original packet.
> 
> In order to support the sample packet in rte_flow, new rte_flow action
> definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure rte_flow_action_sample
> will be introduced.
> 
> Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> Acked-by: Ori Kam <orika@mellanox.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst     | 25 +++++++++++++++++++++++++
>  doc/guides/rel_notes/release_20_08.rst |  6 ++++++
>  lib/librte_ethdev/rte_flow.c           |  1 +
>  lib/librte_ethdev/rte_flow.h           | 28 ++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d5dd18c..50dfe1f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2645,6 +2645,31 @@ timeout passed without any matching on the flow.
>     | ``context``  | user input flow context         |
>     +--------------+---------------------------------+
>  
> +Action: ``SAMPLE``
> +^^^^^^^^^^^^^^^^^^
> +
> +Adds a sample action to a matched flow.
> +
> +The matching packets will be duplicated to a special queue or vport

what is vport above?

> +with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
> +All the packets continues to the target destination.

continues -> continue (if I'm not mistaken)

> +
> +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> +``actions`` represent the different set of actions for the sampled or mirrored
> +packets.
> +
> +.. _table_rte_flow_action_sample:
> +
> +.. table:: SAMPLE
> +
> +   +--------------+---------------------------------+
> +   | Field        | Value                           |
> +   +==============+=================================+
> +   | ``ratio``    | 32 bits sample ratio value      |
> +   +--------------+---------------------------------+
> +   | ``actions``  | sub-action list for sampling    |
> +   +--------------+---------------------------------+
> +
>  Negative types
>  ~~~~~~~~~~~~~~
>  
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 5cbc4ce..313e8d3 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -81,6 +81,12 @@ New Features
>    * Added support for virtio queue statistics.
>    * Added support for MTU update.
>  
> +* **Added flow-based traffic sampling support.**
> +
> +  Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate the matching
> +  packets with given ratio and redirects to vport or queue. The sampled packets

What is vport above?

> +  also can be assigned with an additional optional actions.

May actions list be empty or NULL? If no, it does not look
optional.

> +
>  * **Updated Marvell octeontx2 ethdev PMD.**
>  
>    Updated Marvell octeontx2 driver with cn98xx support.
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 1685be5..733871d 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
>  	MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct rte_flow_action_set_dscp)),
>  	MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)),
>  	MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> +	MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
>  };
>  
>  int
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index b0e4199..c9cd80d 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
>  	 * see enum RTE_ETH_EVENT_FLOW_AGED
>  	 */
>  	RTE_FLOW_ACTION_TYPE_AGE,
> +
> +	/**
> +	 * Redirects specific ratio of packets to vport or queue.
> +	 *
> +	 * See struct rte_flow_action_sample.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_SAMPLE,
>  };
>  
>  /**
> @@ -2709,6 +2716,27 @@ struct rte_flow_action {
>  struct rte_flow;
>  
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_SAMPLE
> + *
> + * Adds a sample action to a matched flow.
> + *
> + * The matching packets will be duplicated to a special queue or vport

again 'vport' here
It sounds misleading and too restrictive to say "be duplicated
to a special queue or vport". There is no specification of the
queue or vport in the control structure.
You should either describe it in a generic way like "be
duplicated and own set of actions with a fate action applied"
or put a restriction about QUEUE, RSS or "vport"-related action
to be present in the sub-actions list.

> + * in the predefined probabiilty, All the packets continues processing

probabiilty -> probability
I think 'predefined' is misleading here, 'specified' is better.
Also strictly speaking it is not a predefined probability (as
Stephen suggested), it is defined ratio.

> + * on the default flow path.
> + *
> + * When the sample ratio is set to 1 then the packets will be 100% mirrored.
> + * Additional action list be supported to add for sampled or mirrored packets.
> + */
> +struct rte_flow_action_sample {
> +	const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */

const is still above and it is meaningless (other actions do
not have 'const' for plain fields).

> +	const struct rte_flow_action *actions;
> +		/**< sub-action list specific for the sampling hit cases. */

Is it required to have fate action?
May I use it to MARK some packets and do not duplicate?
I guess no. Or COUNT and DROP? Just COUNT?

What I'm trying to say that you're adding a generic packet
selection mechanism with very restricted usage by design.

Anyway, if you go with it, please, process other notes above.

> +};
> +
> +/**
>   * Verbose error types.
>   *
>   * Most of them provide the type of the object referenced by struct
>