Thanks Andrew and Ori, > -----Original Message----- > From: Ori Kam <or...@mellanox.com> > Sent: Sunday, July 5, 2020 6:19 PM > To: Andrew Rybchenko <arybche...@solarflare.com>; Jiawei(Jonny) Wang > <jiaw...@mellanox.com>; Slava Ovsiienko <viachesl...@mellanox.com>; > Matan Azrad <ma...@mellanox.com> > Cc: dev@dpdk.org; Thomas Monjalon <tho...@monjalon.net>; Raslan > Darawsheh <rasl...@mellanox.com>; ian.sto...@intel.com; > f...@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-boun...@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 <jiaw...@mellanox.com> > > > Acked-by: Ori Kam <or...@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 > > >