> -----Original Message----- > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > Sent: Saturday, August 24, 2019 10:29 AM > To: Parav Pandit <parav@xxxxxxxxxxxx> > Cc: Jiri Pirko <jiri@xxxxxxxxxxx>; 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 > > On Sat, 24 Aug 2019 03:56:08 +0000 > Parav Pandit <parav@xxxxxxxxxxxx> wrote: > > > > -----Original Message----- > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > > > Sent: Saturday, August 24, 2019 1:14 AM > > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > > Cc: Jiri Pirko <jiri@xxxxxxxxxxx>; 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 > > > > > > On Fri, 23 Aug 2019 18:00:30 +0000 > > > Parav Pandit <parav@xxxxxxxxxxxx> wrote: > > > > > > > > -----Original Message----- > > > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > > > > > Sent: Friday, August 23, 2019 10:47 PM > > > > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > > > > Cc: Jiri Pirko <jiri@xxxxxxxxxxx>; 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 > > > > > > > > > > On Fri, 23 Aug 2019 16:14:04 +0000 Parav Pandit > > > > > <parav@xxxxxxxxxxxx> wrote: > > > > > > > > > > > > > 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. > > > > > > > > > > > > > This doesn't work. At mdev core level 20 bytes sha1 are > > > > > > unique, so mdev core allowed to create a mdev. > > > > > > > > > > The mdev vendor driver has the opportunity to fail the device > > > > > creation in mdev_parent_ops.create(). > > > > > > > > > That is not helpful for below reasons. > > > > 1. vendor driver doesn't have visibility in other vendor's alias. > > > > 2. Even for single vendor, it needs to maintain global list of > > > > devices to see > > > collision. > > > > 3. multiple vendors needs to implement same scheme. > > > > > > > > Mdev core should be the owner. Shifting ownership from one layer > > > > to a lower layer in vendor driver doesn't solve the problem (if > > > > there is one, which I think doesn't exist). > > > > > > > > > > And then devlink core chooses > > > > > > only 6 bytes (12 characters) and there is collision. Things > > > > > > fall apart. Since mdev provides unique uuid based scheme, it's > > > > > > the mdev core's ownership to provide unique aliases. > > > > > > > > > > You're suggesting/contemplating multiple solutions here, 3-char > > > > > prefix + 12- char sha1 vs <parent netdev> + ?-char sha1. Also, > > > > > the 15-char total limit is imposed by an external subsystem, > > > > > where the vendor driver is the gateway between that subsystem > > > > > and mdev. How would mdev integrate with another subsystem that > > > > > maybe only has 9-chars available? Would the vendor driver API > > > > > specify "I need an alias" or would it specify "I need an X-char length > alias"? > > > > Yes, Vendor driver should say how long the alias it wants. > > > > However before we implement that, I suggest let such > > > > vendor/user/driver arrive which needs that. Such variable length > > > > alias can be added at that time and even with that alias collision > > > > can be detected by single mdev module. > > > > > > If we agree that different alias lengths are possible, then I would > > > request that minimally an mdev sample driver be modified to request > > > an alias with a length that can be adjusted without recompiling in order > to exercise the collision path. > > > > > Yes. this can be done. But I fail to understand the need to do so. > > It is not the responsibility of the mdev core to show case sha1 > > collision efficiency/deficiency. So why do you insist exercise it? > > I don't understand what you're trying to imply with "show case sha1 collision > efficiency/deficiency". Are you suggesting that I'm asking for this feature to > experimentally test the probability of collisions at different character > lengths? We can use shell scripts for that. > I'm simply observing that collisions are possible based on user input, but > they're not practical to test for at the character lengths we're using. > Therefore, how do I tell QA to develop a tests to make sure the kernel and > userspace tools that might be involved behave correctly when this rare event > occurs? > Ok. so you want to have code coverage and want to add a knob for that. That is fine. I will have the mdev_parent->ops.alias_len as API instead of bool. And extend mtty module parameter to set the alias length. Unfortunately similar code coverage doesn't exist for API like mdev_get/set_iommu_device() in sample of real vendor driver. And QA is not able to test this functionality without tainting the kernel.
