On Tue, Aug 3, 2021 at 6:24 AM Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote: > > On 8/2/21 5:30 PM, Jonathon Jongsma wrote: > > On Fri, Jul 30, 2021 at 8:01 AM Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote: > >> > >> On 7/30/21 9:48 AM, Michal Prívozník wrote: > >>> On 7/29/21 9:27 PM, Jonathon Jongsma wrote: > >>>> On Thu, Jul 29, 2021 at 1:35 PM Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote: > >>>>> > >>>>> On 7/27/21 4:09 PM, Jonathon Jongsma wrote: > >>>>>> On Tue, Jul 27, 2021 at 3:02 AM Michal Prívozník <mprivozn@xxxxxxxxxx> wrote: > >>>>>>> > >>>>>>> On 7/27/21 12:08 AM, Jonathon Jongsma wrote: > >>>>>>>> On Mon, Jul 26, 2021 at 9:47 AM Michal Prívozník <mprivozn@xxxxxxxxxx> wrote: > >>>>>>>>> > >>>>>>>>> On 7/23/21 6:40 PM, Jonathon Jongsma wrote: > >>>>>>>>>> Unfortunately, mdevctl supports defining more than one mdev with the > >>>>>>>>>> same UUID as long as they have different parent devices. (Only one of > >>>>>>>>>> these devices can be active at any given time). > >>>>>>>>>> > >>>>>>>>>> This means that we can't use the UUID alone as a way to uniquely > >>>>>>>>>> identify mdev node devices. Append the parent address to ensure > >>>>>>>>>> uniqueness. For example: > >>>>>>>>>> > >>>>>>>>>> Before: mdev_88a6b868_46bd_4015_8e5b_26107f82da38 > >>>>>>>>>> After: mdev_88a6b868_46bd_4015_8e5b_26107f82da38_0000_00_02_0 > >>>>>>>>>> > >>>>>>>>>> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1979440 > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > >>>>>>>>>> --- > >>>>>>>>>> src/node_device/node_device_driver.c | 3 ++- > >>>>>>>>>> src/node_device/node_device_udev.c | 2 +- > >>>>>>>>>> tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml | 8 ++++---- > >>>>>>>>>> 3 files changed, 7 insertions(+), 6 deletions(-) > >>>>>>>>> > >>>>>>>>> The patch looks good, but for some reason it leaves API breakage > >>>>>>>>> aftertaste. But I guess we don't document anywhere what MDEV <name/> > >>>>>>>>> looks like, do we? IOW - we are free to change it if needed. > >>>>>>>> > >>>>>>>> > >>>>>>>> That is true -- it does have a bit of a bad aftertaste. As far as I > >>>>>>>> know, we haven't documented or promised any specific naming format for > >>>>>>>> mdevs. But it could certainly cause some pain for people that are > >>>>>>>> using mdevs already, which might be reason enough to reject or adjust > >>>>>>>> this patch. If a person already has assigned a mdev device to their vm > >>>>>>>> with the old name, the name would change when upgrading libvirt. That > >>>>>>>> could make the domain definition become invalid. > >>>>>>>> > >>>>>>>> But, we have to deal with this situation somehow. We don't have a > >>>>>>>> choice but to handle the case where mdevctl might return multiple > >>>>>>>> defined devices with the same UUID. I considered a couple of other > >>>>>>>> approaches to handling this that I rejected, such as > >>>>>>>> - only add a suffix to the second such device with the same UUID > >>>>>>>> - rejected because the name of a given device depends on the order > >>>>>>>> in which it was observed and the presence of other devices. For > >>>>>>>> example, if we have two devices: mdev_$UUID and mdev_$UUID_$PARENT, > >>>>>>>> and the first device was undefined, then when libvirt was restarted, > >>>>>>>> mdev_$UUID_$PARENT would change its name to mdev_$UUID because it is > >>>>>>>> now the only device with that UUID > >>>>>>> > >>>>>>> Agreed, this would not make situation any better, in fact worse. > >>>>>>> > >>>>>>>> - name all *active* mdevs mdev_$UUID and use a different scheme for > >>>>>>>> inactive, defined mdevs > >>>>>>>> - rejected because the device would change names when it changed > >>>>>>>> from inactive to active. > >>>>>>> > >>>>>>> Yeah, this is equally horrible. > >>>>>>> > >>>>>>>> - ignore this situation because probably almost nobody uses multiple > >>>>>>>> devices with the same UUID > >>>>>>>> - but do they? > >>>>>>> > >>>>>>> Frankly, I don't have enough experience with MDEVs, but since it's > >>>>>>> possible to define an MDEV with an existing UUID, it is possible to > >>>>>>> define it also for the same parent? I mean, if I'd have an MDEV capable > >>>>>>> graphics card and I'd define two MDEVs with the same UUID they would > >>>>>>> both have the same parent, wouldn't they? Because in that case, > >>>>>>> appending PCI address to libvirt's name is not enough. > >>>>>> > >>>>>> > >>>>>> Nope, it's the opposite in fact. mdevctl allows you to define two or > >>>>>> more mdevs with the same UUID, but only if they have different > >>>>>> parents. Only a single mdev with a given UUID can be activated at the > >>>>>> same time. So among *active* devices, the UUID is indeed unique. But > >>>>>> not among persistent device definitions. For defined devices, the UUID > >>>>>> is guaranteed unique per parent. > >>>>>> > >>>>>> I discussed this with Alex a while ago and he thought that the reason > >>>>>> that mdevctl supported this was to allow people to assign an mdev with > >>>>>> a specific UUID to a vm, but that UUID could represent one of several > >>>>>> (equivalent?) devices, depending on which parent device was present in > >>>>>> the host or which device was instantiated. That feature doesn't seem > >>>>>> very compelling to me, and Alex wasn't sure that anybody was actually > >>>>>> using it that way. But the fact remains that mdevctl does allow people > >>>>>> to configure things this way and it seems like we need to be prepared > >>>>>> to deal with it. > >>>>> > >>>>> > >>>>> I am questioning how a user is supposed to correlate the nodedev > >>>>> representation with mdev_{uuid}_{parent} to a domain using the mdev like > >>>>> <devices> > >>>>> <hostdev mode='subsystem' type='mdev' model='vfio-ccw'> > >>>>> <source> > >>>>> <address uuid='{uuid}'/> > >>>>> </source> > >>>>> </hostdev> > >>>>> </devices> > >>>> > >>>> Sorry for potentially confusing the impact of this change with my > >>>> previous comments. I had said that domains with mdevs assigned might > >>>> become invalid if the nodedev name changes. But that's not actually > >>>> true. The naming that we're discussing is only related to the name of > >>>> the device within the libvirt nodedev driver. When adding a mediated > >>>> device to a domain, you don't use this nodedev name, you use the UUID > >>>> directly. This is similar to the way you add a physical passthrough > >>>> PCI device to a domain using its PCI address rather than its > >>>> 'pci_$domain_$bus_$slot_$fn' nodedev name. > >>>> > >>>>> Reading uuid it tells me its universal unique > >>>>> (https://en.wikipedia.org/wiki/Universally_unique_identifier) but now > >>>>> this is not true for definitions which breaks the term universal in my > >>>>> small universe since for the defined state of an mdev it becomes > >>>>> "parental" unique... > >>>> > >>>> Yes, that's true. Which is why I think this is a questionable feature. But: > >>>> A) defined devices are only *potential* devices.This is not all that > >>>> different from the fact that a user can manually instantiate a mdev on > >>>> parent A with a specific UUID and then later instantiate a totally > >>>> different mdev on parent B with the exact same UUID. One UUID refers > >>>> to different devices at different times. > >>>> B) the question we're discussing is not whether mdevctl should allow > >>>> duplicate UUIDs or not. mdevctl *already* allows it, so we need to > >>>> figure out how to deal with it in libvirt. > >>>> > >>>> I suppose one potential possibility might be to change upstream > >>>> mdevctl to disallow duplicate UUIDs and then require this new mdevctl > >>>> version for libvirt. But that's a discussion we'd have to have in > >>>> upstream mdevctl first. > >>> > >>> Maybe we can start the discussion, but even in libvirt you are allowed > >>> to have two guests with the same name and UUID if they are in different > >>> drivers (e.g. one in QEMU and one in LXC). > >>> > >>> Meanwhile, I think these can be pushed as we don't have much options anyway. > >>> > >>> Michal > >>> > >> > >> I think that we have at least one option. > >> We could rethink the approach to create nodedev objects based on mdevctl > >> mdev definitions which actually are not a usable host resource for domains. > > > > If I understand your suggestion, I don't think it solves any problems. > > It seems to me that you're saying that libvirt should ignore all mdev > > definitions from mdevctl if the parent does not (yet?) exist on the > > host. (At least that's my interpretation of your phrase "definitions > > which are not a usable host resource"). But it's still possible to > > have definitions with duplicate UUIDs for devices which have parents > > that *do* exist on the host but are simply not instantiated yet. If > > I'm misinterpreting your comment, feel free to clarify. > > I used to think of the nodedev objects as libvirt showing the user its > view of devices it can understand in the sysfs. This changed when mdev > nodedev objects got created based on mdevctl's mediated device definition. > These definition objects are different from the sysfs objects. Yes, but in a similar way, a defined storage pool is different from an active storage pool. Or a defined domain that is different from an active domain. There is plenty of precedent in libvirt of this kind of distinction. > One difference is state active/inactive which caused e.g. the > introduction of the option "inactive" on list operation. > When we now talk about an nodedev mdev object it could be > a) an instantiated mediated devices without mdev definition, > b) an instantiated mediated devices with mdev definition or > c) just an mdev definition, > but just an instantiated mediated device [part of a) and b)] is actually > a host resource one can use in a domain. > So what I meant by "...not usable host resources" was the definition > itself is not usable for a domain. The sysfs based host resources are. Those three options are also true of pretty much every other libvirt object as well. A domain can be transient or defined, active or inactive. Same for pools. I can define a domain in libvirt that uses devices that are not (currently) present on the host system. I can define a storage pool that refers to a nonexistent directory. When I attempt to instantiate these objects, libvirt will return an error, but I am allowed to define objects that refer to unavailable host resources. So all of those things mentioned above seem pretty analogous to existing libvirt objects. > I agree that mdev definitions and operations on these definitions have a > value but I am getting the feeling that they do not fit so well into the > nodedev objects especially with the new twist of the loss of identity > uniqueness which results from my perspective in a very messy nodedev > object space. I don't think we've lost any identity uniqueness. It's just that we initially chose a device name schema for mdevs that turned out to not be unique when mdevctl is used as a backend. Also, I don't think this nodedev object space is any more "messy" than the other libvirt objects (e.g. pools and domains mentioned above). Are you arguing that we should not provide a way to define new persistent nodedevs in libvirt? > There is one other thing I noticed. I can change an mdev definition > after I initiated/started the mdev definition. This is kind of starting > a domain and afterwards change/edit its persisted definition. So when > dumping the xml one would need to be able to chose running/started or > defined. This currently is not a big issue since there are not many > actually no attributes that can be retrieved via mdevctl from a > running/started mdev but it just adds to the disparity since currently > the user of nodedev gets the mdev definition and running/started mdev in > a merged nodedev object. Again, this is not all that different from existing libvirt objects. I can also edit a persistent definition of a storage pool after it is started, for example. It's possible that there are some issues unique to mdevs that I need to fix in this area, but those are just details. In general it seems pretty consistent with other libvirt objects. Jonathon
