Let's move the udevEnumerateDevices into a thread to "speed
up" the initialization process. If the enumeration fails we
can set the Quit flag to ensure that udevEventHandleCallback
will not run.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/node_device/node_device_udev.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 1e1b71742..e0fca6159 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1891,6 +1891,25 @@ udevSetupSystemDev(void)
 }
 
 
+static void
+nodeStateInitializeEnumerate(void *opaque)
+{
+    struct udev *udev = opaque;
+    udevEventDataPtr priv = driver->privateData;
+
+    /* Populate with known devices */
+    if (udevEnumerateDevices(udev) != 0)
+        goto error;
+
+    return;
+
+ error:
+    virObjectLock(priv);
+    priv->threadQuit = true;
+    virObjectUnlock(priv);
+}
+
+
 static int
 udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED)
 {
@@ -1922,6 +1941,7 @@ nodeStateInitialize(bool privileged,
 {
     udevEventDataPtr priv = NULL;
     struct udev *udev = NULL;
+    virThread enumThread;
 
     if (VIR_ALLOC(driver) < 0)
         return -1;
@@ -2002,9 +2022,12 @@ nodeStateInitialize(bool privileged,
     if (udevSetupSystemDev() != 0)
         goto cleanup;
 
-    /* Populate with known devices */
-    if (udevEnumerateDevices(udev) != 0)
+    if (virThreadCreate(&enumThread, false, nodeStateInitializeEnumerate,
+                        udev) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("failed to create udev enumerate thread"));
         goto cleanup;
+    }
 
     return 0;
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Sat, Dec 09, 2017 at 12:29:12PM -0500, John Ferlan wrote:
> Let's move the udevEnumerateDevices into a thread to "speed
> up" the initialization process. If the enumeration fails we
> can set the Quit flag to ensure that udevEventHandleCallback
> will not run.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>

Speedup's okay (even though I don't expect anything significant), but if
something goes wrong with udev, we start into a sort of crippled state where
you're still able to start machines, but only if these don't make use of NPIV
in a sense that a vHBA would need to be created, or managed direct-assigned PCI
devices. Prior to this change if enumeration failed, the daemon terminated - I
think this is an improvement.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 12/14/2017 05:42 AM, Erik Skultety wrote:
> On Sat, Dec 09, 2017 at 12:29:12PM -0500, John Ferlan wrote:
>> Let's move the udevEnumerateDevices into a thread to "speed
>> up" the initialization process. If the enumeration fails we
>> can set the Quit flag to ensure that udevEventHandleCallback
>> will not run.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/node_device/node_device_udev.c | 27 +++++++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>
> 
> Speedup's okay (even though I don't expect anything significant), but if
> something goes wrong with udev, we start into a sort of crippled state where
> you're still able to start machines, but only if these don't make use of NPIV
> in a sense that a vHBA would need to be created, or managed direct-assigned PCI
> devices. Prior to this change if enumeration failed, the daemon terminated - I
> think this is an improvement.
> 
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 

So as a "next step" - do you think it'd be worthwhile to actually
implement the nodeStateReload functionality?  I think we have enough
"capability" now between separate threads for Enumeration and
HandleCallback.  Might be a bit tricky, but also a way to 'restart' a
stopped HandleCallback thread.

I will hold off on pushing for a bit to ensure someone else doesn't have
extreme agita over the enumerate thread though.

Tks -

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Dec 14, 2017 at 08:25:34AM -0500, John Ferlan wrote:
>
>
> On 12/14/2017 05:42 AM, Erik Skultety wrote:
> > On Sat, Dec 09, 2017 at 12:29:12PM -0500, John Ferlan wrote:
> >> Let's move the udevEnumerateDevices into a thread to "speed
> >> up" the initialization process. If the enumeration fails we
> >> can set the Quit flag to ensure that udevEventHandleCallback
> >> will not run.
> >>
> >> Signed-off-by: John Ferlan <jferlan@redhat.com>
> >> ---
> >>  src/node_device/node_device_udev.c | 27 +++++++++++++++++++++++++--
> >>  1 file changed, 25 insertions(+), 2 deletions(-)
> >>
> >
> > Speedup's okay (even though I don't expect anything significant), but if
> > something goes wrong with udev, we start into a sort of crippled state where
> > you're still able to start machines, but only if these don't make use of NPIV
> > in a sense that a vHBA would need to be created, or managed direct-assigned PCI
> > devices. Prior to this change if enumeration failed, the daemon terminated - I
> > think this is an improvement.
> >
> > Reviewed-by: Erik Skultety <eskultet@redhat.com>
> >
>
> So as a "next step" - do you think it'd be worthwhile to actually
> implement the nodeStateReload functionality?  I think we have enough

I wasn't going to suggest that, since there are many other thing on the plate
that need attention, but since you mentioned it, yes, I think it would be
worthwhile to have that kind of functionality. On the other hand, since there's
the resonating incentive on reworking our architecture, we also need to think
with respect to that whether such a change is still going to make sense even
after we make any architectural changes or many things are going to """solve"""
by the nature of it. With that said and not giving not much thought to it, I
don't think StateReload feature would be one of such things.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list