From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 4DB8F1B50E for ; Thu, 12 Jul 2018 12:47:26 +0200 (CEST) Received: by mail-wm0-f66.google.com with SMTP id 69-v6so5501488wmf.3 for ; Thu, 12 Jul 2018 03:47:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=KaGtLJ+JWcbqAFRmEnugWgjAJQyGMvVs3YNfZ6vdNqs=; b=j+kGbGnexGmimJsZjq1eMghXTT2YmM+4CMr8dlSaWaiPDb26T+EDUY50OhN8NwS5KQ Y3OVM7sUnYdJCK6AFWE94DYRgTtLllHI1t1xpZx5SKUNZj02jt7bcAbXb6/4W7u6h6sQ 25/K4B0paMCBZ/ji0sIZtz3LFqZVahCBzKk3sfiGpinpZ5Irwp0WnA+KN0vUiHv593Ag H5iR87eiyQYRQc6akhG4vIEN2U5jPVmYRKioyCMcb5WAY1Z4tOH9E14ZigAOrZ86lZW4 kKcKO0m5elztDe7DtBJeUuhUeOSgp3oL3kpYnUmOb6rK/zbYHCbXr/3BfPOycqQCbzqx IzGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=KaGtLJ+JWcbqAFRmEnugWgjAJQyGMvVs3YNfZ6vdNqs=; b=X4DgNApTPvvz0fTodlWihfHFdjiPTwZ+VI9n7Q5FQ7kSgD2xAyQFhc5p3TTjfdosPw Pnl8JrwVhcY1FJt7Fv06eFDHGu55H/X5/ww+Wk3/R9AiaWYSShXI55euoTRCUJAgZ7ka mbcbNIkJU/l5E9qu7W6CTNe2SPHmgJ15n/e1wm5JF0Qyw4TxuXaaqqbWBre9h7kMQeE2 V4pG0of9BG/Fjhe+4be9E8QXcAHHYYm+xJ02/QsWSIlvEieAka+aONKdMrV8osSMhXSS 3/2bfuS8pQ6p7bHXYMaqTeE1lzVXjceGU+PyrEajpQTAXoDiVIRYyNWjsLvNuOUVqOPI Y2fg== X-Gm-Message-State: AOUpUlFbQX6RMWqnOPvQzB8a0Iyh5D8ASwK6Ent0n+/CZVfg5jpLyhLQ WU4bf9pj0NcI2SyC552URmTQ7w== X-Google-Smtp-Source: AAOMgpc7T6Iiyue4Rxk1clNIVisXBC4wEvngIduYK/KsTjn8DGoxGQ/dpxhq1QWSVuv9Zj8ni4XZZQ== X-Received: by 2002:a1c:b143:: with SMTP id a64-v6mr1130980wmf.114.1531392446099; Thu, 12 Jul 2018 03:47:26 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id f190-v6sm8440367wmd.0.2018.07.12.03.47.25 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 12 Jul 2018 03:47:25 -0700 (PDT) Date: Thu, 12 Jul 2018 12:47:09 +0200 From: Adrien Mazarguil To: Yongseok Koh Cc: Shahaf Shuler , Nelio Laranjeiro , dev@dpdk.org Message-ID: <20180712104709.GU5211@6wind.com> References: <20180627173355.4718-1-adrien.mazarguil@6wind.com> <20180627173355.4718-6-adrien.mazarguil@6wind.com> <20180712011024.GG69686@yongseok-MBP.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180712011024.GG69686@yongseok-MBP.local> Subject: Re: [dpdk-dev] [PATCH 5/6] net/mlx5: add VLAN item and actions to switch flow rules X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Jul 2018 10:47:26 -0000 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 > > @@ -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