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: 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?

> 	- validate items
> 
> Thanks,
> Yongseok
> 

With best regards,
Slava

> > >
> > > For example, you just can call vxlan decap item validation (by splitting
> > > flow_tcf_validate_vxlan_decap()) at this point like:
> > >
> > > 			if (action_flags &
> > > MLX5_FLOW_ACTION_VXLAN_DECAP)
> > > 				ret =
> > > flow_tcf_validate_vxlan_decap_ipv4(...);
> > > 			...
> > >
> > > Same for other items.
> > >