3:23 a.m.
Rather than listening to 'add' udev events, listen for 'bind' events
instead. When we get an 'add' event, the sysfs tree for the device is
often not ready yet. In that case we sleep in a loop until the sysfs
tree appears, or give up after a timeout.
udev added the 'bind' event to give userspace a signal that indicated
when driver-specific attributes were available to be used. In other
words, the sysfs tree *should* be ready and usable at this point.
But just to be safe, we'll leave the wait loop in the code to handle
corner cases, with the hope that it'll never be used.
The udev 'bind' event was added in kernel 4.14 and the oldest platform
we support has kernel 4.18, so it should be safe to make this change.
Previous discussion on the mailing list:
https://listman.redhat.com/archives/libvir-list/2022-August/233933.html
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_udev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 07c10f0d88..781a8b32a6 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1759,7 +1759,7 @@ udevHandleOneDevice(struct udev_device *device)
VIR_DEBUG("udev action: '%s': %s", action,
udev_device_get_syspath(device));
- if (STREQ(action, "add") || STREQ(action, "change"))
+ if (STREQ(action, "bind") || STREQ(action, "change"))
return udevAddOneDevice(device);
if (STREQ(action, "remove"))
--
2.37.2
8:43 p.m.
On 9/20/22 3:23 PM, Jonathon Jongsma wrote:
Rather than listening to 'add' udev events, listen for
'bind' events
instead. When we get an 'add' event, the sysfs tree for the device is
often not ready yet. In that case we sleep in a loop until the sysfs
tree appears, or give up after a timeout.
udev added the 'bind' event to give userspace a signal that indicated
when driver-specific attributes were available to be used. In other
words, the sysfs tree *should* be ready and usable at this point.
But just to be safe, we'll leave the wait loop in the code to handle
corner cases, with the hope that it'll never be used.
The udev 'bind' event was added in kernel 4.14 and the oldest platform
we support has kernel 4.18, so it should be safe to make this change.
Previous discussion on the mailing list:
https://listman.redhat.com/archives/libvir-list/2022-August/233933.html
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
(I'm giving this based on the assumption that you've tried it out with a
few examples of adding devices and they properly show up in virsh
nodedev-list, etc. There is always the chance may be some odd case where
the bind event isn't generated when it should be, but we're never going
to find out by just talking about it :-))
---
src/node_device/node_device_udev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 07c10f0d88..781a8b32a6 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1759,7 +1759,7 @@ udevHandleOneDevice(struct udev_device *device)
VIR_DEBUG("udev action: '%s': %s", action,
udev_device_get_syspath(device));
- if (STREQ(action, "add") || STREQ(action, "change"))
+ if (STREQ(action, "bind") || STREQ(action, "change"))
return udevAddOneDevice(device);
if (STREQ(action, "remove"))
4:59 p.m.
On Tue, Sep 20, 2022 at 02:23:23PM -0500, Jonathon Jongsma wrote:
Rather than listening to 'add' udev events, listen for
'bind' events
instead. When we get an 'add' event, the sysfs tree for the device is
often not ready yet. In that case we sleep in a loop until the sysfs
tree appears, or give up after a timeout.
udev added the 'bind' event to give userspace a signal that indicated
when driver-specific attributes were available to be used. In other
words, the sysfs tree *should* be ready and usable at this point.
But just to be safe, we'll leave the wait loop in the code to handle
corner cases, with the hope that it'll never be used.
The udev 'bind' event was added in kernel 4.14 and the oldest platform
we support has kernel 4.18, so it should be safe to make this change.
Previous discussion on the mailing list:
https://listman.redhat.com/archives/libvir-list/2022-August/233933.html
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
Unfortunately I don't have an mdev-configured machine at hand anymore to try
this out, but based on the trivial change and assuming you've at least tried
the patch, you can have my:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
12:41 a.m.
On 9/22/22 3:59 AM, Erik Skultety wrote:
On Tue, Sep 20, 2022 at 02:23:23PM -0500, Jonathon Jongsma wrote:
> Rather than listening to 'add' udev events, listen for 'bind' events
> instead. When we get an 'add' event, the sysfs tree for the device is
> often not ready yet. In that case we sleep in a loop until the sysfs
> tree appears, or give up after a timeout.
>
> udev added the 'bind' event to give userspace a signal that indicated
> when driver-specific attributes were available to be used. In other
> words, the sysfs tree *should* be ready and usable at this point.
> But just to be safe, we'll leave the wait loop in the code to handle
> corner cases, with the hope that it'll never be used.
>
> The udev 'bind' event was added in kernel 4.14 and the oldest platform
> we support has kernel 4.18, so it should be safe to make this change.
>
> Previous discussion on the mailing list:
> https://listman.redhat.com/archives/libvir-list/2022-August/233933.html
>
> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
> ---
Unfortunately I don't have an mdev-configured machine at hand anymore to try
this out, but based on the trivial change and assuming you've at least tried
the patch, you can have my:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
I have tried it with several devices including usb and mdev.
But I'm no longer sure whether this is a safe change. For example,
imagine I have a device configured to not load a driver (e.g. perhaps
something like `driverctl set-override $DEVICE none`). If this device is
hotplugged, we should get a udev 'add' event but no 'bind' event. So,
previously this driverless device would have showed up in `virsh
nodedev-list`, but after this change it will not.
Also note that this creates a difference between startup and hotplug. At
libvirt startup we enumerate all devices and add them to our nodedev
list. The enumerated devices will include devices both with bound
drivers and those without. But if we only listen to the 'bind' event,
hotplugged devices will need drivers bound to be added to the list. So
if you hotplug a driverless device, it will not show up in the nodedev
list until libvirt is restarted.
Note that the above description is based only on my analysis of the
code, not testing.
I'm inclined to withdraw this patch.
Jonathon
3:46 a.m.
On 9/22/22 12:41 PM, Jonathon Jongsma wrote:
On 9/22/22 3:59 AM, Erik Skultety wrote:
> On Tue, Sep 20, 2022 at 02:23:23PM -0500, Jonathon Jongsma wrote:
>> Rather than listening to 'add' udev events, listen for 'bind'
events
>> instead. When we get an 'add' event, the sysfs tree for the device is
>> often not ready yet. In that case we sleep in a loop until the sysfs
>> tree appears, or give up after a timeout.
>>
>> udev added the 'bind' event to give userspace a signal that indicated
>> when driver-specific attributes were available to be used. In other
>> words, the sysfs tree *should* be ready and usable at this point.
>> But just to be safe, we'll leave the wait loop in the code to handle
>> corner cases, with the hope that it'll never be used.
>>
>> The udev 'bind' event was added in kernel 4.14 and the oldest platform
>> we support has kernel 4.18, so it should be safe to make this change.
>>
>> Previous discussion on the mailing list:
>> https://listman.redhat.com/archives/libvir-list/2022-August/233933.html
>>
>> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
>> ---
>
> Unfortunately I don't have an mdev-configured machine at hand anymore
> to try
> this out, but based on the trivial change and assuming you've at least
> tried
> the patch, you can have my:
>
> Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
>
I have tried it with several devices including usb and mdev.
But I'm no longer sure whether this is a safe change. For example,
imagine I have a device configured to not load a driver (e.g. perhaps
something like `driverctl set-override $DEVICE none`). If this device is
hotplugged, we should get a udev 'add' event but no 'bind' event. So,
previously this driverless device would have showed up in `virsh
nodedev-list`, but after this change it will not.
Also note that this creates a difference between startup and hotplug. At
libvirt startup we enumerate all devices and add them to our nodedev
list. The enumerated devices will include devices both with bound
drivers and those without. But if we only listen to the 'bind' event,
hotplugged devices will need drivers bound to be added to the list. So
if you hotplug a driverless device, it will not show up in the nodedev
list until libvirt is restarted.
Hmm. Good catch - this is one of those things I would have just ended up
finding out later on :-). Maybe we need to listen for both events, and
as each one is received, we populate what we can of the entry in the
device list?
(On one hand, there's nothing that libvirt can directly do with a device
that has no binding to a driver; on the other hand, it's not just
libvirt looking at the list of devices - someone may specifically want
their devices to have no driver when the host boots, but then wants to
discover them from the nodedev list and bind them to vfio-pci at runtime.)
Note that the above description is based only on my analysis of the
code, not testing.
I'm inclined to withdraw this patch.
Jonathon
755
days inactive
758
days old
4 comments
3 participants
participants (3)
-
Erik Skultety -
Jonathon Jongsma -
Laine Stump