> -----Original Message-----
> From: Yongseok Koh
> Sent: Friday, October 26, 2018 6:07
> To: Slava Ovsiienko <viachesl...@mellanox.com>
> Cc: Shahaf Shuler <shah...@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 <viachesl...@mellanox.com>
> > > Cc: Shahaf Shuler <shah...@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.
> > >
