-----Original Message----- From: Erik Skultety [mailto:eskultet@redhat.com] Sent: Thursday, February 01, 2018 4:09 PM To: Wuzongyong (Euler Dept) <cordius.wu(a)huawei.com&gt; Cc: Alex Williamson <alex.williamson(a)redhat.com>; libvir-list(a)redhat.com; weijinfen <weijinfen(a)huawei.com&gt; Subject: Re: [libvirt][Question] nodedev-list --cap return nothing after creating a mdev On Thu, Feb 01, 2018 at 07:19:45AM +0000, Wuzongyong (Euler Dept) wrote: > Hi, > > I meet a problem that libvirt handle new mediated devices wrongly. > Command `virsh nodedev-list -cap mdev` return none after I create a mdev, and the root cause I found is: > > // kernel code > int mdev_device_create(struct kobject *kobj, struct device *dev, > uuid_le uuid) { ... > ret = device_register(&mdev->dev); // send udev event here --- 1 > if (ret) { > put_device(&mdev->dev); > goto create_err; > } > > ret = mdev_device_create_ops(kobj, mdev); > if (ret) > goto create_failed; > > ret = mdev_create_sysfs_files(&mdev->dev, type); // create mdev_type symbol link here --- 2 > if (ret) { > mdev_device_remove_ops(mdev, true); > goto create_failed; > } > > mdev->type_kobj = kobj; > dev_dbg(&mdev->dev, "MDEV: created\n"); ... > } > > At point 1, kernel send a udev event(add). If libvirt receive this > event and handle it before kernel code reach point 2, libvirt just fail to resolve link such as '/sys/devices/pci0000:80/0000:80:02.0/0000:81:00.0/36b79575-ada8-4406- 893d-8cfd8d10a984/mdev_type' > because the link haven't been created yet. Then libvirt thinks this devices doesn't exsit and stop tracking info of this device. > Finally, command `vish nodedev-list -cap` return nothing though the device exist at host indeed. > > To make it easy to reproduce this problem, I just add a `udelay(1)` before point 2 by kprobe. > > So I think maybe we should add 'kobject_uevent(..., KOBJ_ADD)' after point 2 instead of point 1 ? > Or is there any other method to solve this problem? Hi, just a note, we worked around this in upstream libvirt already. Thanks for the pointer though, I'd also be interested in possible solutions in kernel. Erik

On Thu, Feb 01, 2018 at 09:08:59AM +0000, Wuzongyong (Euler Dept) wrote: > Hi, > > I solve this problem by sending a CHANGE event to userspace after creating the symbol link like sriov driver. > > diff --git a/drivers/vfio/mdev/mdev_core.c > b/drivers/vfio/mdev/mdev_core.c index 1269910..a23f1d7 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -351,6 +351,8 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid) > goto create_failed; > } > > + kobject_uevent(&(mdev->dev.kobj), KOBJ_CHANGE); > + > mdev->type_kobj = kobj; > dev_dbg(&mdev->dev, "MDEV: created\n"); > > Is this patch ok? I'm not really familiar with kernel, but from a layman's point of view, the ADD and CHANGE event should be clearly distinguished, but here you expect the CHANGE event to be the result of creating the sysfs entry, whereas in general you'd expect this from the ADD event, since it's really the object has just been created for the first time, right? Erik

