Message-ID: <20190823095229.210e1e84@x1.home>
Date:   Fri, 23 Aug 2019 09:52:29 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Parav Pandit <parav@...lanox.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, Jiri Pirko <jiri@...lanox.com>,
        "David S . Miller" <davem@...emloft.net>,
        Kirti Wankhede <kwankhede@...dia.com>,
        Cornelia Huck <cohuck@...hat.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

On Fri, 23 Aug 2019 14:53:06 +0000
Parav Pandit <parav@...lanox.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson <alex.williamson@...hat.com>
> > Sent: Friday, August 23, 2019 7:58 PM
> > To: Parav Pandit <parav@...lanox.com>
> > Cc: Jiri Pirko <jiri@...nulli.us>; Jiri Pirko <jiri@...lanox.com>; David S . Miller
> > <davem@...emloft.net>; Kirti Wankhede <kwankhede@...dia.com>; Cornelia
> > Huck <cohuck@...hat.com>; kvm@...r.kernel.org; linux-
> > kernel@...r.kernel.org; cjia <cjia@...dia.com>; netdev@...r.kernel.org
> > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > 
> > On Fri, 23 Aug 2019 08:14:39 +0000
> > Parav Pandit <parav@...lanox.com> wrote:
> >   
> > > Hi Alex,
> > >
> > >  
> > > > -----Original Message-----
> > > > From: Jiri Pirko <jiri@...nulli.us>
> > > > Sent: Friday, August 23, 2019 1:42 PM
> > > > To: Parav Pandit <parav@...lanox.com>
> > > > Cc: Alex Williamson <alex.williamson@...hat.com>; Jiri Pirko
> > > > <jiri@...lanox.com>; David S . Miller <davem@...emloft.net>; Kirti
> > > > Wankhede <kwankhede@...dia.com>; Cornelia Huck  
> > <cohuck@...hat.com>;  
> > > > kvm@...r.kernel.org; linux-kernel@...r.kernel.org; cjia
> > > > <cjia@...dia.com>; netdev@...r.kernel.org
> > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > > >
> > > > Thu, Aug 22, 2019 at 03:33:30PM CEST, parav@...lanox.com wrote:  
> > > > >
> > > > >  
> > > > >> -----Original Message-----
> > > > >> From: Jiri Pirko <jiri@...nulli.us>
> > > > >> Sent: Thursday, August 22, 2019 5:50 PM
> > > > >> To: Parav Pandit <parav@...lanox.com>
> > > > >> Cc: Alex Williamson <alex.williamson@...hat.com>; Jiri Pirko
> > > > >> <jiri@...lanox.com>; David S . Miller <davem@...emloft.net>;
> > > > >> Kirti Wankhede <kwankhede@...dia.com>; Cornelia Huck  
> > > > <cohuck@...hat.com>;  
> > > > >> kvm@...r.kernel.org; linux-kernel@...r.kernel.org; cjia
> > > > >> <cjia@...dia.com>; netdev@...r.kernel.org
> > > > >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > > > >>
> > > > >> Thu, Aug 22, 2019 at 12:04:02PM CEST, parav@...lanox.com wrote:  
> > > > >> >
> > > > >> >  
> > > > >> >> -----Original Message-----
> > > > >> >> From: Jiri Pirko <jiri@...nulli.us>
> > > > >> >> Sent: Thursday, August 22, 2019 3:28 PM
> > > > >> >> To: Parav Pandit <parav@...lanox.com>
> > > > >> >> Cc: Alex Williamson <alex.williamson@...hat.com>; Jiri Pirko
> > > > >> >> <jiri@...lanox.com>; David S . Miller <davem@...emloft.net>;
> > > > >> >> Kirti Wankhede <kwankhede@...dia.com>; Cornelia Huck  
> > > > >> <cohuck@...hat.com>;  
> > > > >> >> kvm@...r.kernel.org; linux-kernel@...r.kernel.org; cjia
> > > > >> >> <cjia@...dia.com>; netdev@...r.kernel.org
> > > > >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > > > >> >>
> > > > >> >> Thu, Aug 22, 2019 at 11:42:13AM CEST, parav@...lanox.com wrote:  
> > > > >> >> >
> > > > >> >> >  
> > > > >> >> >> -----Original Message-----
> > > > >> >> >> From: Jiri Pirko <jiri@...nulli.us>
> > > > >> >> >> Sent: Thursday, August 22, 2019 2:59 PM
> > > > >> >> >> To: Parav Pandit <parav@...lanox.com>
> > > > >> >> >> Cc: Alex Williamson <alex.williamson@...hat.com>; Jiri
> > > > >> >> >> Pirko <jiri@...lanox.com>; David S . Miller
> > > > >> >> >> <davem@...emloft.net>; Kirti Wankhede
> > > > >> >> >> <kwankhede@...dia.com>; Cornelia Huck  
> > > > >> >> <cohuck@...hat.com>;  
> > > > >> >> >> kvm@...r.kernel.org; linux-kernel@...r.kernel.org; cjia
> > > > >> >> >> <cjia@...dia.com>; netdev@...r.kernel.org
> > > > >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev
> > > > >> >> >> core
> > > > >> >> >>
> > > > >> >> >> Wed, Aug 21, 2019 at 08:23:17AM CEST, parav@...lanox.com  
> > wrote:  
> > > > >> >> >> >
> > > > >> >> >> >  
> > > > >> >> >> >> -----Original Message-----
> > > > >> >> >> >> From: Alex Williamson <alex.williamson@...hat.com>
> > > > >> >> >> >> Sent: Wednesday, August 21, 2019 10:56 AM
> > > > >> >> >> >> To: Parav Pandit <parav@...lanox.com>
> > > > >> >> >> >> Cc: Jiri Pirko <jiri@...lanox.com>; David S . Miller
> > > > >> >> >> >> <davem@...emloft.net>; Kirti Wankhede
> > > > >> >> >> >> <kwankhede@...dia.com>; Cornelia Huck
> > > > >> >> >> >> <cohuck@...hat.com>; kvm@...r.kernel.org;
> > > > >> >> >> >> linux-kernel@...r.kernel.org; cjia <cjia@...dia.com>;
> > > > >> >> >> >> netdev@...r.kernel.org
> > > > >> >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and
> > > > >> >> >> >> mdev core
> > > > >> >> >> >>  
> > > > >> >> >> >> > > > > Just an example of the alias, not proposing how it's set.
> > > > >> >> >> >> > > > > In fact, proposing that the user does not set
> > > > >> >> >> >> > > > > it, mdev-core provides one  
> > > > >> >> >> >> > > automatically.  
> > > > >> >> >> >> > > > >  
> > > > >> >> >> >> > > > > > > Since there seems to be some prefix
> > > > >> >> >> >> > > > > > > overhead, as I ask about above in how many
> > > > >> >> >> >> > > > > > > characters we actually have to work with in
> > > > >> >> >> >> > > > > > > IFNAMESZ, maybe we start with
> > > > >> >> >> >> > > > > > > 8 characters (matching your "index"
> > > > >> >> >> >> > > > > > > namespace) and expand as necessary for  
> > > > >> >> >> disambiguation.  
> > > > >> >> >> >> > > > > > > If we can eliminate overhead in IFNAMESZ,
> > > > >> >> >> >> > > > > > > let's start with  
> > > > >> 12.  
> > > > >> >> >> >> > > > > > > Thanks,
> > > > >> >> >> >> > > > > > >  
> > > > >> >> >> >> > > > > > If user is going to choose the alias, why does
> > > > >> >> >> >> > > > > > it have to be limited to  
> > > > >> >> >> >> sha1?  
> > > > >> >> >> >> > > > > > Or you just told it as an example?
> > > > >> >> >> >> > > > > >
> > > > >> >> >> >> > > > > > It can be an alpha-numeric string.  
> > > > >> >> >> >> > > > >
> > > > >> >> >> >> > > > > No, I'm proposing a different solution where
> > > > >> >> >> >> > > > > mdev-core creates an alias based on an
> > > > >> >> >> >> > > > > abbreviated sha1.  The user does not provide the  
> > > > >> >> >> >> alias.  
> > > > >> >> >> >> > > > >  
> > > > >> >> >> >> > > > > > Instead of mdev imposing number of characters
> > > > >> >> >> >> > > > > > on the alias, it should be best  
> > > > >> >> >> >> > > > > left to the user.  
> > > > >> >> >> >> > > > > > Because in future if netdev improves on the
> > > > >> >> >> >> > > > > > naming scheme, mdev will be  
> > > > >> >> >> >> > > > > limiting it, which is not right.  
> > > > >> >> >> >> > > > > > So not restricting alias size seems right to me.
> > > > >> >> >> >> > > > > > User configuring mdev for networking devices
> > > > >> >> >> >> > > > > > in a given kernel knows what  
> > > > >> >> >> >> > > > > user is doing.  
> > > > >> >> >> >> > > > > > So user can choose alias name size as it finds suitable.  
> > > > >> >> >> >> > > > >
> > > > >> >> >> >> > > > > That's not what I'm proposing, please read again.
> > > > >> >> >> >> > > > > Thanks,  
> > > > >> >> >> >> > > >
> > > > >> >> >> >> > > > I understood your point. But mdev doesn't know how
> > > > >> >> >> >> > > > user is going to use  
> > > > >> >> >> >> > > udev/systemd to name the netdev.  
> > > > >> >> >> >> > > > So even if mdev chose to pick 12 characters, it
> > > > >> >> >> >> > > > could result in  
> > > > >> >> collision.  
> > > > >> >> >> >> > > > Hence the proposal to provide the alias by the
> > > > >> >> >> >> > > > user, as user know the best  
> > > > >> >> >> >> > > policy for its use case in the environment its using.  
> > > > >> >> >> >> > > > So 12 character sha1 method will still work by user.  
> > > > >> >> >> >> > >
> > > > >> >> >> >> > > Haven't you already provided examples where certain
> > > > >> >> >> >> > > drivers or subsystems have unique netdev prefixes?
> > > > >> >> >> >> > > If mdev provides a unique alias within the
> > > > >> >> >> >> > > subsystem, couldn't we simply define a netdev prefix
> > > > >> >> >> >> > > for the mdev subsystem and avoid all other
> > > > >> >> >> >> > > collisions?  I'm not in favor of the user providing
> > > > >> >> >> >> > > both a uuid and an alias/instance.  Thanks,
> > > > >> >> >> >> > >  
> > > > >> >> >> >> > For a given prefix, say ens2f0, can two UUID->sha1
> > > > >> >> >> >> > first 9 characters have  
> > > > >> >> >> >> collision?
> > > > >> >> >> >>
> > > > >> >> >> >> I think it would be a mistake to waste so many chars on
> > > > >> >> >> >> a prefix, but
> > > > >> >> >> >> 9 characters of sha1 likely wouldn't have a collision
> > > > >> >> >> >> before we have 10s of thousands of devices.  Thanks,
> > > > >> >> >> >>
> > > > >> >> >> >> Alex  
> > > > >> >> >> >
> > > > >> >> >> >Jiri, Dave,
> > > > >> >> >> >Are you ok with it for devlink/netdev part?
> > > > >> >> >> >Mdev core will create an alias from a UUID.
> > > > >> >> >> >
> > > > >> >> >> >This will be supplied during devlink port attr set such
> > > > >> >> >> >as,
> > > > >> >> >> >
> > > > >> >> >> >devlink_port_attrs_mdev_set(struct devlink_port *port,
> > > > >> >> >> >const char *mdev_alias);
> > > > >> >> >> >
> > > > >> >> >> >This alias is used to generate representor netdev's  
> > phys_port_name.  
> > > > >> >> >> >This alias from the mdev device's sysfs will be used by
> > > > >> >> >> >the udev/systemd to  
> > > > >> >> >> generate predicable netdev's name.  
> > > > >> >> >> >Example: enm<mdev_alias_first_12_chars>  
> > > > >> >> >>
> > > > >> >> >> What happens in unlikely case of 2 UUIDs collide?
> > > > >> >> >>  
> > > > >> >> >Since users sees two devices with same phys_port_name, user
> > > > >> >> >should destroy  
> > > > >> >> recently created mdev and recreate mdev with different UUID?
> > > > >> >>
> > > > >> >> Driver should make sure phys port name wont collide,  
> > > > >> >So when mdev creation is initiated, mdev core calculates the
> > > > >> >alias and if there  
> > > > >> is any other mdev with same alias exist, it returns -EEXIST error
> > > > >> before progressing further.  
> > > > >> >This way user will get to know upfront in event of collision
> > > > >> >before the mdev  
> > > > >> device gets created.  
> > > > >> >How about that?  
> > > > >>
> > > > >> Sounds fine to me. Now the question is how many chars do we want to  
> > have.  
> > > > >>  
> > > > >12 characters from Alex's suggestion similar to git?  
> > > >
> > > > Ok.
> > > >  
> > >
> > > Can you please confirm this scheme looks good now? I like to get patches  
> > started.
> > 
> > My only concern is your comment that in the event of an abbreviated
> > sha1 collision (as exceptionally rare as that might be at 12-chars), we'd fail the
> > device create, while my original suggestion was that vfio-core would add an
> > extra character to the alias.  For non-networking devices, the sha1 is
> > unnecessary, so the extension behavior seems preferred.  The user is only
> > responsible to provide a unique uuid.  Perhaps the failure behavior could be
> > applied based on the mdev device_api.  A module option on mdev to specify the
> > default number of alias chars would also be useful for testing so that we can set
> > it low enough to validate the collision behavior.  Thanks,
> >   
> 
> Idea is to have mdev alias as optional.
> Each mdev_parent says whether it wants mdev_core to generate an alias
> or not. So only networking device drivers would set it to true.
> For rest, alias won't be generated, and won't be compared either
> during creation time. User continue to provide only uuid.

