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

[Jiawei(Jonny) Wang]

Thanks Andrew and Ori,

> -----Original Message-----
> From: Ori Kam <orika at mellanox.com>
> Sent: Sunday, July 5, 2020 6:19 PM
> To: Andrew Rybchenko <arybchenko at solarflare.com>; Jiawei(Jonny) Wang
> <jiaweiw at mellanox.com>; Slava Ovsiienko <viacheslavo at mellanox.com>;
> Matan Azrad <matan at mellanox.com>
> Cc: dev at dpdk.org; Thomas Monjalon <thomas at monjalon.net>; Raslan
> Darawsheh <rasland at mellanox.com>; ian.stokes at intel.com;
> fbl at redhat.com
> Subject: RE: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action for
> rte flow
> 
> Hi Andrew,
> 
> I replied to some of your comments.
> Best,
> Ori
> > -----Original Message-----
> > From: dev <dev-bounces at dpdk.org> On Behalf Of Andrew Rybchenko
> > Sent: Saturday, July 4, 2020 4:05 PM
> > Subject: Re: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action
> > for rte flow
> >
> > 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
will change in v3 patch.
> >
> > > 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.
> >
> Those are two different things, lets take a packet that arrives once every two
> seconds and we ask to sample once every second, this means that we will
> always get that packet.
> Also as far as I understand the use case is to have some visibility about the
> traffic.
> so you can assume that if a packet is sent once per second the application
> will get the packet with very high delay and very low visibility. Lets take a use
> case that the hyprivor wants to check if one of the VM is abusing the system
> (sends DDOS packets, or just trying to understand the network) in this case
> we can assume that the VM will send large amount of traffic. and if we only
> check once per second the application will not be able to understand the
> traffic meaning, but if we sample 1% of the traffic then the application will
> see very fast the type of the traffic the VM is sending and if it is trying to
> abuse the system.
> So I vote in favor of keeping as is.
> 
> 
> > > 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.
ok, will move the Comma to the same line.
> >
> > > 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 at mellanox.com>
> > > Acked-by: Ori Kam <orika at 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?
> >
> I think it should be port (when using E-Switch)
Yes, the destination port is working on e-switch mode, change vport->port.
> 
> > > +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)
ok, will change.
> >
> > > +
> > > +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?
> >
> See comment above.
> 
> > > +  also can be assigned with an additional optional actions.
> >
> > May actions list be empty or NULL? If no, it does not look optional.
> >
> I think that the action list can't be NULL or empty. There is no meaning to
> empty list.
> I agree it should be stated.
> 
'optional' means that besides an fate action,  also can combine with addition action if needed,
like mark and queue for sampled packet.

> > > +
> > >  * **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.
> >
I'll change the description to "The matching packets will be duplicated and applied with own set of actions with a fate action"
> > > + * 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.
> >
Thanks for pointing out typo, I will use 'specified ratio' instead of.
> > > + * 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).
> >
> +1
agree,  remove 'const'.
> 
> > > +	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?
> >
> I from my understanding you may use mark and count but it also must have
> a fate action.
> 
Yes, need fate action for sampling, mark or count is optional action.
> > 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
> > >