Re: [PATCH v4 4/7] udev: Convert udevEventHandleThread to an actual thread routine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 09/18/2017 12:34 PM, Erik Skultety wrote:
> Adjust udevEventHandleThread to be a proper thread routine running in an
> infinite loop handling devices. Also introduce udevEventThreadData
> private structure.
> Every time there's and incoming event from udev, udevEventHandleCallback
> only increments the number of events queuing on the monitor and signals
> the worker thread to handle them.
> 
> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
>  src/node_device/node_device_udev.c | 125 +++++++++++++++++++++++++++++++------
>  1 file changed, 107 insertions(+), 18 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index e144472f1..96760d1e4 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -59,6 +59,54 @@ struct _udevPrivate {
>      bool privileged;
>  };
>  
> +typedef struct _udevEventThreadData udevEventThreadData;
> +typedef udevEventThreadData *udevEventThreadDataPtr;
> +
> +struct _udevEventThreadData {
> +    virMutex lock;
> +    virCond threadCond;
> +
> +    bool threadQuit;
> +    int monitor_fd;
> +    unsigned long long nevents; /* number of udev events queuing on monitor */
> +};
> +
> +
> +static udevEventThreadDataPtr
> +udevEventThreadDataNew(int fd)
> +{
> +    udevEventThreadDataPtr ret = NULL;
> +
> +    if (VIR_ALLOC_QUIET(ret) < 0)
> +        return NULL;
> +
> +    if (virMutexInit(&ret->lock) < 0)
> +        goto cleanup;
> +
> +    if (virCondInit(&ret->threadCond) < 0)
> +        goto cleanup_mutex;
> +
> +    ret->monitor_fd = fd;
> +
> +    return ret;
> +
> + cleanup_mutex:
> +    virMutexDestroy(&ret->lock);
> +
> + cleanup:
> +    VIR_FREE(ret);
> +    return NULL;
> +}
> +
> +
> +static void
> +udevEventThreadDataFree(udevEventThreadDataPtr data)
> +{
> +    virMutexDestroy(&data->lock);
> +    virCondDestroy(&data->threadCond);
> +    VIR_FREE(data);
> +}
> +
>  
>  static bool
>  udevHasDeviceProperty(struct udev_device *dev,
> @@ -1648,29 +1696,51 @@ udevEventCheckMonitorFD(struct udev_monitor *udev_monitor,
>  static void
>  udevEventHandleThread(void *opaque)
>  {
> +    udevEventThreadDataPtr privateData = opaque;
>      struct udev_device *device = NULL;
>      struct udev_monitor *udev_monitor = NULL;
> -    int fd = (intptr_t) opaque;
>  
> -    nodeDeviceLock();
> -    udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> +    /* continue rather than break from the loop on non-fatal errors */
^^
This comment could go to [1]

> +    while (1) {
> +        virMutexLock(&privateData->lock);
> +        while (privateData->nevents == 0 && !privateData->threadQuit) {
> +            if (virCondWait(&privateData->threadCond, &privateData->lock)) {
> +                virReportSystemError(errno, "%s",
> +                                     _("handler failed to wait on condition"));

Is a virMutexUnlock required before eventually calling virMutexDestroy...

> +                goto cleanup;
> +            }
> +        }
>  
> -    if (!udevEventCheckMonitorFD(udev_monitor, fd)) {
> +        privateData->nevents--;
> +        virMutexUnlock(&privateData->lock);

If we get here, then either nevents > 0 || threadQuit == true, but we
don't check for threadQuit here before the fetch/check of monitor_fd,
e.g. the reason for threadQuit = true, so although the following
udev_monitor check "works", the question thus becomes is it necessary if
threadQuit == true?  I suppose it could be, but we could also jump to
cleanup if threadQuit == true || !udevEventCheckMonitorFD

> +
> +        nodeDeviceLock();
> +        udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> +
> +        if (!udevEventCheckMonitorFD(udev_monitor, privateData->monitor_fd)) {

This accesses privateData->monitor_fd without the mutex. So we don't
have too many lock/unlock - consider a local @monitor_fd which is
fetched while the lock is held.

> +            nodeDeviceUnlock();
> +            goto cleanup;
> +        }
> +
> +        device = udev_monitor_receive_device(udev_monitor);
>          nodeDeviceUnlock();
> -        return;
> -    }
>  
> -    device = udev_monitor_receive_device(udev_monitor);
> -    nodeDeviceUnlock();

[1]  could move the comment here since that's what I believe it's meant
to describe...

> +        if (!device) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("udev_monitor_receive_device returned NULL"));

Perhaps a VIR_WARN? Doesn't perhaps really matter, but it's not an error
it's just a condition we didn't expect that we're continuing on...


> +            goto next;

This should just be a continue; instead of needing next... Not clear
what happens if udev_device_unref(NULL) is called.

> +        }
> +
> +        udevHandleOneDevice(device);
>  
> -    if (!device) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("udev_monitor_receive_device returned NULL"));
> -        return;
> +    next:
> +        udev_device_unref(device);
>      }
>  
> -    udevHandleOneDevice(device);
> + cleanup:
>      udev_device_unref(device);

