The event loop may get scheduled earlier than the udev event handler
thread which means that it would keep invoking the handler callback with
"new" events, while in fact it's most likely still the same event which
the handler thread hasn't managed to remove from the socket queue yet.
This is due to unwanted increments of the number of events queuing on the
socket and causes the handler thread to spam the logs with an error about
libudev returning NULL (errno == EAGAIN).

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/node_device/node_device_udev.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 444e5be4d..e3a647e3d 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1697,6 +1697,7 @@ udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
 static void
 udevEventHandleThread(void *opaque)
 {
+    udevPrivate *priv = NULL;
     udevEventThreadDataPtr privateData = opaque;
     struct udev_device *device = NULL;
     struct udev_monitor *udev_monitor = NULL;
@@ -1716,6 +1717,7 @@ udevEventHandleThread(void *opaque)
         virMutexUnlock(&privateData->lock);
 
         nodeDeviceLock();
+        priv = driver->privateData;
         udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
 
         if (!udevCheckMonitorFD(udev_monitor, privateData->monitor_fd)) {
@@ -1726,6 +1728,9 @@ udevEventHandleThread(void *opaque)
         device = udev_monitor_receive_device(udev_monitor);
         nodeDeviceUnlock();
 
+        /* Re-enable polling for new events on the @udev_monitor */
+        virEventUpdateHandle(priv->watch, VIR_EVENT_HANDLE_READABLE);
+
         if (!device) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("udev_monitor_receive_device returned NULL"));
@@ -1751,10 +1756,12 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
                         int events ATTRIBUTE_UNUSED,
                         void *opaque)
 {
+    udevPrivate *priv = NULL;
     struct udev_monitor *udev_monitor = NULL;
     udevEventThreadDataPtr threadData = opaque;
 
     nodeDeviceLock();
+    priv = driver->privateData;
     udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
     if (!udevCheckMonitorFD(udev_monitor, fd)) {
         virMutexLock(&threadData->lock);
@@ -1771,6 +1778,16 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
     threadData->nevents++;
     virCondSignal(&threadData->threadCond);
     virMutexUnlock(&threadData->lock);
+
+    /* Due to scheduling, the eventloop might poll the udev monitor before the
+     * handler thread actually removes the data from the socket, thus causing it
+     * to spam errors about data not being ready yet, because
+     * udevEventHandleCallback would be invoked multiple times incrementing the
+     * counter of events queuing for a single event.
+     * Therefore we need to disable polling on the fd until the thread actually
+     * removes the data from the socket.
+     */
+    virEventUpdateHandle(priv->watch, 0);
 }
 
 
-- 
2.13.3

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

On 08/24/2017 07:23 AM, Erik Skultety wrote:
> The event loop may get scheduled earlier than the udev event handler
> thread which means that it would keep invoking the handler callback with
> "new" events, while in fact it's most likely still the same event which
> the handler thread hasn't managed to remove from the socket queue yet.
> This is due to unwanted increments of the number of events queuing on the
> socket and causes the handler thread to spam the logs with an error about
> libudev returning NULL (errno == EAGAIN).
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

And by disabling the polling couldn't we miss an event? That would be
really bad if the event after the one we miss relies on the event we
missed creating something that the subsequent one would need (if that
makes sense).

John

> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 444e5be4d..e3a647e3d 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1697,6 +1697,7 @@ udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
>  static void
>  udevEventHandleThread(void *opaque)
>  {
> +    udevPrivate *priv = NULL;
>      udevEventThreadDataPtr privateData = opaque;
>      struct udev_device *device = NULL;
>      struct udev_monitor *udev_monitor = NULL;
> @@ -1716,6 +1717,7 @@ udevEventHandleThread(void *opaque)
>          virMutexUnlock(&privateData->lock);
>  
>          nodeDeviceLock();
> +        priv = driver->privateData;
>          udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>  
>          if (!udevCheckMonitorFD(udev_monitor, privateData->monitor_fd)) {
> @@ -1726,6 +1728,9 @@ udevEventHandleThread(void *opaque)
>          device = udev_monitor_receive_device(udev_monitor);
>          nodeDeviceUnlock();
>  
> +        /* Re-enable polling for new events on the @udev_monitor */
> +        virEventUpdateHandle(priv->watch, VIR_EVENT_HANDLE_READABLE);
> +
>          if (!device) {
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                             _("udev_monitor_receive_device returned NULL"));
> @@ -1751,10 +1756,12 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>                          int events ATTRIBUTE_UNUSED,
>                          void *opaque)
>  {
> +    udevPrivate *priv = NULL;
>      struct udev_monitor *udev_monitor = NULL;
>      udevEventThreadDataPtr threadData = opaque;
>  
>      nodeDeviceLock();
> +    priv = driver->privateData;
>      udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>      if (!udevCheckMonitorFD(udev_monitor, fd)) {
>          virMutexLock(&threadData->lock);
> @@ -1771,6 +1778,16 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>      threadData->nevents++;
>      virCondSignal(&threadData->threadCond);
>      virMutexUnlock(&threadData->lock);
> +
> +    /* Due to scheduling, the eventloop might poll the udev monitor before the
> +     * handler thread actually removes the data from the socket, thus causing it
> +     * to spam errors about data not being ready yet, because
> +     * udevEventHandleCallback would be invoked multiple times incrementing the
> +     * counter of events queuing for a single event.
> +     * Therefore we need to disable polling on the fd until the thread actually
> +     * removes the data from the socket.
> +     */
> +    virEventUpdateHandle(priv->watch, 0);
>  }
>  
>  
> 

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

On Mon, Aug 28, 2017 at 12:38:08PM -0400, John Ferlan wrote:
>
>
> On 08/24/2017 07:23 AM, Erik Skultety wrote:
> > The event loop may get scheduled earlier than the udev event handler
> > thread which means that it would keep invoking the handler callback with
> > "new" events, while in fact it's most likely still the same event which
> > the handler thread hasn't managed to remove from the socket queue yet.
> > This is due to unwanted increments of the number of events queuing on the
> > socket and causes the handler thread to spam the logs with an error about
> > libudev returning NULL (errno == EAGAIN).
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
>
> And by disabling the polling couldn't we miss an event? That would be
> really bad if the event after the one we miss relies on the event we
> missed creating something that the subsequent one would need (if that
> makes sense).

No, we just don't poll on the @fd for a moment, but the data has kept being
delivered to the udev monitor and waits for us to be extracted.

Erik

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