From: Yongseok Koh <yskoh@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.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 11:49:15 -0700 [thread overview]
Message-ID: <20180712184914.GB73570@yongseok-MBP.local> (raw)
In-Reply-To: <20180712104709.GU5211@6wind.com>
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.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:
>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 <yskoh@mellanox.com>
Thanks
next prev parent reply other threads:[~2018-07-12 18:49 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
2018-07-12 18:49 ` Yongseok Koh [this message]
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=20180712184914.GB73570@yongseok-MBP.local \
--to=yskoh@mellanox.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=nelio.laranjeiro@6wind.com \
--cc=shahafs@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).