Hi All,
After considering both approaches, I think the original is the better approach.
Acked-by: Ori Kam <or...@mellanox.com>
Thanks,
Ori
> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of Ori Kam
> Sent: Monday, June 29, 2020 5:30 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;
> Adrien Mazarguil <adrien.mazarg...@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH 1/8] ethdev: introduce sample action for rte
> flow
>
> Hi All,
>
> > -----Original Message-----
> > From: Andrew Rybchenko <arybche...@solarflare.com>
> > Sent: Monday, June 29, 2020 4:12 PM
> > To: Ori Kam <or...@mellanox.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;
> > Adrien Mazarguil <adrien.mazarg...@6wind.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/8] ethdev: introduce sample action for rte
> > flow
> >
> > Hi all,
> >
> > CC Adrien
> >
> > (I apologize for pulling you to the rte_flow API discussions
> > once again, but may be you can find spare time and share your
> > thoughts. Your opinion as an author and architect of the
> > rte_flow API would be very useful and highly appreciated.)
> >
> > On 6/29/20 2:40 PM, Ori Kam wrote:
> > > Hi all,
> > >
> > >> -----Original Message-----
> > >> From: Andrew Rybchenko <arybche...@solarflare.com>
> > >> Sent: Sunday, June 28, 2020 7:19 PM
> > >> To: Jiawei(Jonny) Wang <jiaw...@mellanox.com>; Ori Kam
> > >> <or...@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 1/8] ethdev: introduce sample action for
> rte
> > >> flow
> > >>
> > >> On 6/28/20 7:16 PM, Jiawei(Jonny) Wang wrote:
> > >>>
> > >>> On Sunday, June 28, 2020 4:27 PM, Andrew Rybchenko
> > >> <arybche...@solarflare.com> wrote:
> > >>>>
> > >>>> On 6/25/20 7:26 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
> > >>>>> 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.
> > >>>>>
> > >>>>> 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 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>
> > >>>>
> > >>>> [snip]
> > >>>>
> > >>>>> @@ -2709,6 +2716,28 @@ 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
> > >>>>> + * in the predefined probabiilty, All the packets continues
> > >>>>> +processing
> > >>>>> + * 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 {
> > >>>>> + /* packets sampled equals to '1/ratio' */
> > >>>>> + const uint32_t ratio;
> > >>>>> + /* sub-action list specific for the sampling hit cases */
> > >>>>> + const struct rte_flow_action *actions;
> > >>>>
> > >>>> This design idea does not look good to me from the very beginning.
> IMHO
> > it
> > >>>> does not fit flow API overall design.
> > >>>> I mean sub-action list.
> > >>>>
> > >>>> As I understand Linux iptables solves it on match level (i.e. in
> > >>>> pattern).
> E.g.
> > >>>> "limit" extension which is basically sampling. Sampling using meta
> pattern
> > >>>> item in combination with PASSTHRU action (to make sampling actions
> > non-
> > >>>> terminating if required) is a better solution from design point of
> > >>>> view.
> > >>>
> > >>> On our design, there're sample flow path and normal flow path, each
> path
> > >> can have different actions.
> > >>> The defined sub-actions list only applied for sampled packets in the
> sample
> > >> flow path;
> > >>> For normal path, all packets will continue to go with the original
> > >>> actions.
> > >>>
> > >>
> > >> In my too.
> > >
> > > First as far as I know TC works close to the suggest approach (that by
> > > itself
> > doesn’t mean anything)
> > > The concept of a PASSTHRU is a good one but it has some issue to consider:
> > > 1. When using PASSTHRU it will mean that the matching part will be needed
> > to be checked
> > > more times this will have performance penalty , also number of HW have
> > limited number of flow that can be offload
> > > this will approach will waste resources.
> >
> > Marking or tagging could be used to address it. E.g. target
> > traffic could be tagged first, then matching by tag should be
> > used to sample and to do HW offloads.
> >
> In this case you are forcing at least two steps, this will hurt performance.
> Matching on mark is the same as matching on other items. While it may have
> extra
> penalty to add the extra mark action. (this is general HW issue I assume
> number of
> manufactures have the same limitation)
>
> > Moreover, mapping of rte_flow API rules into HW rule is not
> > required to be 1-to-1. Yes, 1-to-1 is simple, but it could be
> > more complicated 1-to-N (when one rte_flow API rule is
> > represented by many HW rules) or N-to-1 (when few flow API
> > rules are represented as one HW rule) or even N-to-M.
> > For example, tagging which is not visible outside, could be
> > purely SW and used to build such constructions in SW.
> > It is an implementation detail is out of scope of the generic
> > API definition.
> >
> I don’t understand this part. I never said that 1 to 1 is needed
> but if you try to combine flows in SW it means that you must keep all flows
> in the PMD in order to combine them. I now some HW must do that, but not
> all of them and if they don’t it is just a huge memory waste.
>
> > Yes, it sounds like over-complicating, but I really dislike
> > above sub-action list from design point of view and that's
> > why I"m trying to think in different directions.
> >
> Thinking in different direction is always good.
> I just think that between the two approaches I like the original better.
> May be you can explain your reason for your opinion and we can find the best
> solution together?
>
>
>
> > > 2. Using PASSTHRU will force the order of flows (sure it can be done using
> > priorities but it is more complex to
> > > the application to implement)
> >
> > See above.
> >
>
> See above 😊
>
> > > 3. PASSTHRU will mean that there will be 2 terminal action for each flow
> (for
> > example queue index 2 / passthru)
> > > this also is not native to RTE flow.
> >
> > Sorry, but there are two branches for terminating actions in
> > the sampling action design anyway (yes, internal/hidden).
> > You need two copies of the packet, so whatever you do it
> > will be two terminating actions.
> >
> Yes but you can look at it as 1 flow with 2 sets of actions and not two flows.
>
> > > 4. since we want to select only part of the packets, and we want to have
> some
> > of the actions done on both
> > > packets (the sampled and the standard one) and then we want on the
> > sampled packet do some specific actions
> > > while on the standard packet do different actions.
> >
> > Yes, it is not a problem with PASSTHRU.
> >
> > > Lest check the following use case:
> > > Application is using full offload traffic from the wire to a VM, which
> > > should
> > decaped
> > > So the basic flow is:
> > > Flow create 0 transfer ingress pattern eth / outer.ip =x / end actions
> > > decap
> /
> > port id 3
> > > Since after the offload the application loses visibility of the traffic.
> > > it still
> > wants to sample some of the traffic
> > > in order to verify that the traffic is valid. So the application request
> > > to
> receive
> > some of the original traffic and
> > > mark it with id.
> > >
> > > If we use the original approach (the one in the patch) we will need
> something
> > like this:
> > > Flow 1: flow create 0 transfer ingress pattern eth / outer.ip=x / end
> > > actions
> > sample(ratio 2, actions mark id 3 / port pf)) / decap / port 3
> > >
> > > In the PASSTHRU concept (I'm not sure I can even create such flows)
> > > Flow 1: flow create 0 transfer ingress pattern eth / outer.ip =x / end
> > > actions
> > decap / port 2 /passtthru // original request
> > > Flow 2: flow create 0 transfer ingress pattern eth/ outer.ip=x / should
> sample
> > (new item that selects if the packet is selected based on the ratio)end act
> > /
> > mark / port pf
> > >
> > > The main issue with this case the decap is before the sample so the sample
> > will get decap packet.
> >
> > Order should be simply different: first sampling with pass-
> > through, second decap and deliver to VM.
> >
> Then in your case the packet will be marked. Which is not what the
> application requested.
>
> > Yes, I realize that two actions with basically the same match
> > (modulo sampling match) is not ideal for mapping to HW (even
> > if it collapsed into trivial tag match which pre-rule to make
> > tagging). I'm not 100% happy with it, but I'm even less happy
> > with sub-action list design and just trying to find better
> > alternative solution.
> >
> Please see above, maybe we can find the best solution together.
>
> > > So when looking at everything I think the original API is the best
> > > approach.
> > > For the record I think that passthru action is very important and should
> > > be
> > supported but not the best one for this feature.
> > >
> > > Thanks,
> > > Ori
> > >
> Best,
> Ori