Should this be:

    if (device)
        udev_device_unref(device)


I think the cleanups are obvious, so

Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>

John

> +    udevEventThreadDataFree(privateData);
> +    return;
>  }
>  
>  
> @@ -1678,20 +1748,29 @@ static void
>  udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>                          int fd,
>                          int events ATTRIBUTE_UNUSED,
> -                        void *data ATTRIBUTE_UNUSED)
> +                        void *opaque)
>  {
>      struct udev_monitor *udev_monitor = NULL;
> +    udevEventThreadDataPtr threadData = opaque;
>  
>      nodeDeviceLock();
>      udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>  
>      if (!udevEventCheckMonitorFD(udev_monitor, fd)) {
> +        virMutexLock(&threadData->lock);
> +        threadData->threadQuit = true;
> +        virCondSignal(&threadData->threadCond);
> +        virMutexUnlock(&threadData->lock);
> +
>          nodeDeviceUnlock();
>          return;
>      }
>      nodeDeviceUnlock();
>  
> -    udevEventHandleThread((void *)(intptr_t) fd);
> +    virMutexLock(&threadData->lock);
> +    threadData->nevents++;
> +    virCondSignal(&threadData->threadCond);
> +    virMutexUnlock(&threadData->lock);
>  }
>  
>  
> @@ -1818,6 +1897,9 @@ nodeStateInitialize(bool privileged,
>  {
>      udevPrivate *priv = NULL;
>      struct udev *udev = NULL;
> +    int monitor_fd = -1;
> +    virThread th;
> +    udevEventThreadDataPtr threadData = NULL;
>  
>      if (VIR_ALLOC(priv) < 0)
>          return -1;
> @@ -1878,6 +1960,14 @@ nodeStateInitialize(bool privileged,
>                                               128 * 1024 * 1024);
>  #endif
>  
> +    monitor_fd = udev_monitor_get_fd(priv->udev_monitor);
> +    if (!(threadData = udevEventThreadDataNew(monitor_fd)) ||
> +        virThreadCreate(&th, false, udevEventHandleThread, threadData) < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("failed to create udev handling thread"));
> +        goto cleanup;
> +    }
> +
>      /* We register the monitor with the event callback so we are
>       * notified by udev of device changes before we enumerate existing
>       * devices because libvirt will simply recreate the device if we
> @@ -1886,9 +1976,8 @@ nodeStateInitialize(bool privileged,
>       * enumeration.  The alternative is to register the callback after
>       * we enumerate, in which case we will fail to create any devices
>       * that appear while the enumeration is taking place.  */
> -    priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor),
> -                                    VIR_EVENT_HANDLE_READABLE,
> -                                    udevEventHandleCallback, NULL, NULL);
> +    priv->watch = virEventAddHandle(monitor_fd, VIR_EVENT_HANDLE_READABLE,
> +                                    udevEventHandleCallback, threadData, NULL);
>      if (priv->watch == -1)
>          goto unlock;
>  
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux