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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 6/28/20 7:16 PM, Jiawei(Jonny) Wang wrote:
> 
> On Sunday, June 28, 2020 4:27 PM, Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>
>> On 6/25/20 7:26 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
>>> 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.
>>>
>>> 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 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>
>>
>> [snip]
>>
>>> @@ -2709,6 +2716,28 @@ 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
>>> + * in the predefined probabiilty, All the packets continues
>>> +processing
>>> + * 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 {
>>> +	/* packets sampled equals to '1/ratio' */
>>> +	const uint32_t ratio;
>>> +	/* sub-action list specific for the sampling hit cases */
>>> +	const struct rte_flow_action *actions;
>>
>> This design idea does not look good to me from the very beginning. IMHO it
>> does not fit flow API overall design.
>> I mean sub-action list.
>>
>> As I understand Linux iptables solves it on match level (i.e. in pattern). E.g.
>> "limit" extension which is basically sampling. Sampling using meta pattern
>> item in combination with PASSTHRU action (to make sampling actions non-
>> terminating if required) is a better solution from design point of view.
> 
> On our design, there're sample flow path and normal flow path, each path can have different actions.
> The defined sub-actions list only applied for sampled packets in the sample flow path;
> For normal path, all packets will continue to go with the original actions.
> 

In my too.