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 B01B51B7B9 for ; Thu, 12 Apr 2018 11:08:55 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id g8so9906534wmd.2 for ; Thu, 12 Apr 2018 02:08:55 -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=PPGJ44abv9nQIWQmNkL08huKfRhzgiT2wKe5gusEO4E=; b=BLmKqF5cXeaBhENeerqq3r5svHjEthpJv2A4N61no5djbHzu9ELtqMzh1CSMjjVZlL HRctYMyl8fv8P2UeGzWbdJwpGgBVcjp9AMgnLlLrShI51atDRUMcniEZ5ztT1MwUygBv gTrbETxvDmNMORarS5820kuWd/VDodR2H+86/LM6NhWg+mlu8YyKoFvZ2VtrKt8tuCWs XpMO5ptp3Ooi1cDClT1XKlZnuHp22TgERX0ql0PXPeNs6VtyfeX/KpOB8oFGGLzL5nAU 6KUN63TQTar4pPRn78aLrs8Cg0hKxtb9oYSGABLefYQajmm5oASmCYdJinsmBnrQBMOy LkpQ== 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=PPGJ44abv9nQIWQmNkL08huKfRhzgiT2wKe5gusEO4E=; b=LXL0l3uk0UT61QI8e29cW9gcnR11R7hNq3lQPjwmLyepVX/P4vlOcLUycI71kIsEml ZqrnHs0dliLRGLDyZOsjYvgW1MYnURP5i4rAMa4cePqcblZygebMTSSQGAISGCx4zOfd klAjy85dtAg7Rtg/ppXcoWcVAJUubGB385QIq4UoDn5Lz1muql93BNz4hOo2E5r/u25r ZeWYlOMn97a2b56TjoGX91kbI4zlxIDdAFHDcspVWBPTgD/UPFa78iXtLgHYblQQTb2H IBV7QEk6cBGkFHU0Z2lqcTbXv29jIF0bfwJSdcHVeD130gS688xo7N1D7s4znw4g4PKk OKhw== X-Gm-Message-State: ALQs6tDHEhM5lbhBgyGFvYWRECocDmQZ8wuQDjNfo3eS64UPrhlt00Un +TOe4p+BZavvxCOb/0eHBFRF X-Google-Smtp-Source: AIpwx49DTOm/fclpysAUrQ1sLswsAsQYEdQwDjYj63bRMtsCrT2ZdFE4MzxNf75dqYqCuhVCiuynsA== X-Received: by 10.28.1.197 with SMTP id 188mr34887wmb.49.1523524135420; Thu, 12 Apr 2018 02:08:55 -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 o63sm3095024wmb.4.2018.04.12.02.08.54 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 12 Apr 2018 02:08:54 -0700 (PDT) Date: Thu, 12 Apr 2018 11:09:16 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: "Xueming(Steven) Li" Cc: Shahaf Shuler , "dev@dpdk.org" Message-ID: <20180412090916.6ibyognm554hmy6b@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> 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 09:08:55 -0000 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... > > > 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; > > > if (parser->drop) { > > > - parser->queue[HASH_RXQ_ETH].ibv_attr->priority = > > > - attr->priority + > > > - hash_rxq_init[HASH_RXQ_ETH].flow_priority; > > > + parser->queue[HASH_RXQ_ETH].ibv_attr->priority = priority + > > > + hash_rxq_init[HASH_RXQ_ETH].flow_priority; > > > return; > > > } > > > for (i = 0; i != hash_rxq_init_n; ++i) { > > > - if (parser->queue[i].ibv_attr) { > > > - parser->queue[i].ibv_attr->priority = > > > - attr->priority + > > > - hash_rxq_init[i].flow_priority - > > > - (parser->inner ? 1 : 0); > > > - } > > > + if (!parser->queue[i].ibv_attr) > > > + continue; > > > + parser->queue[i].ibv_attr->priority = priority + > > > + hash_rxq_init[i].flow_priority; Previous code was subtracting one from the table priorities which was starting at 1. In the new code I don't see it. What I am missing? > > > } > > > } > > > > > > @@ -1087,7 +1097,7 @@ mlx5_flow_convert(struct rte_eth_dev *dev, > > > .layer = HASH_RXQ_ETH, > > > .mark_id = MLX5_FLOW_MARK_DEFAULT, > > > }; > > > - ret = mlx5_flow_convert_attributes(attr, error); > > > + ret = mlx5_flow_convert_attributes(dev, attr, error); > > > if (ret) > > > return ret; > > > ret = mlx5_flow_convert_actions(dev, actions, error, parser); @@ > > > -1158,7 +1168,7 @@ mlx5_flow_convert(struct rte_eth_dev *dev, > > > */ > > > if (!parser->drop) > > > mlx5_flow_convert_finalise(parser); > > > - mlx5_flow_update_priority(parser, attr); > > > + mlx5_flow_update_priority(dev, parser, attr); > > > exit_free: > > > /* Only verification is expected, all resources should be released. > > */ > > > if (!parser->create) { > > > @@ -2450,7 +2460,7 @@ mlx5_ctrl_flow_vlan(struct rte_eth_dev *dev, > > > struct priv *priv = dev->data->dev_private; > > > const struct rte_flow_attr attr = { > > > .ingress = 1, > > > - .priority = MLX5_CTRL_FLOW_PRIORITY, > > > + .priority = priv->config.control_flow_priority, > > > }; > > > struct rte_flow_item items[] = { > > > { > > > @@ -3161,3 +3171,50 @@ mlx5_dev_filter_ctrl(struct rte_eth_dev *dev, > > > } > > > return 0; > > > } > > > + > > > +/** > > > + * Detect number of Verbs flow priorities supported. > > > + * > > > + * @param dev > > > + * Pointer to Ethernet device. > > > + */ > > > +void > > > +mlx5_flow_priorities_detect(struct rte_eth_dev *dev) { > > > + struct priv *priv = dev->data->dev_private; > > > + uint32_t verb_priorities = MLX5_VERBS_FLOW_PRIO_8 * 2; > > > + struct { > > > + struct ibv_flow_attr attr; > > > + struct ibv_flow_spec_eth eth; > > > + struct ibv_flow_spec_action_drop drop; > > > + } flow_attr = { > > > + .attr = { > > > + .num_of_specs = 2, > > > + .priority = verb_priorities - 1, > > > + }, > > > + .eth = { > > > + .type = IBV_FLOW_SPEC_ETH, > > > + .size = sizeof(struct ibv_flow_spec_eth), > > > + }, > > > + .drop = { > > > + .size = sizeof(struct ibv_flow_spec_action_drop), > > > + .type = IBV_FLOW_SPEC_ACTION_DROP, > > > + }, > > > + }; > > > + struct ibv_flow *flow; > > > + > > > + if (priv->config.control_flow_priority) > > > + return; > > > + flow = mlx5_glue->create_flow(priv->flow_drop_queue->qp, > > > + &flow_attr.attr); > > > + if (flow) { > > > + priv->config.flow_priority_shift = MLX5_VERBS_FLOW_PRIO_8 / 2; > > > + claim_zero(mlx5_glue->destroy_flow(flow)); > > > + } else { > > > + priv->config.flow_priority_shift = 1; > > > + verb_priorities = verb_priorities / 2; > > > + } > > > + priv->config.control_flow_priority = 1; > > > + DRV_LOG(INFO, "port %u Verbs flow priorities: %d", > > > + dev->data->port_id, verb_priorities); } > > > 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. > > 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. Thanks, -- Nélio Laranjeiro 6WIND