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 <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? > +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 >