On 10/11/2017 10:52 AM, Erik Skultety wrote:
> Adjust udevEventHandleThread to be a proper thread routine running in an
> infinite loop handling devices. The handler thread pulls all available
> data from the udev monitor and only then waits until a wakeup signal for
> new incoming data has been emitted by udevEventHandleCallback.
>
This doing a bit more by removing the driver locks from initialization
too. Perhaps those locks should be kept on Initialization for now and
then in a followup patch remove them since @privateData (or whatever
name it becomes) is then totally self locking.
I don't think we'd run into any deadlocks since Initialization and
Cleanup would still be the only consumers.
> Signed-off-by: Erik Skultety <eskul...@redhat.com>
> ---
> src/node_device/node_device_udev.c | 125
> +++++++++++++++++++++++++++----------
> 1 file changed, 93 insertions(+), 32 deletions(-)
>
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index f7646cd8a..a6d7e6d70 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -62,6 +62,12 @@ struct _udevEventData {
> struct udev_monitor *udev_monitor;
> int watch;
> bool privileged;
> +
> + /* Thread data */
> + virThread th;
> + virCond threadCond;
> + bool threadQuit;
> + bool dataReady;
> };
>
> static virClassPtr udevEventDataClass;
> @@ -80,6 +86,8 @@ udevEventDataDispose(void *obj)
>
> udev_unref(udev);
> udev_monitor_unref(data->udev_monitor);
> +
> + virCondDestroy(&data->threadCond);
> }
>
>
> @@ -108,9 +116,15 @@ udevEventDataNew(void)
> if (!(ret = virObjectLockableNew(udevEventDataClass)))
> return NULL;
>
> + if (virCondInit(&ret->threadCond) < 0) {
> + virObjectUnref(ret);
> + return NULL;
> + }
> +
> ret->watch = -1;
> return ret;
> -}
> +};
> +
Stray keystrokes that should be removed - blame the gremlins.
>
>
> static bool
> @@ -1625,15 +1639,25 @@ udevPCITranslateDeinit(void)
> static int
> nodeStateCleanup(void)
> {
> + udevEventDataPtr priv = NULL;
> +
> if (!driver)
> return -1;
>
> + priv = driver->privateData;
> +
Although perhaps impossible, better safe than sorry
if (!priv)
return;
Do we even need nodeDeviceLock now? It was removed from Initialize
shouldn't this be after nodeDeviceLock...
> nodeDeviceLock();
>
> - virObjectUnref(driver->privateData);
> + virObjectLock(priv);
> + priv->threadQuit = true;
> + virCondSignal(&priv->threadCond);
> + virObjectUnlock(priv);
> + virThreadJoin(&priv->th);
> +
> + virObjectUnref(priv);
> virObjectUnref(driver->nodeDeviceEventState);
> -
> virNodeDeviceObjListFree(driver->devs);
> +
> nodeDeviceUnlock();
> virMutexDestroy(&driver->lock);
> VIR_FREE(driver);
> @@ -1691,30 +1715,60 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv,
>
>
> static void
> -udevEventHandleThread(void *opaque)
> +udevEventHandleThread(void *opaque ATTRIBUTE_UNUSED)
> {
> udevEventDataPtr priv = driver->privateData;
> - int fd = (intptr_t) opaque;
> struct udev_device *device = NULL;
>
> - virObjectLock(priv);
> + /* continue rather than break from the loop on non-fatal errors */
> + while (1) {
> + virObjectLock(priv);
> + while (!priv->dataReady && !priv->threadQuit) {
> + if (virCondWait(&priv->threadCond, &priv->parent.lock)) {
> + virReportSystemError(errno, "%s",
> + _("handler failed to wait on
> condition"));
> + virObjectUnlock(priv);
> + return;
> + }
> + }
>
> - if (!udevEventMonitorSanityCheck(priv, fd)) {
> + if (priv->threadQuit) {
> + virObjectUnlock(priv);
> + return;
> + }
> +
> + errno = 0;
> + device = udev_monitor_receive_device(priv->udev_monitor);
> virObjectUnlock(priv);
> - return;
> - }
>
> - device = udev_monitor_receive_device(priv->udev_monitor);
> - virObjectUnlock(priv);
> + if (!device) {
> + if (errno == 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("udev_monitor_receive_device failed"));
> + return;
> + }
>
> - if (device == NULL) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("udev_monitor_receive_device returned NULL"))
> - return
> - }
> + /* POSIX allows both EAGAIN and EWOULDBLOCK to be used
> + * interchangeably when the read would block or timeout was fired
> + */
> + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
> + if (errno != EAGAIN && errno != EWOULDBLOCK) {
> + VIR_WARNINGS_RESET
> + virReportSystemError(errno, "%s",
> + _("udev_monitor_receive_device
> failed"));
how about "unexpected error receiving udev device"
> + return;
> + }
> +
> + virObjectLock(priv);
> + priv->dataReady = false;
> + virObjectUnlock(priv);
>
> - udevHandleOneDevice(device);
> - udev_device_unref(device);
> + continue;
> + }
> +
> + udevHandleOneDevice(device);
> + udev_device_unref(device);
> + }
> }
>
>
> @@ -1722,18 +1776,19 @@ static void
> udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> int fd,
> int events ATTRIBUTE_UNUSED,
> - void *data ATTRIBUTE_UNUSED)
> + void *opaque ATTRIBUTE_UNUSED)
> {
> udevEventDataPtr priv = driver->privateData;
>
> virObjectLock(priv);
> - if (!udevEventMonitorSanityCheck(priv, fd)) {
> - virObjectUnlock(priv);
> - return;
> - }
> +
> + if (!udevEventMonitorSanityCheck(priv, fd))
> + priv->threadQuit = true;
> + else
> + priv->dataReady = true;
> +
> + virCondSignal(&priv->threadCond);
> virObjectUnlock(priv);
> -
> - udevEventHandleThread((void *)(intptr_t) fd);
> }
>
>
> @@ -1875,29 +1930,29 @@ nodeStateInitialize(bool privileged,
> return -1;
> }
>
> - nodeDeviceLock();
> -
And now the only time we ever use nodeDeviceLock/Unlock in
node_device_udev is in Cleanup... Does something else prevent Cleanup
from being called while perhaps Initialize is still running? Looking at
libvirtd.c, virStateCleanup can only be called once driversInitialized
is set. That's only set once virStateInitialize has been run successfully.
Perhaps we should keep it just for this patch and remove in the "next"
patch leaving the goto unlock intact, but modifying it to be :
unlock:
if (priv)
virObjectUnlock(priv);
nodeDeviceUnlock();
Then the next patch alters the gots's and unlock: label to what this
patch has. It also removes nodeDeviceLock from Cleanup
With a few tweaks:
Reviewed-by: John Ferlan <jfer...@redhat.com>
John
> if (!(driver->devs = virNodeDeviceObjListNew()) ||
> !(priv = udevEventDataNew()))
> - goto unlock;
> + goto cleanup;
>
> driver->privateData = priv;
> driver->nodeDeviceEventState = virObjectEventStateNew();
>
> if (udevPCITranslateInit(privileged) < 0)
> - goto unlock;
> + goto cleanup;
>
> udev = udev_new();
> if (!udev) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("failed to create udev context"));
> - goto unlock;
> + goto cleanup;
> }
> #if HAVE_UDEV_LOGGING
> /* cast to get rid of missing-format-attribute warning */
> udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction);
> #endif
>
> + virObjectLock(priv);
> +
> priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
> if (!priv->udev_monitor) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -1916,6 +1971,12 @@ nodeStateInitialize(bool privileged,
> 128 * 1024 * 1024);
> #endif
>
> + if (virThreadCreate(&priv->th, true, udevEventHandleThread, NULL) < 0) {
> + virReportSystemError(errno, "%s",
> + _("failed to create udev handler thread"));
> + goto unlock;
> + }
> +
> /* 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
> @@ -1934,7 +1995,7 @@ nodeStateInitialize(bool privileged,
> if (udevSetupSystemDev() != 0)
> goto unlock;
>
> - nodeDeviceUnlock();
> + virObjectUnlock(priv);
>
> /* Populate with known devices */
> if (udevEnumerateDevices(udev) != 0)
> @@ -1947,7 +2008,7 @@ nodeStateInitialize(bool privileged,
> return -1;
>
> unlock:
> - nodeDeviceUnlock();
> + virObjectUnlock(priv);
> goto cleanup;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
