From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Yongseok Koh <yskoh@mellanox.com>
Cc: Shahaf Shuler <shahafs@mellanox.com>,
Nelio Laranjeiro <nelio.laranjeiro@6wind.com>,
dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 5/6] net/mlx5: add VLAN item and actions to switch flow rules
Date: Thu, 12 Jul 2018 12:47:09 +0200 [thread overview]
Message-ID: <20180712104709.GU5211@6wind.com> (raw)
In-Reply-To: <20180712011024.GG69686@yongseok-MBP.local>
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@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
next prev parent reply other threads:[~2018-07-12 10:47 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-27 18:08 [dpdk-dev] [PATCH 0/6] net/mlx5: add support for " Adrien Mazarguil
2018-06-27 18:08 ` [dpdk-dev] [PATCH 1/6] net/mlx5: lay groundwork for switch offloads Adrien Mazarguil
2018-07-12 0:17 ` Yongseok Koh
2018-07-12 10:46 ` Adrien Mazarguil
2018-07-12 17:33 ` Yongseok Koh
2018-06-27 18:08 ` [dpdk-dev] [PATCH 2/6] net/mlx5: add framework for switch flow rules Adrien Mazarguil
2018-07-12 0:59 ` Yongseok Koh
2018-07-12 10:46 ` Adrien Mazarguil
2018-07-12 18:25 ` Yongseok Koh
2018-06-27 18:08 ` [dpdk-dev] [PATCH 3/6] net/mlx5: add fate actions to " Adrien Mazarguil
2018-07-12 1:00 ` Yongseok Koh
2018-06-27 18:08 ` [dpdk-dev] [PATCH 4/6] net/mlx5: add L2-L4 pattern items " Adrien Mazarguil
2018-07-12 1:02 ` Yongseok Koh
2018-06-27 18:08 ` [dpdk-dev] [PATCH 5/6] net/mlx5: add VLAN item and actions " Adrien Mazarguil
2018-07-12 1:10 ` Yongseok Koh
2018-07-12 10:47 ` Adrien Mazarguil [this message]
2018-07-12 18:49 ` Yongseok Koh
2018-06-27 18:08 ` [dpdk-dev] [PATCH 6/6] net/mlx5: add port ID pattern item " Adrien Mazarguil
2018-07-12 1:13 ` Yongseok Koh
2018-06-28 9:05 ` [dpdk-dev] [PATCH 0/6] net/mlx5: add support for " Nélio Laranjeiro
2018-07-13 9:40 ` [dpdk-dev] [PATCH v2 " Adrien Mazarguil
2018-07-13 9:40 ` [dpdk-dev] [PATCH v2 1/6] net/mlx5: lay groundwork for switch offloads Adrien Mazarguil
2018-07-14 1:29 ` Yongseok Koh
2018-07-23 21:40 ` Ferruh Yigit
2018-07-24 0:50 ` Stephen Hemminger
2018-07-24 4:35 ` Shahaf Shuler
2018-07-24 19:33 ` Stephen Hemminger
2018-07-13 9:40 ` [dpdk-dev] [PATCH v2 2/6] net/mlx5: add framework for switch flow rules Adrien Mazarguil
2018-07-13 9:40 ` [dpdk-dev] [PATCH v2 3/6] net/mlx5: add fate actions to " Adrien Mazarguil
2018-07-13 9:40 ` [dpdk-dev] [PATCH v2 4/6] net/mlx5: add L2-L4 pattern items " Adrien Mazarguil
2018-07-13 9:40 ` [dpdk-dev] [PATCH v2 5/6] net/mlx5: add VLAN item and actions " Adrien Mazarguil
2018-07-13 9:40 ` [dpdk-dev] [PATCH v2 6/6] net/mlx5: add port ID pattern item " Adrien Mazarguil
2018-07-22 11:21 ` [dpdk-dev] [PATCH v2 0/6] net/mlx5: add support for " Shahaf Shuler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180712104709.GU5211@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=nelio.laranjeiro@6wind.com \
--cc=shahafs@mellanox.com \
--cc=yskoh@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).