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
