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

[Jiri Pirko]

Thu, Aug 22, 2019 at 12:04:02PM CEST, parav@xxxxxxxxxxxx wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@xxxxxxxxxxx>
>> Sent: Thursday, August 22, 2019 3:28 PM
>> To: Parav Pandit <parav@xxxxxxxxxxxx>
>> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>; Jiri Pirko
>> <jiri@xxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Kirti
>> Wankhede <kwankhede@xxxxxxxxxx>; Cornelia Huck <cohuck@xxxxxxxxxx>;
>> kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; cjia <cjia@xxxxxxxxxx>;
>> netdev@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
>> 
>> Thu, Aug 22, 2019 at 11:42:13AM CEST, parav@xxxxxxxxxxxx wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Jiri Pirko <jiri@xxxxxxxxxxx>
>> >> Sent: Thursday, August 22, 2019 2:59 PM
>> >> To: Parav Pandit <parav@xxxxxxxxxxxx>
>> >> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>; Jiri Pirko
>> >> <jiri@xxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Kirti
>> >> Wankhede <kwankhede@xxxxxxxxxx>; Cornelia Huck
>> <cohuck@xxxxxxxxxx>;
>> >> kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; cjia
>> >> <cjia@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx
>> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
>> >>
>> >> Wed, Aug 21, 2019 at 08:23:17AM CEST, parav@xxxxxxxxxxxx wrote:
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
>> >> >> Sent: Wednesday, August 21, 2019 10:56 AM
>> >> >> To: Parav Pandit <parav@xxxxxxxxxxxx>
>> >> >> Cc: Jiri Pirko <jiri@xxxxxxxxxxxx>; David S . Miller
>> >> >> <davem@xxxxxxxxxxxxx>; Kirti Wankhede <kwankhede@xxxxxxxxxx>;
>> >> >> Cornelia Huck <cohuck@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx;
>> >> >> linux-kernel@xxxxxxxxxxxxxxx; cjia <cjia@xxxxxxxxxx>;
>> >> >> netdev@xxxxxxxxxxxxxxx
>> >> >> 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.

>
>
>> in this case that it does
>> not provide 2 same attrs for 2 different ports.
>> Hmm, so the order of creation matters. That is not good.
>> 
>> >>
>> >> >I took Ethernet mdev as an example.
>> >> >New prefix 'm' stands for mediated device.
>> >> >Remaining 12 characters are first 12 chars of the mdev alias.
>> >>
>> >> Does this resolve the identification of devlink port representor?
>> >Not sure if I understood your question correctly, attemping to answer below.
>> >phys_port_name of devlink port is defined by the first 12 characters of mdev
>> alias.
>> >> I assume you want to use the same 12(or so) chars, don't you?
>> >Mdev's netdev will also use the same mdev alias from the sysfs to rename
>> netdev name from ethX to enm<mdev_alias>, where en=Etherenet, m=mdev.
>> >
>> >So yes, same 12 characters are use for mdev's netdev and mdev devlink port's
>> phys_port_name.
>> >
>> >Is that what are you asking?
>> 
>> Yes. Then you have 3 chars to handle the rest of the name (pci, pf)...