[dpdk-dev] [PATCH 5/6] net/mlx5: add VLAN item and actions to switch flow rules

[Adrien Mazarguil]

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.mazarguil at 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:

> 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.

> 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.

-- 
Adrien Mazarguil
6WIND