Re: [dpdk-dev] [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation routine

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----Original Message-----
> From: Yongseok Koh
> Sent: Saturday, October 27, 2018 0:57
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation
> routine
> 
> On Fri, Oct 26, 2018 at 01:39:38AM -0700, Slava Ovsiienko wrote:
> > > -----Original Message-----
> > > From: Yongseok Koh
> > > Sent: Friday, October 26, 2018 6:07
> > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> > > Subject: Re: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation
> > > routine
> > >
> > > On Thu, Oct 25, 2018 at 06:53:11AM -0700, Slava Ovsiienko wrote:
> > > > > -----Original Message-----
> > > > > From: Yongseok Koh
> > > > > Sent: Tuesday, October 23, 2018 13:05
> > > > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > > > Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> > > > > Subject: Re: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow
> > > > > validation routine
> > > > >
> > > > > On Mon, Oct 15, 2018 at 02:13:30PM +0000, Viacheslav Ovsiienko
> wrote:
> > > [...]
> > > > > > @@ -1114,7 +1733,6 @@ struct pedit_parser {
> > > > > >  							   error);
> > > > > >  			if (ret < 0)
> > > > > >  				return ret;
> > > > > > -			item_flags |=
> MLX5_FLOW_LAYER_OUTER_L3_IPV4;
> > > > > >  			mask.ipv4 = flow_tcf_item_mask
> > > > > >  				(items, &rte_flow_item_ipv4_mask,
> > > > > >  				 &flow_tcf_mask_supported.ipv4,
> @@ -1135,13 +1753,22 @@
> > > > > > struct pedit_parser {
> > > > > >  				next_protocol =
> > > > > >  					((const struct
> rte_flow_item_ipv4 *)
> > > > > >  					 (items->spec))-
> >hdr.next_proto_id;
> > > > > > +			if (item_flags &
> > > > > MLX5_FLOW_LAYER_OUTER_L3_IPV4) {
> > > > > > +				/*
> > > > > > +				 * Multiple outer items are not
> allowed as
> > > > > > +				 * tunnel parameters, will raise an
> error later.
> > > > > > +				 */
> > > > > > +				ipv4 = NULL;
> > > > >
> > > > > Can't it be inner then?
> > > > AFAIK,  no for tc rules, we can not specify multiple levels (inner
> > > > + outer) for
> > > them.
> > > > There is just no TCA_FLOWER_KEY_xxx attributes  for specifying
> > > > inner
> > > items
> > > > to match by flower.
> > >
> > > When I briefly read the kernel code, I thought TCA_FLOWER_KEY_* are
> > > for inner header before decap. I mean TCA_FLOWER_KEY_IPV4_SRC is
> for
> > > inner L3 and TCA_FLOWER_KEY_ENC_IPV4_SRC is for outer tunnel
> header.
> > > Please do some experiments with tc-flower command.
> >
> > Hm. Interesting. I will check.
> >
> > > > It is quite unclear comment, not the best one, sorry. I did not
> > > > like it too, just forgot to rewrite.
> > > >
> > > > ipv4, ipv6 , udp variables gather the matching items during the
> > > > item list
> > > scanning,
> > > > later variables are used for VXLAN decap action validation only.
> > > > So, the
> > > "outer"
> > > > means that ipv4 variable contains the VXLAN decap outer addresses,
> > > > and should be NULL-ed if multiple items are found in the items list.
> > > >
> > > > But we can generate an error here if we have valid action_flags
> > > > (gathered by prepare function) and VXLAN decap is set. Raising an
> > > > error looks more relevant and clear.
> > >
> > > You can't use flags at this point. It is validate() so prepare()
> > > might not be preceded.
> > >
> > > > >   flow create 1 ingress transfer
> > > > >     pattern eth src is 66:77:88:99:aa:bb
> > > > >       dst is 00:11:22:33:44:55 / ipv4 src is 2.2.2.2 dst is 1.1.1.1 /
> > > > >       udp src is 4789 dst is 4242 / vxlan vni is 0x112233 /
> > > > >       eth / ipv6 / tcp dst is 42 / end
> > > > >     actions vxlan_decap / port_id id 2 / end
> > > > >
> > > > > Is this flow supported by linux tcf? I took this example from
> > > > > Adrien's
> > > patch -
> > > > > "[8/8] net/mlx5: add VXLAN decap support to switch flow rules".
> > > > > If so,
> > > isn't it
> > > > > possible to have inner L3 layer (MLX5_FLOW_LAYER_INNER_*)? If
> > > > > not,
> > > you
> > > > > should return error in this case. I don't see any code to check
> > > > > redundant outer items.
> > > > > Did I miss something?
> > > >
> > > > Interesting, besides rule has correct syntax, I'm not sure whether
> > > > it can be
> > > applied w/o errors.
> > >
> > > Please try. You owns this patchset. However, you just can prohibit
> > > such flows (tunneled item) and come up with follow-up patches to
> > > enable it later if it is support by tcf as this whole patchset
> > > itself is pretty huge enough and we don't have much time.
> > >
> > > > At least our current flow_tcf_translate() implementation does not
> > > > support
> > > any INNERs.
> > > > But it seems the flow_tcf_validate() does, it's subject to recheck
> > > > - we
> > > should not allow
> > > > unsupported items to pass the validation. I'll check and provide
> > > > the
> > > separate bugfix patch
> > > > (if any).
> > >
> > > Neither has tunnel support. It is the first time to add tunnel support to
> TCF.
> > > If it was needed, you should've added it, not skipping it.
> > >
> > > You can check how MLX5_FLOW_LAYER_TUNNEL is used in Verbs/DV as
> a
> > > reference.
> >
> > Yes. I understood your point. Will check and add tunnel support for TCF
> rules.
> > Anyway, inner MAC addresses are supported for VXLAN decap, I think we
> > should specify these ones in the rule as inners (after VNI item),
> > definitely some tunnel support in validate/parse/translate should be added.
> >
> > >
> > > > > BTW, for the tunneled items, why don't you follow the code of
> > > > > Verbs(mlx5_flow_verbs.c) and DV(mlx5_flow_dv.c)? For tcf, it is
> > > > > the first
> > > time
> > > > For VXLAN it has some specifics (warning about ignored params,
> > > > etc.) I've checked which of verbs/dv code could be reused and did
> > > > not
> > > discovered
> > > > a lot. I'll recheck the latest code commits, possible it became
> > > > more
> > > appropriate
> > > > for VXLAN.
> > >
> > > Agreed. I'm not forcing you to do it because we run out of time but
> > > mentioned it because if there's any redundancy in our code, that
> > > usually causes bug later.
> > > Let's not waste too much time for that. Just grab low hanging fruits if
> any.
> > >
> > > > > to add tunneled item, but Verbs/DV already have validation code
> > > > > for
> > > tunnel,
> > > > > so you can reuse the existing code. In
> > > > > flow_tcf_validate_vxlan_decap(),
> > > not
> > > > > every validation is VXLAN-specific but some of them can be
> > > > > common
> > > code.
> > > > >
> > > > > And if you need to know whether there's the VXLAN decap action
> > > > > prior to outer header item validation, you can relocate the code
> > > > > - action
> > > validation
> > > > > first and item validation next, as there's no dependency yet in
> > > > > the current
> > > >
> > > > We can not validate action first - we need items to be preliminary
> > > gathered,
> > > > to check them in action's specific fashion and to check action itself.
> > > > I mean, if we see VXLAN decap action, we should check the presence
> > > > of L2, L3, L4 and VNI items. I minimized the number of passes
> > > > along the item and action lists. BTW, Adrien's approach performed
> > > > two passes, mine does
> > > only.
> > > >
> > > > > code. Defining ipv4, ipv6, udp seems to make the code path more
> > > complex.
> > > > Yes, but it allows us to avoid the extra item list scanning and
> > > > minimizes the
> > > changes
> > > > of existing code.
> > > > In your approach we should:
> > > > - scan actions, w/o full checking, just action_flags gathering and
> > > > checking
> > > > - scan items, performing variating check (depending on gathered
> > > > action
> > > flags)
> > > > - scan actions again, performing full check with params (at least
> > > > for now check whether all params gathered)
> > >
> > > Disagree. flow_tcf_validate_vxlan_encap() doesn't even need any info
> > > of items and flow_tcf_validate_vxlan_decap() needs item_flags to
> > > check whether VXLAN item is there or not and ipv4/ipv6/udp are all
> > > for item checks. Let me give you very detailed exmaple:
> > >
> > > {
> > > 	for (actions[]...) {
> > > 		...
> > > 		case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP:
> > > 			...
> > > 			flow_tcf_validate_vxlan_encap();
> > > 			...
> > > 			break;
> > > 		case RTE_FLOW_ACTION_TYPE_VXLAN_DECAP:
> > > 			if (action_flags & (MLX5_ACTION_VXLAN_ENCAP
> > > 					   | MLX5_ACTION_VXLAN_DECAP))
> > > 				return rte_flow_error_set
> > > 					(error, ENOTSUP,
> > > 					 RTE_FLOW_ERROR_TYPE_ACTION,
> > > 					 actions,
> > > 					 "can't have multiple vxlan actions");
> > > 			/* Don't call flow_tcf_validate_vxlan_decap(). */
> > > 			action_flags |= MLX5_ACTION_VXLAN_DECAP;
> > > 			break;
> > > 	}
> > > 	for (items[]...) {
> > > 		...
> > > 		case RTE_FLOW_ITEM_TYPE_IPV4:
> > > 			/* Existing common validation. */
> > > 			...
> > > 			if (action_flags & MLX5_ACTION_VXLAN_DECAP) {
> > > 				/* Do ipv4 validation in
> > > 				 * flow_tcf_validate_vxlan_decap()/
> > > 			}
> > > 			break;
> > > 	}
> > > }
> > >
> > > Curretly you are doing,
> > >
> > > 	- validate items
> > > 	- validate actions
> > > 	- validate items again if decap.
> > >
> > > But this can simply be
> > >
> > > 	- validate actions
> > How  we could validate VXLAN decap at this stage?
> > As we do not have item_flags set yet?
> > Do I miss something?
> 
> Look at my pseudo code above.
> Nothing much to be done in validating decap action. And item validation for
> decap can be done together in item validation code.
> 
VXLAB decap action should check:
- whether outer destination UDP port is present (otherwise we cannot assign VTEP VXLAN)
- whether outer destination IP is present (otherwise we cannot assign IP to ifouter/build route)
- whether VNI is present (to identify VXLAN traffic)

How do you  propose check these issues in your approach?

With best regards,
Slava


> Thanks,
> Yongseok
> 
> >
> > > 	- validate items
> > >