[dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action for rte flow
Ajit Khaparde
ajit.khaparde at broadcom.com
Mon Jul 6 01:54:50 CEST 2020
On Sun, Jul 5, 2020 at 3:19 AM Ori Kam <orika at mellanox.com> wrote:
> 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
> >
> > > 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.
>
Thanks for bringing this up.
So an application may want to sample either "finite" packets per second or
"a percentage" of packets per second or "all" packets.
So a "uint32_t ratio" may not just be enough by itself.
Maybe we need to also couple it with a unit.
uint32_t sampling_unit; /* Specifies one of the units to use while
sampling. */
RTE_FLOW_NUM_PACKETS_PER_SEC /* Samples specific number of packets per
second. */
RTE_FLOW_PERCENT_PACKETS_PER_SEC /* Samples a percentage of packets per
second. */
RTE_FLOW_ALL_PACKETS /* SAMPLES all packets - equivalent to mirror */
This may be redundant if percentage is specified and ratio is 100.
In that case instead of "uint32_t ratio", just use "uint32_t sample"?
>
> > > 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 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)
>
> > > +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?
> >
> 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.
>
> > > +
> > > * **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).
> >
> +1
>
> > > + 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.
>
> > 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
> > >
>
>
More information about the dev
mailing list