Message-ID: <AM0PR05MB4866DF5936E1550EEDFB3B78D1AA0@AM0PR05MB4866.eurprd05.prod.outlook.com>
Date:   Wed, 21 Aug 2019 02:42:00 +0000
From:   Parav Pandit <parav@...lanox.com>
To:     Cornelia Huck <cohuck@...hat.com>
CC:     Christophe de Dinechin <christophe.de.dinechin@...il.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Jiri Pirko <jiri@...lanox.com>,
        "David S . Miller" <davem@...emloft.net>,
        Kirti Wankhede <kwankhede@...dia.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        cjia <cjia@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v2 0/2] Simplify mtty driver and mdev core



> -----Original Message-----
> From: Cornelia Huck <cohuck@...hat.com>
> Sent: Tuesday, August 20, 2019 10:01 PM
> > > > Option-1: mdev index
> > > > Introduce an optional mdev index/handle as u32 during mdev create
> time.
> > > > User passes mdev index/handle as input.
> > > >
> > > > phys_port_name=mIndex=m%u
> > > > mdev_index will be available in sysfs as mdev attribute for udev
> > > > to name the
> > > mdev's netdev.
> > > >
> > > > example mdev create command:
> > > > UUID=$(uuidgen)
> > > > echo $UUID index=10 >
> > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create
> > > > example netdevs:
> > > > repnetdev=ens2f0_m10	/*ens2f0 is parent PF's netdevice */
> > > > mdev_netdev=enm10
> > > >
> > > > Pros:
> > > > 1. mdevctl and any other existing tools are unaffected.
> > > > 2. netdev stack, ovs and other switching platforms are unaffected.
> > > > 3. achieves unique phys_port_name for representor netdev 4.
> > > > achieves unique mdev eth netdev name for the mdev using udev/systemd
> extension.
> > > > 5. Aligns well with mdev and netdev subsystem and similar to
> > > > existing sriov
> > > bdf's.
> > > >
> > > > Option-2: shorter mdev name
> > > > Extend mdev to have shorter mdev device name in addition to UUID.
> > > > such as 'foo', 'bar'.
> > > > Mdev will continue to have UUID.
> 
> I fail to understand how 'uses uuid' and 'allow shorter device name'
> are supposed to play together?
> 
Each mdev will have uuid as today. Instead of naming device based on UUID, name it based on explicit name given by the user.
Again, I want to repeat, this name parameter is optional.

> > > > phys_port_name=mdev_name
> > > >
> > > > Pros:
> > > > 1. All same as option-1, except mdevctl needs upgrade for newer usage.
> > > > It is common practice to upgrade iproute2 package along with the kernel.
> > > > Similar practice to be done with mdevctl.
> > > > 2. Newer users of mdevctl who wants to work with non_UUID names,
> > > > will use
> > > newer mdevctl/tools.
> > > > Cons:
> > > > 1. Dual naming scheme of mdev might affect some of the existing tools.
> > > > It's unclear how/if it actually affects.
> > > > mdevctl [2] is very recently developed and can be enhanced for
> > > > dual naming
> > > scheme.
> 
> The main problem is not tools we know about (i.e. mdevctl), but those we don't
> know about.
> 
Well, if it not part of the distros, there is very little can do about it by kernel.
I tried mdevctl with mdev named using non UUID and it were able to list them.

> IOW, this (and the IFNAMESIZ change, which seems even worse) are the
> options I would not want at all.
> 
Ok.

> > > >
> > > > Option-3: mdev uuid alias
> > > > Instead of shorter mdev name or mdev index, have alpha-numeric
> > > > name
> > > alias.
> > > > Alias is an optional mdev sysfs attribute such as 'foo', 'bar'.
> > > > example mdev create command:
> > > > UUID=$(uuidgen)
> > > > echo $UUID alias=foo >
> > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create
> > > > example netdevs:
> > > > examle netdevs:
> > > > repnetdev = ens2f0_mfoo
> > > > mdev_netdev=enmfoo
> > > >
> > > > Pros:
> > > > 1. All same as option-1.
> > > > 2. Doesn't affect existing mdev naming scheme.
> > > > Cons:
> > > > 1. Index scheme of option-1 is better which can number large
> > > > number of
> > > mdevs with fewer characters, simplifying the management tool.
> > >
> > > I believe that Alex pointed out another "Cons" to all three options,
> > > which is that it forces user-space to resolve potential race
> > > conditions when creating an index or short name or alias.
> > >
> > This race condition exists for at least two subsystems that I know of, i.e.
> netdev and rdma.
> > If a device with a given name exists, subsystem returns error.
> > When user space gets error code EEXIST, and it can picks up different
> identifier(s).
> 
> If you decouple device creation and setting the alias/index, you make the issue
> visible and thus much more manageable.
> 
I thought about it. It has two issues.
1. user should be able to set this only once. Repeatedly setting it requires changing/notifying it.
2. setting alias translating in creating devlink port doesn't sound correct.
Because if user attempts to reset to different value, it required unregistration, reregistration.
All of such race conditions handling it not worth it.
So setting the index, I liked Alex's term more 'instance number', at instance creation time is lot more simple.

> >
> > > Also, what happens if `index=10` is not provided on the command-line?
> > > Does that make the device unusable for your purpose?
> > Yes, it is unusable to an extent.
> > Currently we have DEVLINK_PORT_FLAVOUR_PCI_VF in
> > include/uapi/linux/devlink.h Similar to it, we need to have
> DEVLINK_PORT_FLAVOUR_MDEV for mdev eswitch ports.
> > This port flavour needs to generate phys_port_name(). This should be user
> parameter driven.
> > Because representor netdevice name is generated based on this parameter.
> 
> I'm also unsure how the extra parameter is supposed to work; writing it to the
> create attribute does not sound right.
> 
Why? When you create a device it takes multiple mandatory and optional parameters.
This is common for netdev (vxlan, vlan, macvlan, ipvlan, gre and more).

> mdevctl supports setting additional parameters on an already created device
> (see the examples provided for vfio-ap), so going that route would actually
> work out of the box from the tooling side.
> 
I explained that setting and re-setting attributes for instance create time value is not worth.

> What you would need is some kind of synchronization/locking to make sure that
> you only link up to the other device after the extra attribute has been set and
> that you don't allow to change it as long as it is associated with the other side. I
> do not know enough about the actual devices to suggest something here; if you
> need userspace cooperation, maybe uevents would be an option.

Powered by blists - more mailing lists