From: Shahaf Shuler <shahafs@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>,
"Xueming(Steven) Li" <xuemingl@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v4 07/10] net/mlx5: probe all port representors
Date: Tue, 10 Jul 2018 10:13:25 +0000 [thread overview]
Message-ID: <DB7PR05MB4426B888F4951FA025EAC196C35B0@DB7PR05MB4426.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20180710093708.GH5211@6wind.com>
Tuesday, July 10, 2018 12:37 PM, Adrien Mazarguil:
> Subject: Re: [PATCH v4 07/10] net/mlx5: probe all port representors
>
> On Mon, Jul 09, 2018 at 11:57:29AM +0000, Shahaf Shuler wrote:
> > Hi Adrien,
> >
> >
> > Thursday, July 5, 2018 11:46 AM, Adrien Mazarguil:
> > > Subject: [PATCH v4 07/10] net/mlx5: probe all port representors
> > >
> > > Probe existing port representors in addition to their master device
> > > and associate them automatically.
> > >
> > > To avoid collision between Ethernet devices, they are named as follows:
> > >
> > > - "{DBDF}" for master/switch devices.
> > > - "{DBDF}_representor_{rep}" with "rep" starting from 0 for port
> > > representors.
> > >
> > > (Patch based on prior work from Yuanhan Liu)
> > >
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > > Reviewed-by: Xueming Li <xuemingl@mellanox.com>
> > > Cc: Xueming Li <xuemingl@mellanox.com>
> > > Cc: Shahaf Shuler <shahafs@mellanox.com>
> > > --
> > > v4 changes:
> > >
> > > - Fixed domain ID release once the last port using it is closed. Closed
> > > devices are not necessarily detached, their presence is not a good
> > > indicator. Code was modified to check if they still use their domain IDs
> > > before deciding to release it.
> <snip>
> > > @@ -883,6 +915,41 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> > > priv->nl_socket_rdma = mlx5_nl_init(0, NETLINK_RDMA);
> > > priv->nl_socket_route = mlx5_nl_init(RTMGRP_LINK,
> > > NETLINK_ROUTE);
> > > priv->nl_sn = 0;
> > > + priv->representor = !!switch_info->representor;
> > > + priv->domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
> > > + priv->representor_id =
> > > + switch_info->representor ? switch_info->port_name : -1;
> > > + /*
> > > + * Look for sibling devices in order to reuse their switch domain
> > > + * if any, otherwise allocate one.
> > > + */
> > > + i = mlx5_dev_to_port_id(dpdk_dev, NULL, 0);
> > > + if (i > 0) {
> > > + uint16_t port_id[i];
> > > +
> > > + i = RTE_MIN(mlx5_dev_to_port_id(dpdk_dev, port_id, i), i);
> > > + while (i--) {
> > > + const struct priv *opriv =
> > > + rte_eth_devices[port_id[i]].data-
> > > >dev_private;
> > > +
> > > + if (!opriv ||
> > > + opriv->domain_id ==
> > > + RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
> > > + continue;
> > > + priv->domain_id = opriv->domain_id;
> >
> > It looks like for the second port it will use the domain_id of the first port. Is
> that what you intent?
>
> Yes, it's on purpose. Master and representors of a given device must share
> the same domain ID to let applications know they can create flow rules to
> forward traffic between them all.
But this is not the case in Mellanox devices. On Mellanox devices each PF along w/ its representors has a separate eswitch, and traffic cannot be routed between the switches using flow rules.
For example if we have PF0 along w/ its representor REP0_0 and PF1 along w/ its representor REP1_0 . PF0 and REP0_0 will belong to switch X and PF1 and REP1_0 will belong to switch domain Y. it is also being reflected on the phys_switch_id.
We should have switch domain per PF.
>
> > Note - I couldn't test it due to compilation errors:
> >
> >
> /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5
> _nl.c: In function 'mlx5_nl_switch_info_cb':
> >
> /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5
> _
> > nl.c:843:8: error: 'IFLA_PHYS_PORT_NAME' undecl ared (first use in this
> function)
> > case IFLA_PHYS_PORT_NAME:
> > ^
> >
> /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5
> _
> > nl.c:843:8: note: each undeclared identifier is reported only once
> > for each function it appears in
> >
> /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5
> _
> > nl.c:851:8: error: 'IFLA_PHYS_SWITCH_ID' undecl ared (first use in this
> function)
> > case IFLA_PHYS_SWITCH_ID:
> > ^
> >
> > My system info:
> > NAME="Red Hat Enterprise Linux Server"
> > VERSION="7.3 (Maipo)"
> > ID="rhel"
> > ID_LIKE="fedora"
> > VERSION_ID="7.3"
> > PRETTY_NAME="Red Hat Enterprise Linux Server 7.3 (Maipo)"
> > ANSI_COLOR="0;31"
> > CPE_NAME="cpe:/o:redhat:enterprise_linux:7.3:GA:server"
> >
> HOME_URL="https://emea01.safelinks.protection.outlook.com/?url=https%
> 3A%2F%2Fwww.redhat.com%2F&data=02%7C01%7Cshahafs%40mellan
> ox.com%7C661e7b51087b460817c008d5e648bf1e%7Ca652971c7d2e4d9ba6a4
> d149256f461b%7C0%7C0%7C636668122474445351&sdata=Lg8arhiYLvH5L
> 2hef8DVhS8A3fVJ%2B5IZkLIHmqCd%2FmY%3D&reserved=0"
> >
> BUG_REPORT_URL="https://emea01.safelinks.protection.outlook.com/?url=
> https%3A%2F%2Fbugzilla.redhat.com%2F&data=02%7C01%7Cshahafs%
> 40mellanox.com%7C661e7b51087b460817c008d5e648bf1e%7Ca652971c7d2e
> 4d9ba6a4d149256f461b%7C0%7C0%7C636668122474445351&sdata=3Do
> RKjxovM8tOgKLssC1mq2wwfhjpVUZSExXV4ywBEQ%3D&reserved=0"
> >
> > REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 7"
> > REDHAT_BUGZILLA_PRODUCT_VERSION=7.3
> > REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux"
> > REDHAT_SUPPORT_PRODUCT_VERSION="7.3"
> > Red Hat Enterprise Linux Server release 7.3 (Maipo) Red Hat Enterprise
> > Linux Server release 7.3 (Maipo)
>
> OK, I'll redefine in v5 in case they are missing on the host system.
>
> <snip>
> > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > > 704046270..cc01310e0 100644
> > > --- a/drivers/net/mlx5/mlx5.h
> > > +++ b/drivers/net/mlx5/mlx5.h
> > > @@ -159,6 +159,7 @@ struct priv {
> > > struct ibv_context *ctx; /* Verbs context. */
> > > struct ibv_device_attr_ex device_attr; /* Device properties. */
> > > struct ibv_pd *pd; /* Protection Domain. */
> > > + char ibdev_name[IBV_SYSFS_NAME_MAX]; /* IB device name. */
> >
> >
> > Why we need a dedicated entry for the ibdev_name? it is already part of
> priv->ctx->device->name.
>
> Heh, same reason as the next line below, don't forget those damn
> secondaries which can't dereference local pointers from the primary process
> :)
Right 😊.
>
> > > char ibdev_path[IBV_SYSFS_PATH_MAX]; /* IB device path for
> > > secondary */
> <snip>
> > > struct rte_eth_dev_info *info)
> > > info->speed_capa = priv->link_speed_capa;
> > > info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
> > > mlx5_set_default_params(dev, info);
> > > + info->switch_info.name = dev->data->name;
> > > + info->switch_info.domain_id = priv->domain_id;
> > > + info->switch_info.port_id = priv->representor_id;
> > > + if (priv->representor) {
> > > + unsigned int i = mlx5_dev_to_port_id(dev->device, NULL, 0);
> > > + uint16_t port_id[i];
> > > +
> > > + i = RTE_MIN(mlx5_dev_to_port_id(dev->device, port_id, i),
> > > i);
> > > + while (i--) {
> > > + struct priv *opriv =
> > > + rte_eth_devices[port_id[i]].data-
> > > >dev_private;
> > > +
> > > + if (!opriv ||
> > > + opriv->representor ||
> > > + opriv->domain_id != priv->domain_id)
> > > + continue;
> > > + /*
> > > + * Override switch name with that of the master
> > > + * device.
> > > + */
> > > + info->switch_info.name = opriv->dev_data->name;
> > > + break;
> >
> > According to this logic it means once the master device is closed, all the
> representors are no longer belong to the same switch (switch name of each
> is different) which is not correct.
>
> They still share the same domain ID, which is what actually matters. The
> switch name is only provided to let applications identify the master
> (control) device in case it's needed.
>
> > According to your notes it is possible to close master w/o closing the
> representor.
>
> This allows devices to be probed in any order on a needed basis, not all at
> once. It's done on purpose to pave the way for hotplug support.
>
> > Why not just storing the master switch name when probing the
> representor and to use it as is on the dev_info?
>
> The switch name *must* be that of the master device. If the master is not
> probed, there can't be a switch name. However there's no real provision for
> this in the API, so I chose the most acceptable unique name, which is the
> name of the local device. Would you prefer an empty name instead?
The current approach is OK.
I was just suggesting to skip the loop iteration by saving the switch name on the private structure.
>
> Thing is, on mlx5 flow rules can be created directly between representors
> without involving the master device. An empty switch name may be
> misleading in this respect.
>
> What do you suggest?
>
> --
> Adrien Mazarguil
> 6WIND
next prev parent reply other threads:[~2018-07-10 10:13 UTC|newest]
Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-25 16:35 [dpdk-dev] [PATCH 0/7] net/mlx5: add port representor support Adrien Mazarguil
2018-05-25 16:35 ` [dpdk-dev] [PATCH 1/7] net/mlx5: rename confusing object in probe code Adrien Mazarguil
2018-06-10 11:00 ` Xueming(Steven) Li
2018-06-12 13:19 ` Adrien Mazarguil
2018-05-25 16:35 ` [dpdk-dev] [PATCH 2/7] net/mlx5: remove redundant objects " Adrien Mazarguil
2018-06-10 11:00 ` Xueming(Steven) Li
2018-06-12 13:19 ` Adrien Mazarguil
2018-05-25 16:35 ` [dpdk-dev] [PATCH 3/7] net/mlx5: split PCI from generic probing code Adrien Mazarguil
2018-06-10 12:59 ` Xueming(Steven) Li
2018-06-12 13:20 ` Adrien Mazarguil
2018-05-25 16:35 ` [dpdk-dev] [PATCH 4/7] net/mlx5: re-indent generic probing function Adrien Mazarguil
2018-06-11 11:42 ` Xueming(Steven) Li
2018-05-25 16:35 ` [dpdk-dev] [PATCH 5/7] net/mlx5: add port representor awareness Adrien Mazarguil
2018-05-25 16:35 ` [dpdk-dev] [PATCH 6/7] net/mlx5: probe all port representors Adrien Mazarguil
2018-06-12 6:42 ` Xueming(Steven) Li
2018-06-12 13:20 ` Adrien Mazarguil
2018-05-25 16:35 ` [dpdk-dev] [PATCH 7/7] net/mlx5: add parameter for " Adrien Mazarguil
2018-06-12 8:02 ` Xueming(Steven) Li
2018-06-12 13:20 ` Adrien Mazarguil
2018-06-12 13:43 ` Xueming(Steven) Li
2018-06-14 8:01 ` Adrien Mazarguil
2018-06-12 14:44 ` Xueming(Steven) Li
2018-06-13 13:11 ` Adrien Mazarguil
2018-06-14 8:34 ` [dpdk-dev] [PATCH v2 0/7] net/mlx5: add port representor support Adrien Mazarguil
2018-06-14 8:34 ` [dpdk-dev] [PATCH v2 1/7] net/mlx5: rename confusing object in probe code Adrien Mazarguil
2018-06-16 8:24 ` Xueming(Steven) Li
2018-06-14 8:34 ` [dpdk-dev] [PATCH v2 2/7] net/mlx5: remove redundant objects " Adrien Mazarguil
2018-06-16 8:27 ` Xueming(Steven) Li
2018-06-17 10:14 ` Shahaf Shuler
2018-06-27 13:30 ` Adrien Mazarguil
2018-06-28 5:35 ` Shahaf Shuler
2018-06-14 8:34 ` [dpdk-dev] [PATCH v2 3/7] net/mlx5: split PCI from generic probing code Adrien Mazarguil
2018-06-16 8:29 ` Xueming(Steven) Li
2018-06-17 10:14 ` Shahaf Shuler
2018-06-27 13:31 ` Adrien Mazarguil
2018-06-14 8:34 ` [dpdk-dev] [PATCH v2 4/7] net/mlx5: re-indent generic probing function Adrien Mazarguil
2018-06-14 8:34 ` [dpdk-dev] [PATCH v2 5/7] net/mlx5: add port representor awareness Adrien Mazarguil
2018-06-16 8:37 ` Xueming(Steven) Li
2018-06-27 13:32 ` Adrien Mazarguil
2018-06-14 8:35 ` [dpdk-dev] [PATCH v2 6/7] net/mlx5: probe all port representors Adrien Mazarguil
2018-06-16 8:57 ` Xueming(Steven) Li
2018-06-17 10:15 ` Shahaf Shuler
2018-06-24 13:33 ` Shahaf Shuler
2018-06-27 13:32 ` Adrien Mazarguil
2018-06-28 5:57 ` Shahaf Shuler
2018-06-28 9:13 ` Adrien Mazarguil
2018-06-27 13:32 ` Adrien Mazarguil
2018-06-27 17:30 ` Xueming(Steven) Li
2018-06-28 6:01 ` Shahaf Shuler
2018-06-28 8:45 ` Adrien Mazarguil
2018-06-28 9:06 ` Shahaf Shuler
2018-06-27 13:32 ` Adrien Mazarguil
2018-06-14 8:35 ` [dpdk-dev] [PATCH v2 7/7] net/mlx5: add parameter for " Adrien Mazarguil
2018-06-16 8:59 ` Xueming(Steven) Li
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 00/10] net/mlx5: add port representor support Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 01/10] net/mlx5: rename confusing object in probe code Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 02/10] net/mlx5: remove redundant objects " Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 03/10] net/mlx5: drop useless support for several Verbs ports Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 04/10] net/mlx5: split PCI from generic probing code Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 05/10] net/mlx5: re-indent generic probing function Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 06/10] net/mlx5: add port representor awareness Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 07/10] net/mlx5: probe all port representors Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 08/10] net/mlx5: probe port representors in natural order Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 09/10] net/mlx5: add parameter for port representors Adrien Mazarguil
2018-07-04 17:27 ` [dpdk-dev] [PATCH v3 10/10] net/mlx5: support negative identifiers " Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 00/10] net/mlx5: add port representor support Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 01/10] net/mlx5: rename confusing object in probe code Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 02/10] net/mlx5: remove redundant objects " Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 03/10] net/mlx5: drop useless support for several Verbs ports Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 04/10] net/mlx5: split PCI from generic probing code Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 05/10] net/mlx5: re-indent generic probing function Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 06/10] net/mlx5: add port representor awareness Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 07/10] net/mlx5: probe all port representors Adrien Mazarguil
2018-07-09 11:57 ` Shahaf Shuler
2018-07-10 9:37 ` Adrien Mazarguil
2018-07-10 10:13 ` Shahaf Shuler [this message]
2018-07-10 10:58 ` Adrien Mazarguil
2018-07-10 11:17 ` Shahaf Shuler
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 08/10] net/mlx5: probe port representors in natural order Adrien Mazarguil
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 09/10] net/mlx5: add parameter for port representors Adrien Mazarguil
2018-07-09 11:57 ` Shahaf Shuler
2018-07-10 9:37 ` Adrien Mazarguil
2018-07-10 10:16 ` Shahaf Shuler
2018-07-10 10:58 ` Adrien Mazarguil
2018-07-10 11:15 ` Shahaf Shuler
2018-07-05 8:45 ` [dpdk-dev] [PATCH v4 10/10] net/mlx5: support negative identifiers " Adrien Mazarguil
2018-07-09 11:58 ` Shahaf Shuler
2018-07-10 9:37 ` Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 00/10] net/mlx5: add port representor support Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 01/10] net/mlx5: rename confusing object in probe code Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 02/10] net/mlx5: remove redundant objects " Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 03/10] net/mlx5: drop useless support for several Verbs ports Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 04/10] net/mlx5: split PCI from generic probing code Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 05/10] net/mlx5: re-indent generic probing function Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 06/10] net/mlx5: add port representor awareness Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 07/10] net/mlx5: probe all port representors Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 08/10] net/mlx5: probe port representors in natural order Adrien Mazarguil
2018-07-10 16:04 ` [dpdk-dev] [PATCH v5 09/10] net/mlx5: add parameter for port representors Adrien Mazarguil
2018-07-10 16:05 ` [dpdk-dev] [PATCH v5 10/10] net/mlx5: support negative identifiers " Adrien Mazarguil
2018-07-12 7:51 ` [dpdk-dev] [PATCH v5 00/10] net/mlx5: add port representor support Shahaf Shuler
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=DB7PR05MB4426B888F4951FA025EAC196C35B0@DB7PR05MB4426.eurprd05.prod.outlook.com \
--to=shahafs@mellanox.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=nelio.laranjeiro@6wind.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).