On Thu, Jul 12, 2018 at 12:47:09PM +0200, Adrien Mazarguil wrote:
> On Wed, Jul 11, 2018 at 06:10:25PM -0700, Yongseok Koh wrote:
> > On Wed, Jun 27, 2018 at 08:08:18PM +0200, Adrien Mazarguil wrote:
> > > This enables flow rules to explicitly match VLAN traffic (VLAN pattern
> > > item) and perform various operations on VLAN headers at the switch level
> > > (OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID and OF_SET_VLAN_PCP actions).
> > >
> > > Testpmd examples:
> > >
> > > - Directing all VLAN traffic received on port ID 1 to port ID 0:
> > >
> > > flow create 1 ingress transfer pattern eth / vlan / end actions
> > > port_id id 0 / end
> > >
> > > - Adding a VLAN header to IPv6 traffic received on port ID 1 and directing
> > > it to port ID 0:
> > >
> > > flow create 1 ingress transfer pattern eth / ipv6 / end actions
> > > of_push_vlan ethertype 0x8100 / of_set_vlan_vid / port_id id 0 / end
> > >
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
> <snip>
> > > @@ -681,6 +772,84 @@ mlx5_nl_flow_transpose(void *buf,
> > > mnl_attr_nest_end(buf, act_index);
> > > ++action;
> > > break;
> > > + case ACTION_OF_POP_VLAN:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_POP_VLAN)
> > > + goto trans;
> > > + conf.of_push_vlan = NULL;
> > > + i = TCA_VLAN_ACT_POP;
> > > + goto action_of_vlan;
> > > + case ACTION_OF_PUSH_VLAN:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN)
> > > + goto trans;
> > > + conf.of_push_vlan = action->conf;
> > > + i = TCA_VLAN_ACT_PUSH;
> > > + goto action_of_vlan;
> > > + case ACTION_OF_SET_VLAN_VID:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID)
> > > + goto trans;
> > > + conf.of_set_vlan_vid = action->conf;
> > > + if (na_vlan_id)
> > > + goto override_na_vlan_id;
> > > + i = TCA_VLAN_ACT_MODIFY;
> > > + goto action_of_vlan;
> > > + case ACTION_OF_SET_VLAN_PCP:
> > > + if (action->type != RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP)
> > > + goto trans;
> > > + conf.of_set_vlan_pcp = action->conf;
> > > + if (na_vlan_priority)
> > > + goto override_na_vlan_priority;
> > > + i = TCA_VLAN_ACT_MODIFY;
> > > + goto action_of_vlan;
> > > +action_of_vlan:
> > > + act_index =
> > > + mnl_attr_nest_start_check(buf, size, act_index_cur++);
> > > + if (!act_index ||
> > > + !mnl_attr_put_strz_check(buf, size, TCA_ACT_KIND, "vlan"))
> > > + goto error_nobufs;
> > > + act = mnl_attr_nest_start_check(buf, size, TCA_ACT_OPTIONS);
> > > + if (!act)
> > > + goto error_nobufs;
> > > + if (!mnl_attr_put_check(buf, size, TCA_VLAN_PARMS,
> > > + sizeof(struct tc_vlan),
> > > + &(struct tc_vlan){
> > > + .action = TC_ACT_PIPE,
> > > + .v_action = i,
> > > + }))
> > > + goto error_nobufs;
> > > + if (i == TCA_VLAN_ACT_POP) {
> > > + mnl_attr_nest_end(buf, act);
> > > + ++action;
> > > + break;
> > > + }
> > > + if (i == TCA_VLAN_ACT_PUSH &&
> > > + !mnl_attr_put_u16_check(buf, size,
> > > + TCA_VLAN_PUSH_VLAN_PROTOCOL,
> > > + conf.of_push_vlan->ethertype))
> > > + goto error_nobufs;
> > > + na_vlan_id = mnl_nlmsg_get_payload_tail(buf);
> > > + if (!mnl_attr_put_u16_check(buf, size, TCA_VLAN_PAD, 0))
> > > + goto error_nobufs;
> > > + na_vlan_priority = mnl_nlmsg_get_payload_tail(buf);
> > > + if (!mnl_attr_put_u8_check(buf, size, TCA_VLAN_PAD, 0))
> > > + goto error_nobufs;
> > > + mnl_attr_nest_end(buf, act);
> > > + mnl_attr_nest_end(buf, act_index);
> > > + if (action->type == RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID) {
> > > +override_na_vlan_id:
> > > + na_vlan_id->nla_type = TCA_VLAN_PUSH_VLAN_ID;
> > > + *(uint16_t *)mnl_attr_get_payload(na_vlan_id) =
> > > + rte_be_to_cpu_16
> > > + (conf.of_set_vlan_vid->vlan_vid);
> > > + } else if (action->type ==
> > > + RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP) {
> > > +override_na_vlan_priority:
> > > + na_vlan_priority->nla_type =
> > > + TCA_VLAN_PUSH_VLAN_PRIORITY;
> > > + *(uint8_t *)mnl_attr_get_payload(na_vlan_priority) =
> > > + conf.of_set_vlan_pcp->vlan_pcp;
> > > + }
> > > + ++action;
> > > + break;
> >
> > I'm wondering if there's no need to check the existence of VLAN in pattern
> > when
> > having VLAN modification actions. For example, if flow has POP_VLAN action,
> > its
> > pattern has to have VLAN item, doesn't it?
>
> Not necessarily. For instance there is no need to explicitly match VLAN
> traffic if you somehow guarantee that only VLAN traffic goes through,
> e.g. in case peer is configured to always push a VLAN header regardless,
> requesting explicit match in this sense can be thought as an unnecessary
> limitation.
>
> I agree this check would have been mandatory if this check wasn't performed
> elsewhere, as discussed below:
>From user's perspective, it may not be necessary to specify VLAN in the pattern
as specifying POP_VLAN action implies that. But from device/PMD perspective,
there could be two options, a) complain about the violation or b) silently add
VLAN pattern to not cause unexpected behavior in the device.
> > Even though kernel driver has such
> > validation checks, mlx5_flow_validate() can't validate it.
>
> Yes, note this is consistent with the rest of this particular implementation
> (VLAN POP is not an exception). This entire code is a somewhat generic
> rte_flow-to-TC converter which doesn't check HW capabilities at all, it
> doesn't check the private structure, type of device and so on. This role is
> left to the kernel implementation and (optionally) the caller function.
>
> The only explicit checks are performed at conversion stage; if something
> cannot be converted from rte_flow to TC, as is the case for VLAN DEI (hence
> the 0xefff mask). The rest is implicit, for instance I didn't bother to
> implement all pattern items and fields, only the bare minimum.
>
> Further, ConnectX-4 and ConnectX-5 have different capabilities. The former
> only supports offloading destination MAC matching and the drop action at the
> switch level. Depending on driver/firmware combinations, such and such
> feature may or may not be present.
>
> Checking everything in order to print nice error messages would have been
> nice, but would have required a lot of effort. Hence the decision to
> restrict the scope of this function.
I worried about a case where a flow gets success from validation call but
creation call returns an error (error from kernel). It would be a violation of
requirement of rte_flow library.
However, I agree that this implementation should have limited scope for now as
the current lib/ker implementation is quite divergent. We have two separate
paths to configure the flow and this should be unified. Good news is we'll get
to have the unified path eventually.
> > In the PRM,
> > 8.18.2.7 Packet Classification Ambiguities
> > ...
> > In addition, a flow should not match or attempt to modify (Modify Header
> > action, Pop VLAN action) non-existing fields of a packet, as defined by
> > the packet classification process.
> > ...
>
> Fortunately this code is not running on top of PRM :)
>
> This is my opinion anyway. If you think we need extra safety for (and only
> for) VLAN POP, I'll add it, please confirm.
Well, I'll leave the decision to you.
Acked-by: Yongseok Koh <ys...@mellanox.com>
Thanks
