[dpdk-dev] [PATCH v4 07/10] net/mlx5: probe all port representors

Shahaf Shuler shahafs at mellanox.com
Tue Jul 10 12:13:25 CEST 2018


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 at 6wind.com>
> > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro at 6wind.com>
> > > Reviewed-by: Xueming Li <xuemingl at mellanox.com>
> > > Cc: Xueming Li <xuemingl at mellanox.com>
> > > Cc: Shahaf Shuler <shahafs at 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


More information about the dev mailing list