Ok

> I am tempted to have alias collision detection only within children
> mdevs of the same parent, but doing so will always mandate to prefix
> in netdev name. And currently we are left with only 3 characters to
> prefix it, so that may not be good either. Hence, I think mdev core
> wide alias is better with 12 characters.

I suppose it depends on the API, if the vendor driver can ask the mdev
core for an alias as part of the device creation process, then it could
manage the netdev namespace for all its devices, choosing how many
characters to use, and fail the creation if it can't meet a uniqueness
requirement.  IOW, mdev-core would always provide a full sha1 and
therefore gets itself out of the uniqueness/collision aspects.

> I do not understand how an extra character reduces collision, if
> that's what you meant.

If the default were for example 3-chars, we might already have device
'abc'.  A collision would expose one more char of the new device, so we
might add device with alias 'abcd'.  I mentioned previously that this
leaves an issue for userspace that we can't change the alias of device
abc, so without additional information, userspace can only determine via
elimination the mapping of alias to device, but userspace has more
information available to it in the form of sysfs links.

> Module options are almost not encouraged
> anymore with other subsystems/drivers.

We don't live in a world of absolutes.  I agree that the defaults
should work in the vast majority of cases.  Requiring a user to twiddle
module options to make things work is undesirable, verging on a bug.  A
module option to enable some specific feature, unsafe condition, or test
that is outside of the typical use case is reasonable, imo.

> For testing collision rate, a sample user space script and sample
> mtty is easy and get us collision count too. We shouldn't put that
> using module option in production kernel. I practically have the code
> ready to play with; Changing 12 to smaller value is easy with module
> reload.
> 
> #define MDEV_ALIAS_LEN 12

If it can't be tested with a shipping binary, it probably won't be
tested.  Thanks,

Alex

Powered by blists - more mailing lists