From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f171.google.com (mail-wr0-f171.google.com [209.85.128.171]) by dpdk.org (Postfix) with ESMTP id C5FE41BB4C for ; Fri, 13 Apr 2018 10:37:25 +0200 (CEST) Received: by mail-wr0-f171.google.com with SMTP id s18so7503161wrg.9 for ; Fri, 13 Apr 2018 01:37:25 -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 :user-agent; bh=HQg0xhcnfHJvzinxwRiSDFq4zbnEWu7Gc+T4vb+/wUc=; b=LpjvKaNVX/MFO9JiwG5/OfHgAVN4Fxo8DT/Uw6tesa0HKyPCkWeM7w3Zf9XDj81/6K mDNOu30PlIPXSaEmC+QCyDbxa+1815HiIwGfKJxyR69Ow5D7uWClB8Gu6scQyZkuX31I sCpjVP8ZugMZ7EqdBKNYMXZbS7HiMGfjg4iriI04F2wFnNUwCCElL3EslAIFJttP+W7H 8czDQPrp8NKkYCvoJrMKu1HfQStJumOxepuPZ2QlxkEoR2b33yZhtFsZk5IeYlZuuilq pF5OKqwgWwB871/HCRw00wbU4VWFTbD71W+lP9O6eGgHeayHN2gqOr87HY5s3yCP3CP4 9Htw== 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:user-agent; bh=HQg0xhcnfHJvzinxwRiSDFq4zbnEWu7Gc+T4vb+/wUc=; b=axRcS6xRVP3OSAIxBjBHDNfpJ0UaFc3SECGJM2zmINdQ9sjSriMghe6a6SuUedamPe CpIDYaZELIh5vs5tRIcKrMuYcvIFz1FyLAmPqUr+1kEgr4qd4j4R0WKTHNFKWXWU9Est 1zPjG7BdJA39k0LjVh4cT/27IT9W2TZXjxbHdBvP/jwhT5yc/lJryuZdm5Zn3p+qWFBj jBQvvWmYLFBkHgc/av6BuWNGKaQf+o2WsVViJaaW/3zJY8fOOp+WUrDPMOYYpUit+jr4 kwQ3jhwUbnO6jchqbPcmEdbdGEmw5qq8G/0t5lV0QUs8KMKc1r840DgSIvdHD6Vp9Et7 atQw== X-Gm-Message-State: ALQs6tBDSYR3u/ZuS+nrvhqAJBkkJLKRqf6dZAN656Bc9WrKCkVb1ws9 /lKf0lEipA89iGgWS6ActGxloPcC0Q== X-Google-Smtp-Source: AIpwx4+Oo7IdR3J9o1BVqHp3Du4j3eyVjrwoaR2k118iCWJ6Nbe8zBAXluTku5XFv2hYYVOShwaEbA== X-Received: by 10.223.199.3 with SMTP id k3mr2996794wrg.26.1523608645387; Fri, 13 Apr 2018 01:37:25 -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 6sm1543794wmf.31.2018.04.13.01.37.24 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 13 Apr 2018 01:37:24 -0700 (PDT) Date: Fri, 13 Apr 2018 10:37:48 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: "Xueming(Steven) Li" Cc: Shahaf Shuler , "dev@dpdk.org" Message-ID: <20180413083748.nshoium4m5dpysvb@laranjeiro-vm.dev.6wind.com> References: <20180410133415.189905-1-xuemingl@mellanox.com> <20180410133415.189905-5-xuemingl@mellanox.com> <20180410151711.zlxxkzycknrtlwhr@laranjeiro-vm.dev.6wind.com> <20180412095058.3vcynmscyvc6wl7e@laranjeiro-vm.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 04/15] net/mlx5: support Rx tunnel type identification 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: Fri, 13 Apr 2018 08:37:26 -0000 On Thu, Apr 12, 2018 at 02:27:45PM +0000, Xueming(Steven) Li wrote: > > -----Original Message----- > > From: Nélio Laranjeiro > > Sent: Thursday, April 12, 2018 5:51 PM > > To: Xueming(Steven) Li > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v2 04/15] net/mlx5: support Rx tunnel type > > identification > > > > On Wed, Apr 11, 2018 at 08:11:50AM +0000, Xueming(Steven) Li wrote: > > > Hi Nelio, > > > > > > > -----Original Message----- > > > > From: Nélio Laranjeiro > > > > Sent: Tuesday, April 10, 2018 11:17 PM > > > > To: Xueming(Steven) Li > > > > Cc: Shahaf Shuler ; dev@dpdk.org > > > > Subject: Re: [PATCH v2 04/15] net/mlx5: support Rx tunnel type > > > > identification > > > > > > > > On Tue, Apr 10, 2018 at 09:34:04PM +0800, Xueming Li wrote: > > > > > This patch introduced tunnel type identification based on flow rules. > > > > > If flows of multiple tunnel types built on same queue, > > > > > RTE_PTYPE_TUNNEL_MASK will be returned, bits in flow mark could be > > > > > used as tunnel type identifier. > > > > > > > > I don't see anywhere in this patch where the bits are reserved to > > > > identify a flow, nor values which can help to identify it. > > > > > > > > Is this missing? > > > > > > > > Anyway we have already very few bits in the mark making it difficult > > > > to be used by the user, reserving again some to may lead to remove > > > > the mark support from the flows. > > > > > > Not all users will use multiple tunnel types, this is not included in > > > this patch set and left to user decision. I'll update comments to make > > this clear. > > > > Thanks, > > > > > > > Signed-off-by: Xueming Li > > > > > > > /** > > > > > + * RXQ update after flow rule creation. > > > > > + * > > > > > + * @param dev > > > > > + * Pointer to Ethernet device. > > > > > + * @param flow > > > > > + * Pointer to the flow rule. > > > > > + */ > > > > > +static void > > > > > +mlx5_flow_create_update_rxqs(struct rte_eth_dev *dev, struct > > > > > +rte_flow > > > > > +*flow) { > > > > > + struct priv *priv = dev->data->dev_private; > > > > > + unsigned int i; > > > > > + > > > > > + if (!dev->data->dev_started) > > > > > + return; > > > > > + for (i = 0; i != flow->rss_conf.queue_num; ++i) { > > > > > + struct mlx5_rxq_data *rxq_data = (*priv->rxqs) > > > > > + [(*flow->queues)[i]]; > > > > > + struct mlx5_rxq_ctrl *rxq_ctrl = > > > > > + container_of(rxq_data, struct mlx5_rxq_ctrl, rxq); > > > > > + uint8_t tunnel = PTYPE_IDX(flow->tunnel); > > > > > + > > > > > + rxq_data->mark |= flow->mark; > > > > > + if (!tunnel) > > > > > + continue; > > > > > + rxq_ctrl->tunnel_types[tunnel] += 1; > > > > > > > > I don't understand why you need such array, the NIC is unable to > > > > return the tunnel type has it returns only one bit saying tunnel. > > > > Why don't it store in the priv structure the current configured tunnel? > > > > > > This array is used to count tunnel types bound to queue, if only one > > > tunnel type, ptype will report that tunnel type, TUNNEL MASK(max > > > value) will be returned if multiple types bound to a queue. > > > > > > Flow rss action specifies queues that binding to tunnel, thus we can't > > > assume all queues have same tunnel types, so this is a per queue > > structure. > > > > There is something I am missing here, how in the dataplane the PMD can > > understand from 1 bit which kind of tunnel the packet is matching? > > The code under this line is answer, let me post here: > if (rxq_data->tunnel != flow->tunnel) > rxq_data->tunnel = rxq_data->tunnel ? > RTE_PTYPE_TUNNEL_MASK : > flow->tunnel; > If no tunnel type associated to rxq, use tunnel type from flow. > If a new tunnel type from flow, use RTE_PTYPE_TUNNEL_MASK. >>From my understanding, when in the same queue there are several tunnel offloads, the mbuf ptype will contains RTE_PTYPE_TUNNEL_MASK: @@ -1601,7 +1605,7 @@ rxq_cq_to_pkt_type(volatile struct mlx5_cqe *cqe) * bit[7] = outer_l3_type */ idx = ((pinfo & 0x3) << 6) | ((ptype & 0xfc00) >> 10); - return mlx5_ptype_table[idx]; + return mlx5_ptype_table[idx] | rxq->tunnel * !!(idx & (1 << 6)); } Used by Rx burst functions, /* Update packet information. */ pkt->packet_type = rxq_cq_to_pkt_type(cqe); Is this correct? There is another strange point here, + [PTYPE_IDX(RTE_PTYPE_TUNNEL_VXLAN)] = RTE_PTYPE_TUNNEL_VXLAN | + RTE_PTYPE_L4_UDP, According to the RFC 7348 [1] having a VXLAN with an outer IPv6 is possible. How do you handle it? > > > > > > > @@ -2334,9 +2414,9 @@ mlx5_flow_stop(struct rte_eth_dev *dev, > > > > > struct mlx5_flows *list) { > > > > > struct priv *priv = dev->data->dev_private; > > > > > struct rte_flow *flow; > > > > > + unsigned int i; > > > > > > > > > > TAILQ_FOREACH_REVERSE(flow, list, mlx5_flows, next) { > > > > > - unsigned int i; > > > > > struct mlx5_ind_table_ibv *ind_tbl = NULL; > > > > > > > > > > if (flow->drop) { > > > > > @@ -2382,6 +2462,16 @@ mlx5_flow_stop(struct rte_eth_dev *dev, > > > > > struct > > > > mlx5_flows *list) > > > > > DRV_LOG(DEBUG, "port %u flow %p removed", dev->data- > > >port_id, > > > > > (void *)flow); > > > > > } > > > > > + /* Cleanup Rx queue tunnel info. */ > > > > > + for (i = 0; i != priv->rxqs_n; ++i) { > > > > > + struct mlx5_rxq_data *q = (*priv->rxqs)[i]; > > > > > + struct mlx5_rxq_ctrl *rxq_ctrl = > > > > > + container_of(q, struct mlx5_rxq_ctrl, rxq); > > > > > + > > > > > + memset((void *)rxq_ctrl->tunnel_types, 0, > > > > > + sizeof(rxq_ctrl->tunnel_types)); > > > > > + q->tunnel = 0; > > > > > + } > > > > > } > > > > > > > > This hunk does not handle the fact the Rx queue array may have some > > > > holes i.e. the application is allowed to ask for 10 queues and only > > > > initialise some. In such situation this code will segfault. > > > > > > In other words, "q" could be NULL, correct? I'll add check for this. > > > > Correct. > > > > > BTW, there should be an action item to add such check in rss/queue flow > > creation. > > > > As it is the responsibility of the application/user to make rule according > > to what it has configured, it has not been added. It can still be added, > > but it cannot be considered as a fix. > > > > > > It should only memset the Rx queues making part of the flow not the > > others. > > > > > > Clean this(decrease tunnel_types counter of each queue) from each flow > > > would be time consuming. > > > > Considering flows are already relying on syscall to communicate with the > > kernel, the extra cycles consumption to only clear the queues making part > > of this flow is neglectable. > > > > By the way in the same function the mark is cleared only for the queues > > making part of the flow, the same loop can be used to clear those tunnel > > informations at the same time. > > > > > If an error happened, counter will not be cleared and such state will > > > impact tunnel type after port start again. > > > > Unless an implementation error which other kind of them do you fear to > > happen? > > Mark of rxq simply reset to 0, this field is counter, the final target is to > clear field value, so my code should be straight forward and error free 😊 > > From a quick look, this function could be much simple that what it is today: > 1. clean verb flow and hrex where possible, despite of flow type. > 2. clean rxq state: mark and tunnel_types. Ok. Thanks, [1] https://dpdk.org/patch/37965 -- Nélio Laranjeiro 6WIND