From: "Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>
To: "Xueming(Steven) Li" <xuemingl@mellanox.com>
Cc: Shahaf Shuler <shahafs@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 01/15] net/mlx5: support 16 hardware priorities
Date: Thu, 12 Apr 2018 16:02:56 +0200 [thread overview]
Message-ID: <20180412140256.q7zskb7ouqlvbt2p@laranjeiro-vm.dev.6wind.com> (raw)
In-Reply-To: <VI1PR05MB1678F921A2B53C2CA00F33CEACBC0@VI1PR05MB1678.eurprd05.prod.outlook.com>
On Thu, Apr 12, 2018 at 01:43:04PM +0000, Xueming(Steven) Li wrote:
>
>
> > -----Original Message-----
> > From: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Sent: Thursday, April 12, 2018 5:09 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; 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 <nelio.laranjeiro@6wind.com>
> > > > Sent: Tuesday, April 10, 2018 10:42 PM
> > > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > > Cc: Shahaf Shuler <shahafs@mellanox.com>; 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 <xuemingl@mellanox.com>
> > <snip/>
> > > > > },
> > > > > [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.
>
> >
> > <snip/>
> > > > > 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.
<snip/>
> > > > > 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
next prev parent reply other threads:[~2018-04-12 14:02 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-10 13:34 [dpdk-dev] [PATCH v2 00/15] mlx5 Rx tunnel offloading Xueming Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 01/15] net/mlx5: support 16 hardware priorities Xueming Li
2018-04-10 14:41 ` Nélio Laranjeiro
2018-04-10 15:22 ` Xueming(Steven) Li
2018-04-12 9:09 ` Nélio Laranjeiro
2018-04-12 13:43 ` Xueming(Steven) Li
2018-04-12 14:02 ` Nélio Laranjeiro [this message]
2018-04-12 14:46 ` Xueming(Steven) Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 02/15] net/mlx5: support GRE tunnel flow Xueming Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 03/15] net/mlx5: support L3 vxlan flow Xueming Li
2018-04-10 14:53 ` Nélio Laranjeiro
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 04/15] net/mlx5: support Rx tunnel type identification Xueming Li
2018-04-10 15:17 ` Nélio Laranjeiro
2018-04-11 8:11 ` Xueming(Steven) Li
2018-04-12 9:50 ` Nélio Laranjeiro
2018-04-12 14:27 ` Xueming(Steven) Li
2018-04-13 8:37 ` Nélio Laranjeiro
2018-04-13 12:09 ` Xueming(Steven) Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 05/15] net/mlx5: support tunnel inner checksum offloads Xueming Li
2018-04-10 15:27 ` Nélio Laranjeiro
2018-04-11 8:46 ` Xueming(Steven) Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 06/15] net/mlx5: split flow RSS handling logic Xueming Li
2018-04-10 15:28 ` Nélio Laranjeiro
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 07/15] net/mlx5: support tunnel RSS level Xueming Li
2018-04-11 8:55 ` Nélio Laranjeiro
2018-04-14 12:25 ` Xueming(Steven) Li
2018-04-16 7:14 ` Nélio Laranjeiro
2018-04-16 7:46 ` Xueming(Steven) Li
2018-04-16 8:09 ` Nélio Laranjeiro
2018-04-16 10:06 ` Xueming(Steven) Li
2018-04-16 12:27 ` Nélio Laranjeiro
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 08/15] net/mlx5: add hardware flow debug dump Xueming Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 09/15] net/mlx5: introduce VXLAN-GPE tunnel type Xueming Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 10/15] net/mlx5: allow flow tunnel ID 0 with outer pattern Xueming Li
2018-04-11 12:25 ` Nélio Laranjeiro
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 11/15] net/mlx5: support MPLS-in-GRE and MPLS-in-UDP Xueming Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 12/15] doc: update mlx5 guide on tunnel offloading Xueming Li
2018-04-11 12:32 ` Nélio Laranjeiro
2018-04-11 12:43 ` Thomas Monjalon
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 13/15] net/mlx5: setup RSS flow regardless of queue count Xueming Li
2018-04-11 12:37 ` Nélio Laranjeiro
2018-04-11 13:01 ` Xueming(Steven) Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 14/15] net/mlx5: fix invalid flow item check Xueming Li
2018-04-10 13:34 ` [dpdk-dev] [PATCH v2 15/15] net/mlx5: support RSS configuration in isolated mode Xueming Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180412140256.q7zskb7ouqlvbt2p@laranjeiro-vm.dev.6wind.com \
--to=nelio.laranjeiro@6wind.com \
--cc=dev@dpdk.org \
--cc=shahafs@mellanox.com \
--cc=xuemingl@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).