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
>
> > 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.
>
> > 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)
> > +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
> >