> > Thanks, > Zongyong Wu > > > > -----Original Message----- > > From: Erik Skultety [mailto:eskultet@redhat.com] > > Sent: Thursday, February 01, 2018 4:09 PM > > To: Wuzongyong (Euler Dept) <cordius.wu(a)huawei.com&gt; > > Cc: Alex Williamson <alex.williamson(a)redhat.com>; > > libvir-list(a)redhat.com; weijinfen <weijinfen(a)huawei.com&gt; > > Subject: Re: [libvirt][Question] nodedev-list --cap return nothing > > after creating a mdev > > > > On Thu, Feb 01, 2018 at 07:19:45AM +0000, Wuzongyong (Euler Dept) wrote: > > > Hi, > > > > > > I meet a problem that libvirt handle new mediated devices wrongly. > > > Command `virsh nodedev-list -cap mdev` return none after I create > > > a mdev, > > and the root cause I found is: > > > > > > // kernel code > > > int mdev_device_create(struct kobject *kobj, struct device *dev, > > > uuid_le uuid) { ... > > > ret = device_register(&mdev->dev); // send udev event here --- 1 > > > if (ret) { > > > put_device(&mdev->dev); > > > goto create_err; > > > } > > > > > > ret = mdev_device_create_ops(kobj, mdev); > > > if (ret) > > > goto create_failed; > > > > > > ret = mdev_create_sysfs_files(&mdev->dev, type); // create > > mdev_type symbol link here --- 2 > > > if (ret) { > > > mdev_device_remove_ops(mdev, true); > > > goto create_failed; > > > } > > > > > > mdev->type_kobj = kobj; > > > dev_dbg(&mdev->dev, "MDEV: created\n"); ... > > > } > > > > > > At point 1, kernel send a udev event(add). If libvirt receive this > > > event and handle it before kernel code reach point 2, libvirt just > > > fail > > to resolve link such as > > '/sys/devices/pci0000:80/0000:80:02.0/0000:81:00.0/36b79575-ada8-440 > > 6- > > 893d-8cfd8d10a984/mdev_type' > > > because the link haven't been created yet. Then libvirt thinks > > > this > > devices doesn't exsit and stop tracking info of this device. > > > Finally, command `vish nodedev-list -cap` return nothing though > > > the > > device exist at host indeed. > > > > > > To make it easy to reproduce this problem, I just add a > > > `udelay(1)` > > before point 2 by kprobe. > > > > > > So I think maybe we should add 'kobject_uevent(..., KOBJ_ADD)' > > > after > > point 2 instead of point 1 ? > > > Or is there any other method to solve this problem? > > > > Hi, > > just a note, we worked around this in upstream libvirt already. > > > > Thanks for the pointer though, I'd also be interested in possible > > solutions in kernel. > > > > Erik

On Thu, Feb 01, 2018 at 12:03:42PM +0000, Wuzongyong (Euler Dept) wrote: > a mdev > > > > On Thu, Feb 01, 2018 at 09:08:59AM +0000, Wuzongyong (Euler Dept) wrote: > > > Hi, > > > > > > I solve this problem by sending a CHANGE event to userspace after > > creating the symbol link like sriov driver. > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > b/drivers/vfio/mdev/mdev_core.c index 1269910..a23f1d7 100644 > > > --- a/drivers/vfio/mdev/mdev_core.c > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > @@ -351,6 +351,8 @@ int mdev_device_create(struct kobject *kobj, > > > struct > > device *dev, uuid_le uuid) > > > goto create_failed; > > > } > > > > > > + kobject_uevent(&(mdev->dev.kobj), KOBJ_CHANGE); > > > + > > > mdev->type_kobj = kobj; > > > dev_dbg(&mdev->dev, "MDEV: created\n"); > > > > > > Is this patch ok? > > > > I'm not really familiar with kernel, but from a layman's point of > > view, the ADD and CHANGE event should be clearly distinguished, but > > here you expect the CHANGE event to be the result of creating the > > sysfs entry, whereas in general you'd expect this from the ADD > > event, since it's really the object has just been created for the first time, right? > > > > Erik > > > > The ADD event has been sended before sending CHANGE event. And before > sending ADD event, the mdev sysfs file node and some general attribute > such as power/uevent has been created, but the mdev_type file(link > libvirt want to resolve) has not been created yet. So I think maybe we > should send a CHANGE event to tell libvirt this link has been created so libvirt can resolve it correctly. Well, as much beneficial as getting an event from udev can be, it wouldn't change anything within libvirt really, since when we get the ADD event, we try to wait for the link to appear and only then continue. With the CHANGE event we'll update the device again. I'm only saying it because we'll be supporting running upstream on kernels prior to this potential change/fix, so it will take "a few" years until libvirt is going to solely rely on udev events here. The commit ID you asked in the other mail is 1af45804. Erik