Re: [PATCH v2 0/2] Simplify mtty driver and mdev core

[Cornelia Huck]

On Tue, 20 Aug 2019 11:25:05 +0000
Parav Pandit <parav@xxxxxxxxxxxx> wrote:

> > -----Original Message-----
> > From: Christophe de Dinechin <christophe.de.dinechin@xxxxxxxxx>
> > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > 
> > 
> > Parav Pandit writes:
> >   
> > > + Dave.
> > >
> > > Hi Jiri, Dave, Alex, Kirti, Cornelia,
> > >
> > > Please provide your feedback on it, how shall we proceed?
> > >
> > > Hence, I would like to discuss below options.
> > >
> > > 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?

> > > 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.

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

> > >
> > > 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.

> 
> > 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.

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.

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.