From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id D39781BE38 for ; Wed, 4 Jul 2018 14:03:07 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id p11-v6so5438376wmc.4 for ; Wed, 04 Jul 2018 05:03:07 -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:content-transfer-encoding:in-reply-to:openpgp :user-agent; bh=+cqbHc4rDoioQ/3Q8E2z/q0WWkEE4WhejwLaTz/5HWo=; b=GGTug0KlR3MDhxV7nWtTngY747+MKHDoLIG4nXbArsw4ejnBOyBZ2v/sHSpOnKryCR n029zmqFuJGQuTzTmMUgUWQf19mPJBdn9tTAcpO7KGHekz1yKUfvlsPOcXWsm0fvTu5p ASg6vzVtdNRzR2HV9MbZDObDA6Oely6Wg68TlI3OiNYnZrjNvF8YOt5BhFdtdurY2Bdb cUzy8czLT1F8A2g8TpQMMP/tDTqFXE9X+trSMbSRSDdTcQ5b4scTm/jOpGaZFQyFbDYl eeSHFd6s6PXiIH4tXRKMwE0Qv0mcSDviHrBd69Gwir82GShTM7GaSoldao5E615H45WF CpFQ== 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:content-transfer-encoding :in-reply-to:openpgp:user-agent; bh=+cqbHc4rDoioQ/3Q8E2z/q0WWkEE4WhejwLaTz/5HWo=; b=BPmNaqXZT14b17ShifckYseLmOUNfd6dotp5TrPDTxEPOGtPvQ+MvKAxplScc6T9HO D7xBVIQaAqdc35fO08mbhIVndXzpHSWn4hd17KPqq12WTtiv/odmweuwkMs3YH6fL8BN MAFgyKbphQYHBrXGXPdy1fNqhhHvQQexBxXTQYI6i0H4zsOIB9q7ltfVuD6I3LdPjjkn xWhVo5kDbazXjnB0cCtZEPgvpYThkJxxYsB0c+TxMXXFlPXZSIlhZF4OYCJsfStYpdnz icx5qdXv3hQfBPxffOOHsZ5gt0FXjbugp1IMkc8mjtPsQIsmOv+HPXPbNMd0qbXS8RbW ZnvQ== X-Gm-Message-State: APt69E2OPE1PRX4JQiZHqL+KsyjMQBQCwsc5RE4Vry0JC4IDdsEmeTCk VL1SMC1YEQyBngHbHgFuYsKX X-Google-Smtp-Source: AAOMgpfMgbix2BZZgtt7pQz3mP95G4AVS5MjIocJ7iU2TeRMo5cUnQPIfsU4V1E+ujNSwnLvoi6olA== X-Received: by 2002:a1c:6902:: with SMTP id e2-v6mr1526656wmc.95.1530705787387; Wed, 04 Jul 2018 05:03:07 -0700 (PDT) Received: from laranjeiro-vm.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 189-v6sm7894215wmy.25.2018.07.04.05.03.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 04 Jul 2018 05:03:06 -0700 (PDT) Date: Wed, 4 Jul 2018 14:03:03 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Yongseok Koh Cc: dev@dpdk.org, Adrien Mazarguil Message-ID: <20180704120303.xsu7aiyu3f5b4i5x@laranjeiro-vm.dev.6wind.com> References: <39544c46ba7237c339e89f10480c3e24a3810754.1530111623.git.nelio.laranjeiro@6wind.com> <20180703235620.GE41721@yongseok-MBP.local> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180703235620.GE41721@yongseok-MBP.local> OpenPGP: id=A0075DA8F66A5949 preference=signencrypt User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 07/20] net/mlx5: add flow VLAN item 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: Wed, 04 Jul 2018 12:03:08 -0000 On Tue, Jul 03, 2018 at 04:56:21PM -0700, Yongseok Koh wrote: > On Wed, Jun 27, 2018 at 05:07:39PM +0200, Nelio Laranjeiro wrote: > > Signed-off-by: Nelio Laranjeiro > > --- > > drivers/net/mlx5/mlx5_flow.c | 123 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 123 insertions(+) > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c > > index 6593eafa0..6a576ddd9 100644 > > --- a/drivers/net/mlx5/mlx5_flow.c > > +++ b/drivers/net/mlx5/mlx5_flow.c > > @@ -420,6 +420,126 @@ mlx5_flow_item_eth(const struct rte_flow_item *item, struct rte_flow *flow, > > return size; > > } > > > > +/** > > + * Update the VLAN tag in the Ethernet spec. > > + * > > + * @param attr[in, out] > > + * Pointer to Verbs attributes structure. > > + * @param eth[in] > > + * Verbs structure containing the VLAN information to copy. > > + */ > > +static void > > +mlx5_flow_item_vlan_update(struct ibv_flow_attr *attr, > > + struct ibv_flow_spec_eth *eth) > > +{ > > + unsigned int i; > > + enum ibv_flow_spec_type search = IBV_FLOW_SPEC_ETH; > > + struct ibv_spec_header *hdr = (struct ibv_spec_header *) > > + ((uint8_t *)attr + sizeof(struct ibv_flow_attr)); > > + > > + for (i = 0; i != attr->num_of_specs; ++i) { > > + if (hdr->type == search) { > > + struct ibv_flow_spec_eth *e = > > + (struct ibv_flow_spec_eth *)hdr; > > + > > + e->val.vlan_tag = eth->val.vlan_tag; > > + e->mask.vlan_tag = eth->mask.vlan_tag; > > You'll have to overwrite ether_type as well, otherwise the e->ether_type would > have TPID. Right, > > + break; > > + } > > + hdr = (struct ibv_spec_header *)((uint8_t *)hdr + hdr->size); > > + } > > +} > > + > > +/** > > + * Validate VLAN layer and possibly create/modify the Verbs specification. > > + * > > + * @param item[in] > > + * Item specification. > > + * @param flow[in, out] > > + * Pointer to flow structure. > > + * @param flow_size[in] > > + * Size of the buffer to store the specification. > > + * @param error > > + * Pointer to error structure. > > + * > > + * @return > > + * size in bytes necessary for the conversion, a negative errno value > > + * otherwise and rte_errno is set. > > + */ > > +static int > > +mlx5_flow_item_vlan(const struct rte_flow_item *item, struct rte_flow *flow, > > + const size_t flow_size, struct rte_flow_error *error) > > +{ > > + const struct rte_flow_item_vlan *spec = item->spec; > > + const struct rte_flow_item_vlan *mask = item->mask; > > + const struct rte_flow_item_vlan nic_mask = { > > + .tci = RTE_BE16(0x0fff), > > + }; > > + unsigned int size = sizeof(struct ibv_flow_spec_eth); > > + struct ibv_flow_spec_eth eth = { > > + .type = IBV_FLOW_SPEC_ETH, > > + .size = size, > > + }; > > + int ret; > > + const uint32_t lm = MLX5_FLOW_LAYER_OUTER_L3 | > > + MLX5_FLOW_LAYER_OUTER_L4; > > What does 'lm' stand for? Better to use l34m? l34m seems better, > > + const uint32_t vlanm = MLX5_FLOW_LAYER_OUTER_VLAN; > > + const uint32_t l2m = MLX5_FLOW_LAYER_OUTER_L2; > > + > > + if (flow->layers & vlanm) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "L2 layers already configured"); > > L2? Not VLAN? Indeed it is VLAN. > > + else if ((flow->layers & lm) != 0) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "L2 layer cannot follow L3/L4 layer"); > > + if (!mask) > > + mask = &rte_flow_item_vlan_mask; > > + ret = mlx5_flow_item_validate(item, (const uint8_t *)mask, > > + (const uint8_t *)&nic_mask, > > + sizeof(struct rte_flow_item_vlan), error); > > + if (ret) > > + return ret; > > + if (spec) { > > + eth.val.vlan_tag = spec->tci; > > + eth.mask.vlan_tag = mask->tci; > > + eth.val.vlan_tag &= eth.mask.vlan_tag; > > + eth.val.ether_type = spec->inner_type; > > + eth.mask.ether_type = mask->inner_type; > > + eth.val.ether_type &= eth.mask.ether_type; > > + } > > + /* > > + * From verbs perspective an empty VLAN is equivalent > > + * to a packet without VLAN layer. > > + */ > > + if (!eth.mask.vlan_tag) > > + return rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ITEM_SPEC, > > + item->spec, > > + "VLAN cannot be empty"); > > + /* Outer TPID cannot be matched. */ > > + if (eth.mask.ether_type) > > + return rte_flow_error_set(error, ENOTSUP, > > + RTE_FLOW_ERROR_TYPE_ITEM_SPEC, > > + item->spec, > > + "VLAN TPID matching is not" > > + " supported"); > > Not sure 100% but I don't think ether_type means TPID but the inner packet type > coming after the VLAN ID. E.g. > > /dmac/smac/0x8100/TCI/0x0800/ipv4... > ^ > | I remember to have faced such issue several months ago, from what I understood it is configurable but there is no way in Verbs to make it. Currently (after testing) it matches the above logic, I will remove this check. > > + if (!(flow->layers & l2m)) { > > + if (size <= flow_size) > > + mlx5_flow_spec_verbs_add(flow, ð, size); > > + } else { > > + if (flow->verbs.attr) > > + mlx5_flow_item_vlan_update(flow->verbs.attr, ð); > > + size = 0; /**< Only an update is done in eth specification. */ > > Any specific reason to use doxygen style comment only here? No reason, I'll remove it. Thanks, -- Nélio Laranjeiro 6WIND