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 9D54D1C073 for ; Thu, 12 Apr 2018 16:02:35 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id x82so9798070wmg.1 for ; Thu, 12 Apr 2018 07:02:35 -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=KVPs6QhbnF00jO8AYAcRPi58ti1RaLIkIeCOlRdgtpU=; b=UL6RqS6gjTbnzX24gZsY7Blf15DhtPjoy7LMr3tzL7Way/elVC4tKIQQk28Ad0HyYg XymI6AArSYVhrwWjmrMv5GfQXwyU37nnXChaDXjpnRB7FNGVdJk1X2u0jDaKQiQkO2DQ bq1wTRw31OxxPrdL8ZD0wvEs6o3aT7WbcD1WYNYJsocRplPvSabFtcsh3SwciQFHhQcF f7sb8rrp/YVqvBdY5lL9GZOQ6PRcVXyaJks3BD8+vAk03Zm5W8DH1xDItIyRaIDbEViF +ibcPGkiPm5u25S9+lHzk125KBo7eO/Q4mFiEY8ShUwCD0HkZuaJB4Hgf9yphejy2GpO Dvlw== 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=KVPs6QhbnF00jO8AYAcRPi58ti1RaLIkIeCOlRdgtpU=; b=M1FAowKY66UZGIBN8bs5N4UaZELXKc0m20wHfhiyZbd6XSlDoBf0QLAwWzMFNZoKAY wVPxyZtOYIqm9MDZNGBNACYGykt+4HfGzQVwwpn79Ww/J5JETQ3iKpkXi/lEeoq6uBnA tcsItAmCyMg+oPDlvubfhKh0YX4842fuJuLWHcQvUCR/QmMcvIngw0999JLUXabBbtLR 8zyEhuHgE1ZhH9AtRnx9cvIGoBjVjxHTr4JVlnhszN43cgSeWt/bWZf/3UXhO0MC6grk /F6onitvkkXh/L7j1YmUiaux1pzrjN87FbiYznhN+Tgwr4rLUCBthivuAUFbdEzItcD8 nlWg== X-Gm-Message-State: ALQs6tBWite+NQOi857T15PBR+QCJdLYtmNSOA9nR0W+UDh+v3/IzGn6 y3retw8UWPs08Uqi/2PKsLax X-Google-Smtp-Source: AIpwx4+SoGKuduzimZ+hb1ilcuwOwvzJ0BIGTowgZFKsUzc2AIzfdofzs/NwsxtTI1GZqoLwlyjvhA== X-Received: by 10.28.54.8 with SMTP id d8mr795092wma.145.1523541755310; Thu, 12 Apr 2018 07:02:35 -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 p128sm5370518wmd.45.2018.04.12.07.02.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 12 Apr 2018 07:02:34 -0700 (PDT) Date: Thu, 12 Apr 2018 16:02:56 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: "Xueming(Steven) Li" Cc: Shahaf Shuler , "dev@dpdk.org" Message-ID: <20180412140256.q7zskb7ouqlvbt2p@laranjeiro-vm.dev.6wind.com> References: <20180410133415.189905-1-xuemingl@mellanox.com> <20180410133415.189905-2-xuemingl@mellanox.com> <20180410144130.6byrrolegsimzyu3@laranjeiro-vm.dev.6wind.com> <20180412090916.6ibyognm554hmy6b@laranjeiro-vm.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 01/15] net/mlx5: support 16 hardware priorities 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 Apr 2018 14:02:35 -0000 On Thu, Apr 12, 2018 at 01:43:04PM +0000, Xueming(Steven) Li wrote: > > > > -----Original Message----- > > From: Nélio Laranjeiro > > Sent: Thursday, April 12, 2018 5:09 PM > > To: Xueming(Steven) Li > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v2 01/15] net/mlx5: support 16 hardware priorities > > > > On Tue, Apr 10, 2018 at 03:22:46PM +0000, Xueming(Steven) Li wrote: > > > Hi Nelio, > > > > > > > -----Original Message----- > > > > From: Nélio Laranjeiro > > > > Sent: Tuesday, April 10, 2018 10:42 PM > > > > To: Xueming(Steven) Li > > > > Cc: Shahaf Shuler ; dev@dpdk.org > > > > Subject: Re: [PATCH v2 01/15] net/mlx5: support 16 hardware > > > > priorities > > > > > > > > On Tue, Apr 10, 2018 at 09:34:01PM +0800, Xueming Li wrote: > > > > > Adjust flow priority mapping to adapt new hardware 16 verb flow > > > > > priorites support: > > > > > 0-3: RTE FLOW tunnel rule > > > > > 4-7: RTE FLOW non-tunnel rule > > > > > 8-15: PMD control flow > > > > > > > > This commit log is inducing people in error, this amount of priority > > > > depends on the Mellanox OFED installed, it is not available on > > > > upstream Linux kernel yet nor in the current Mellanox OFED GA. > > > > > > > > What happens when those amount of priority are not available, is it > > > > removing a functionality? Will it collide with other flows? > > > > > > If 16 priorities not available, simply behavior as 8 priorities. > > > > It is not described in the commit log, please add it. > > > > > > > Signed-off-by: Xueming Li > > > > > > > }, > > > > > [HASH_RXQ_ETH] = { > > > > > .hash_fields = 0, > > > > > .dpdk_rss_hf = 0, > > > > > - .flow_priority = 3, > > > > > + .flow_priority = 2, > > > > > }, > > > > > }; > > > > > > > > If the amount of priorities remains 8, you are removing the priority > > > > for the tunnel flows introduced by commit 749365717f5c ("net/mlx5: > > > > change tunnel flow priority") > > > > > > > > Please keep this functionality when this patch fails to get the > > > > expected > > > > 16 Verbs priorities. > > > > > > These priority shift are different in 16 priorities scenario, I > > > changed it to calculation. In function mlx5_flow_priorities_detect(), > > > priority shift will be 1 if 8 priorities, 4 in case of 16 priorities. > > > Please refer to changes in function mlx5_flow_update_priority() as well. > > > > Please light my lamp, I don't see it... > > Sorry, please refer to priv->config.flow_priority_shift. > > > > > > > > > > static void > > > > > -mlx5_flow_update_priority(struct mlx5_flow_parse *parser, > > > > > +mlx5_flow_update_priority(struct rte_eth_dev *dev, > > > > > + struct mlx5_flow_parse *parser, > > > > > const struct rte_flow_attr *attr) { > > > > > + struct priv *priv = dev->data->dev_private; > > > > > unsigned int i; > > > > > + uint16_t priority; > > > > > > > > > > + if (priv->config.flow_priority_shift == 1) > > > > > + priority = attr->priority * MLX5_VERBS_FLOW_PRIO_4; > > > > > + else > > > > > + priority = attr->priority * MLX5_VERBS_FLOW_PRIO_8; > > > > > + if (!parser->inner) > > > > > + priority += priv->config.flow_priority_shift; > > Here, if non-tunnel flow, lower(increase) 1 for 8 priorities, lower 4 otherwise. > I'll append a comment here. Thanks, I totally missed this one. > > > > > diff --git a/drivers/net/mlx5/mlx5_trigger.c > > > > > b/drivers/net/mlx5/mlx5_trigger.c index 6bb4ffb14..d80a2e688 > > > > > 100644 > > > > > --- a/drivers/net/mlx5/mlx5_trigger.c > > > > > +++ b/drivers/net/mlx5/mlx5_trigger.c > > > > > @@ -148,12 +148,6 @@ mlx5_dev_start(struct rte_eth_dev *dev) > > > > > int ret; > > > > > > > > > > dev->data->dev_started = 1; > > > > > - ret = mlx5_flow_create_drop_queue(dev); > > > > > - if (ret) { > > > > > - DRV_LOG(ERR, "port %u drop queue allocation failed: %s", > > > > > - dev->data->port_id, strerror(rte_errno)); > > > > > - goto error; > > > > > - } > > > > > DRV_LOG(DEBUG, "port %u allocating and configuring hash Rx > > queues", > > > > > dev->data->port_id); > > > > > rte_mempool_walk(mlx5_mp2mr_iter, priv); @@ -202,7 +196,6 @@ > > > > > mlx5_dev_start(struct rte_eth_dev *dev) > > > > > mlx5_traffic_disable(dev); > > > > > mlx5_txq_stop(dev); > > > > > mlx5_rxq_stop(dev); > > > > > - mlx5_flow_delete_drop_queue(dev); > > > > > rte_errno = ret; /* Restore rte_errno. */ > > > > > return -rte_errno; > > > > > } > > > > > @@ -237,7 +230,6 @@ mlx5_dev_stop(struct rte_eth_dev *dev) > > > > > mlx5_rxq_stop(dev); > > > > > for (mr = LIST_FIRST(&priv->mr); mr; mr = LIST_FIRST(&priv- > > >mr)) > > > > > mlx5_mr_release(mr); > > > > > - mlx5_flow_delete_drop_queue(dev); > > > > > } > > > > > > > > > > /** > > > > > -- > > > > > 2.13.3 > > > > > > > > I have few concerns on this, mlx5_pci_probe() will also probe any > > > > under layer verbs device, and in a near future the representors > > > > associated to a VF. > > > > Making such detection should only be done once by the PF, I also > > > > wander if it is possible to make such drop action in a representor > > > > directly using Verbs. > > > > > > Then there should be some work to disable flows in representors? that > > > supposed to cover this. > > > > The code raising another Verbs device is already present and since the > > first entrance of this PMD in the DPDK tree, you must respect the code > > already present. > > This request is not related directly to a new feature but to an existing > > one, the representors being just an example. > > > > This detection should be only done once and not for each of them. > > Could you please elaborate on "under layer verbs device" and how to judge > dependency to PF, is there a probe order between them? The place where you are adding the mlx5_flow_create_drop_queue() in mlx5_pci_probe() is probing any device returned by the verbs ibv_get_device_list(). This code is also present in mlx4 where for a single PCI id there are 2 physical ports. If the NIC handle several ibvdev (Verbs devices) it will create a new rte_eth_dev for each one. > BTW, VF representor code seems exists in 17.11, not upstream. > > > > > > > Another concern is, this patch will be reverted in some time when > > > > those > > > > 16 priority will be always available. It will be easier to remove > > > > this detection function than searching for all those modifications. > > > > > > > > I would suggest to have a standalone mlx5_flow_priorities_detect() > > > > which creates and deletes all resources needed for this detection. > > > > > > There is an upcoming new feature to support priorities more than 16, > > > auto detection will be kept IMHO. > > > > Until the final values of priorities will be backported to all kernels we > > support. You don't see far enough in the future. > > > > > Besides, there will be a bundle of resource creation and removal in > > > this standalone function, I'm not sure it valuable to duplicate them, > > > please refer to function mlx5_flow_create_drop_queue(). > > > > You misunderstood, I am not asking you to not use the default drop queues > > but instead of making an rte_flow attributes, items and actions to make > > directly the Verbs specification on the stack. It will be faster than > > making a bunch of conversions (relying on malloc) from rte_flow to Verbs > > whereas you know exactly what it needs i.e. 1 spec. > > Sorry, still confused, mlx5_flow_priorities_detect() invokes ibv_destroy_flow(), > not rte_flow stuff, no malloc at all. BTW, mlx5 flow api bypass verb flow in > offline mode, we can't use it to create flows at such stage. Sorry I was the one confused. Priority detection is Ok. After reading this, I'll suggest to use a boolean in the mlx5_pci_probe() to only make this detection once, the underlying verbs devices will inherit from such knowledge and adjust their own shift accordingly. What do you think? -- Nélio Laranjeiro 6WIND