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
