[libvirt] [PATCH v3 0/5] Support of auto-dump on watchdog event in libvirtd
This patch series adds a new watchdog action `dump' which lets libvirtd can do auto-dump when receiving a watchdog event from qemu guest. In order to make the function work, there must be a watchdog device added to guest, and guest must have a watchdog daemon running, for example, /etc/init.d/watchdog start or auto-started on boot. Changes from v2: - let default auto-dump dir be /var/lib/libvirt/qemu/dump Hu Tao (5): Add a threadpool implementation Fall back to QEMUD_SAVE_FORMAT_RAW if compression method fails. Add a new function doCoreDump Add a watchdog action `dump' Add me to AUTHORS to make `make syntax-check' happy AUTHORS | 1 + src/Makefile.am | 3 +- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu.conf | 5 + src/qemu/qemu_conf.c | 13 +++- src/qemu/qemu_conf.h | 4 + src/qemu/qemu_driver.c | 235 ++++++++++++++++++++++++++++++++++-------------- src/util/threadpool.c | 140 ++++++++++++++++++++++++++++ src/util/threadpool.h | 35 +++++++ 10 files changed, 367 insertions(+), 71 deletions(-) create mode 100644 src/util/threadpool.c create mode 100644 src/util/threadpool.h -- 1.7.3 -- Thanks, Hu Tao
When dumping a domain, it's reasonable to save dump-file in raw format if dump format is misconfigured or the corresponding compress program is not available rather then fail dumping. --- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++-------------- 1 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f00d8a3..ad67e52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -137,6 +137,8 @@ struct _qemuDomainObjPrivate { int persistentAddrs; }; +static int getCompressionType(struct qemud_driver *driver); + static int qemudShutdown(void); static void qemuDriverLock(struct qemud_driver *driver) @@ -6055,16 +6057,10 @@ cleanup: return ret; } -static int qemudDomainCoreDump(virDomainPtr dom, - const char *path, - int flags ATTRIBUTE_UNUSED) { - struct qemud_driver *driver = dom->conn->privateData; - virDomainObjPtr vm; - int resume = 0, paused = 0; - int ret = -1, fd = -1; - virDomainEventPtr event = NULL; +static int getCompressionType(struct qemud_driver *driver) +{ int compress; - qemuDomainObjPrivatePtr priv; + /* * We reuse "save" flag for "dump" here. Then, we can support the same * format in "save" and "dump". @@ -6073,18 +6069,33 @@ static int qemudDomainCoreDump(virDomainPtr dom, if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); if (compress < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid dump image format specified in " - "configuration file")); - return -1; + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Invalid dump image format specified in " + "configuration file")); + return QEMUD_SAVE_FORMAT_RAW; } if (!qemudCompressProgramAvailable(compress)) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Compression program for dump image format " "in configuration file isn't available")); - return -1; + return QEMUD_SAVE_FORMAT_RAW; } } + return compress; +} + +static int qemudDomainCoreDump(virDomainPtr dom, + const char *path, + int flags ATTRIBUTE_UNUSED) { + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + int resume = 0, paused = 0; + int ret = -1, fd = -1; + virDomainEventPtr event = NULL; + int compress; + qemuDomainObjPrivatePtr priv; + + compress = getCompressionType(driver); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); -- 1.7.3 -- Thanks, Hu Tao
On 11/30/2010 12:12 AM, Hu Tao wrote:
When dumping a domain, it's reasonable to save dump-file in raw format if dump format is misconfigured or the corresponding compress program is not available rather then fail dumping.
Makes sense to me.
--- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++-------------- 1 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f00d8a3..ad67e52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -137,6 +137,8 @@ struct _qemuDomainObjPrivate { int persistentAddrs; };
+static int getCompressionType(struct qemud_driver *driver);
This would be better as enum qemud_save_formats rather than int. No need for a forward declaration, since you implemented the function in topological order.
+static int qemudDomainCoreDump(virDomainPtr dom, + const char *path, + int flags ATTRIBUTE_UNUSED) { + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + int resume = 0, paused = 0; + int ret = -1, fd = -1; + virDomainEventPtr event = NULL; + int compress;
Likewise. ACK with those nits fixed. So I squashed in 5/5 to keep 'make syntax-check' happy for the purposes of 'git bisect', made the above changes, and pushed this patch. The rest of your series still needs a v4. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch prepares for the next patch. --- src/qemu/qemu_driver.c | 145 ++++++++++++++++++++++++++---------------------- 1 files changed, 78 insertions(+), 67 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ad67e52..80ce9f6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -138,6 +138,7 @@ struct _qemuDomainObjPrivate { }; static int getCompressionType(struct qemud_driver *driver); +static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress); static int qemudShutdown(void); @@ -6057,6 +6058,81 @@ cleanup: return ret; } +static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress) +{ + int fd = -1; + int ret = -1; + qemuDomainObjPrivatePtr priv; + + priv = vm->privateData; + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + /* Create an empty file with appropriate ownership. */ + if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to create '%s'"), path); + goto endjob; + } + + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, + _("unable to save file %s"), + path); + goto endjob; + } + + if (driver->securityDriver && + driver->securityDriver->domainSetSavedStateLabel && + driver->securityDriver->domainSetSavedStateLabel(driver->securityDriver, + vm, path) == -1) + goto endjob; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (compress == QEMUD_SAVE_FORMAT_RAW) { + const char *args[] = { + "cat", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); + } else { + const char *prog = qemudSaveCompressionTypeToString(compress); + const char *args[] = { + prog, + "-c", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + if (ret < 0) + goto endjob; + + ret = qemuDomainWaitForMigrationComplete(driver, vm); + + if (ret < 0) + goto endjob; + + if (driver->securityDriver && + driver->securityDriver->domainRestoreSavedStateLabel && + driver->securityDriver->domainRestoreSavedStateLabel(driver->securityDriver, + vm, path) == -1) + goto endjob; + +endjob: + if (ret != 0) + unlink(path); + return ret; +} + static int getCompressionType(struct qemud_driver *driver) { int compress; @@ -6090,13 +6166,10 @@ static int qemudDomainCoreDump(virDomainPtr dom, struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; int resume = 0, paused = 0; - int ret = -1, fd = -1; + int ret = -1; virDomainEventPtr event = NULL; - int compress; qemuDomainObjPrivatePtr priv; - compress = getCompressionType(driver); - qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6109,35 +6182,6 @@ static int qemudDomainCoreDump(virDomainPtr dom, } priv = vm->privateData; - if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) - goto cleanup; - - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - } - - /* Create an empty file with appropriate ownership. */ - if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("failed to create '%s'"), path); - goto endjob; - } - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, - _("unable to save file %s"), - path); - goto endjob; - } - - if (driver->securityDriver && - driver->securityDriver->domainSetSavedStateLabel && - driver->securityDriver->domainSetSavedStateLabel(driver->securityDriver, - vm, path) == -1) - goto endjob; - /* Migrate will always stop the VM, so the resume condition is independent of whether the stop command is issued. */ resume = (vm->state == VIR_DOMAIN_RUNNING); @@ -6161,43 +6205,12 @@ static int qemudDomainCoreDump(virDomainPtr dom, } } - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (compress == QEMUD_SAVE_FORMAT_RAW) { - const char *args[] = { - "cat", - NULL, - }; - ret = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, 0); - } else { - const char *prog = qemudSaveCompressionTypeToString(compress); - const char *args[] = { - prog, - "-c", - NULL, - }; - ret = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, 0); - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret < 0) - goto endjob; - - ret = qemuDomainWaitForMigrationComplete(driver, vm); - + ret = doCoreDump(driver, vm, path, getCompressionType(driver)); if (ret < 0) goto endjob; paused = 1; - if (driver->securityDriver && - driver->securityDriver->domainRestoreSavedStateLabel && - driver->securityDriver->domainRestoreSavedStateLabel(driver->securityDriver, - vm, path) == -1) - goto endjob; - endjob: if ((ret == 0) && (flags & VIR_DUMP_CRASH)) { qemudShutdownVMDaemon(driver, vm, 0); @@ -6230,8 +6243,6 @@ endjob: } cleanup: - if (ret != 0) - unlink(path); if (vm) virDomainObjUnlock(vm); if (event) -- 1.7.3 -- Thanks, Hu Tao
On 11/30/2010 12:13 AM, Hu Tao wrote:
This patch prepares for the next patch. --- src/qemu/qemu_driver.c | 145 ++++++++++++++++++++++++++---------------------- 1 files changed, 78 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ad67e52..80ce9f6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -138,6 +138,7 @@ struct _qemuDomainObjPrivate { };
static int getCompressionType(struct qemud_driver *driver); +static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress);
static int qemudShutdown(void);
@@ -6057,6 +6058,81 @@ cleanup: return ret; }
+static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress) +{ + int fd = -1; + int ret = -1; + qemuDomainObjPrivatePtr priv; + + priv = vm->privateData;
You failed to move the qemuDomainObjBeginJobWithDriver() check to this factored function, but deleted it from the caller - that's a surefire way to crash the program, especially since other code remaining in in qemudDomainCoreDump also wants to use qemuDomainObjEnterMonitorWithDriver.
+ + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + }
Therefore, it's easier to leave this check in the caller, and assume that doCoreDump will only be called by someone that already owns the locks and has verified that vm is still running (but it does mean that your new caller in 4/5 will have to comply with those assumptions).
+ + /* Create an empty file with appropriate ownership. */ + if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to create '%s'"), path); + goto endjob; + }
At which point 'endjob' is not the best name for the label within doCoreDump; sticking with 'cleanup' seems better.
@@ -6109,35 +6182,6 @@ static int qemudDomainCoreDump(virDomainPtr dom, } priv = vm->privateData;
- if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) - goto cleanup; - - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - }
In other words, this portion should probably not be moved out. I do like the idea of this factorization though, so I'm looking forward to v4. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
`dump' watchdog action lets libvirtd to dump the guest when receives a watchdog event (which probably means a guest crash) Currently only qemu is supported. --- src/Makefile.am | 2 +- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu.conf | 5 +++ src/qemu/qemu_conf.c | 13 +++++++- src/qemu/qemu_conf.h | 4 ++ src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 99 insertions(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 5febd76..9484c2d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1089,7 +1089,7 @@ libvirt_test_la_LIBADD = $(libvirt_la_LIBADD) libvirt_test_la_LDFLAGS = $(test_LDFLAGS) $(AM_LDFLAGS) libvirt_test_la_CFLAGS = $(AM_CFLAGS) -libvirt_qemu_la_SOURCES = libvirt-qemu.c +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c libvirt_qemu_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) \ -version-info $(LIBVIRT_VERSION_INFO) \ $(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) \ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3f14cee..a6cb444 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -245,6 +245,7 @@ VIR_ENUM_IMPL(virDomainWatchdogAction, VIR_DOMAIN_WATCHDOG_ACTION_LAST, "shutdown", "poweroff", "pause", + "dump", "none") VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 899b19f..7f50b79 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -462,6 +462,7 @@ enum virDomainWatchdogAction { VIR_DOMAIN_WATCHDOG_ACTION_SHUTDOWN, VIR_DOMAIN_WATCHDOG_ACTION_POWEROFF, VIR_DOMAIN_WATCHDOG_ACTION_PAUSE, + VIR_DOMAIN_WATCHDOG_ACTION_DUMP, VIR_DOMAIN_WATCHDOG_ACTION_NONE, VIR_DOMAIN_WATCHDOG_ACTION_LAST diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f4f965e..fb35ebc 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -191,6 +191,11 @@ # save_image_format = "raw" # dump_image_format = "raw" +# When a domain is configured to be auto-dumped when libvirtd receives a +# watchdog event from qemu guest, libvirtd will saves dump files in directory +# specified by auto_dump_path. Default value is /var/lib/libvirt/qemu/dump +# +# auto_dump_path = "/var/lib/libvirt/qemu/dump" # If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 35caccc..accd0c4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -386,6 +386,17 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } } + p = virConfGetValue (conf, "auto_dump_path"); + CHECK_TYPE ("auto_dump_path", VIR_CONF_STRING); + if (p && p->str) { + VIR_FREE(driver->autoDumpPath); + if (!(driver->autoDumpPath = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } + } + p = virConfGetValue (conf, "hugetlbfs_mount"); CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING); if (p && p->str) { @@ -5369,7 +5380,7 @@ int qemudBuildCommandLine(virConnectPtr conn, } ADD_ARG(optstr); - const char *action = virDomainWatchdogActionTypeToString(watchdog->action); + const char *action = virDomainWatchdogActionTypeToString(watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP ? VIR_DOMAIN_WATCHDOG_ACTION_PAUSE : watchdog->action); if (!action) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid watchdog action")); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 790ce98..72f961a 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -106,6 +106,8 @@ enum qemud_cmd_flags { struct qemud_driver { virMutex lock; + struct virWorkerPool *workerPool; + int privileged; uid_t user; @@ -173,6 +175,8 @@ struct qemud_driver { char *saveImageFormat; char *dumpImageFormat; + char *autoDumpPath; + pciDeviceList *activePciHostdevs; virBitmapPtr reservedVNCPorts; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80ce9f6..550c6da 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -55,6 +55,7 @@ #include "qemu_driver.h" #include "qemu_conf.h" #include "qemu_monitor.h" +#include "qemu_monitor_json.h" #include "qemu_bridge_filter.h" #include "c-ctype.h" #include "event.h" @@ -85,6 +86,7 @@ #include "files.h" #include "fdstream.h" #include "configmake.h" +#include "threadpool.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -137,8 +139,15 @@ struct _qemuDomainObjPrivate { int persistentAddrs; }; +struct watchdogEvent +{ + virDomainObjPtr vm; + int action; +}; + static int getCompressionType(struct qemud_driver *driver); static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress); +static void processWatchdogEvent(void *data); static int qemudShutdown(void); @@ -1206,6 +1215,16 @@ qemuHandleDomainWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) VIR_WARN("Unable to save status on vm %s after IO error", vm->def->name); } + + if (vm->def->watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) { + struct watchdogEvent *wdEvent; + if (VIR_ALLOC(wdEvent) == 0) { + wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP; + wdEvent->vm = vm; + virWorkerPoolSendJob(driver->workerPool, wdEvent); + } + } + virDomainObjUnlock(vm); if (watchdogEvent || lifecycleEvent) { @@ -1788,6 +1807,9 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->snapshotDir, "%s/lib/libvirt/qemu/snapshot", LOCALSTATEDIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->autoDumpPath, + "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) == -1) + goto out_of_memory; } else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -1816,6 +1838,8 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->snapshotDir, "%s/qemu/snapshot", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump", base) == -1) + goto out_of_memory; } if (virFileMakePath(qemu_driver->stateDir) != 0) { @@ -1848,6 +1872,12 @@ qemudStartup(int privileged) { qemu_driver->snapshotDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } + if (virFileMakePath(qemu_driver->autoDumpPath) != 0) { + char ebuf[1024]; + VIR_ERROR(_("Failed to create dump dir '%s': %s"), + qemu_driver->autoDumpPath, virStrerror(errno, ebuf, sizeof ebuf)); + goto error; + } /* Configuration paths are either ~/.libvirt/qemu/... (session) or * /etc/libvirt/qemu/... (system). @@ -1973,6 +2003,10 @@ qemudStartup(int privileged) { qemudAutostartConfigs(qemu_driver); + qemu_driver->workerPool = virWorkerPoolNew(0, 1, processWatchdogEvent); + if (!qemu_driver->workerPool) + goto error; + if (conn) virConnectClose(conn); @@ -2108,6 +2142,8 @@ qemudShutdown(void) { qemuDriverUnlock(qemu_driver); virMutexDestroy(&qemu_driver->lock); + if (qemu_driver->workerPool) + virWorkerPoolFree(qemu_driver->workerPool); VIR_FREE(qemu_driver); return 0; @@ -4433,6 +4469,45 @@ retry: } } +static void processWatchdogEvent(void *data) +{ + struct watchdogEvent *wdEvent = data; + + switch (wdEvent->action) { + case VIR_DOMAIN_WATCHDOG_ACTION_DUMP: + { + char *dumpfile; + int i; + + qemuDomainObjPrivatePtr priv = wdEvent->vm->privateData; + + i = virAsprintf(&dumpfile, "%s/%s-%u", qemu_driver->autoDumpPath, wdEvent->vm->def->name, (unsigned int)time(NULL)); + dumpfile[i] = '\0'; + + qemuDriverLock(qemu_driver); + virDomainObjLock(wdEvent->vm); + + if (qemuDomainObjBeginJobWithDriver(qemu_driver, wdEvent->vm) < 0) + break; + + doCoreDump(qemu_driver, wdEvent->vm, dumpfile, getCompressionType(qemu_driver)); + qemuDomainObjEnterMonitorWithDriver(qemu_driver, wdEvent->vm); + qemuMonitorJSONStartCPUs(priv->mon, NULL); + qemuDomainObjExitMonitorWithDriver(qemu_driver, wdEvent->vm); + + qemuDomainObjEndJob(wdEvent->vm); + + virDomainObjUnlock(wdEvent->vm); + qemuDriverUnlock(qemu_driver); + + VIR_FREE(dumpfile); + } + break; + } + + VIR_FREE(wdEvent); +} + static virDrvOpenStatus qemudOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { -- 1.7.3 -- Thanks, Hu Tao
On 11/30/2010 12:13 AM, Hu Tao wrote:
`dump' watchdog action lets libvirtd to dump the guest when receives a watchdog event (which probably means a guest crash)
Currently only qemu is supported. --- src/Makefile.am | 2 +- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu.conf | 5 +++ src/qemu/qemu_conf.c | 13 +++++++- src/qemu/qemu_conf.h | 4 ++ src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 5febd76..9484c2d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1089,7 +1089,7 @@ libvirt_test_la_LIBADD = $(libvirt_la_LIBADD) libvirt_test_la_LDFLAGS = $(test_LDFLAGS) $(AM_LDFLAGS) libvirt_test_la_CFLAGS = $(AM_CFLAGS)
-libvirt_qemu_la_SOURCES = libvirt-qemu.c +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
Why is this change necessary? Shouldn't libvirt_util.la already include threadpool.c, and the qemu driver already be linking with libvirt_util.la?
libvirt_qemu_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) \ -version-info $(LIBVIRT_VERSION_INFO) \ $(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) \ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3f14cee..a6cb444 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -245,6 +245,7 @@ VIR_ENUM_IMPL(virDomainWatchdogAction, VIR_DOMAIN_WATCHDOG_ACTION_LAST, "shutdown", "poweroff", "pause", + "dump", "none")
VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 899b19f..7f50b79 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -462,6 +462,7 @@ enum virDomainWatchdogAction { VIR_DOMAIN_WATCHDOG_ACTION_SHUTDOWN, VIR_DOMAIN_WATCHDOG_ACTION_POWEROFF, VIR_DOMAIN_WATCHDOG_ACTION_PAUSE, + VIR_DOMAIN_WATCHDOG_ACTION_DUMP, VIR_DOMAIN_WATCHDOG_ACTION_NONE,
VIR_DOMAIN_WATCHDOG_ACTION_LAST diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f4f965e..fb35ebc 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -191,6 +191,11 @@ # save_image_format = "raw" # dump_image_format = "raw"
+# When a domain is configured to be auto-dumped when libvirtd receives a +# watchdog event from qemu guest, libvirtd will saves dump files in directory
s/saves/save/
+# specified by auto_dump_path. Default value is /var/lib/libvirt/qemu/dump +# +# auto_dump_path = "/var/lib/libvirt/qemu/dump"
# If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 35caccc..accd0c4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -386,6 +386,17 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } }
+ p = virConfGetValue (conf, "auto_dump_path"); + CHECK_TYPE ("auto_dump_path", VIR_CONF_STRING); + if (p && p->str) {
Where did you provide the default autoDumpPath if auto_dump_path is not specified in the .conf file?
+ VIR_FREE(driver->autoDumpPath); + if (!(driver->autoDumpPath = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } + } + p = virConfGetValue (conf, "hugetlbfs_mount"); CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING); if (p && p->str) { @@ -5369,7 +5380,7 @@ int qemudBuildCommandLine(virConnectPtr conn, } ADD_ARG(optstr);
- const char *action = virDomainWatchdogActionTypeToString(watchdog->action); + const char *action = virDomainWatchdogActionTypeToString(watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP ? VIR_DOMAIN_WATCHDOG_ACTION_PAUSE : watchdog->action);
Please wrap to 80 columns when possible. For example, I would have done something like: { int act = watchdog->action; if (act == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) act = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE; const char *action = virDomainWatchdogActionTypeToString(act); }
if (!action) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid watchdog action")); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 790ce98..72f961a 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -106,6 +106,8 @@ enum qemud_cmd_flags { struct qemud_driver { virMutex lock;
+ struct virWorkerPool *workerPool; +
Where's the header that declares virWorkerPool?
int privileged;
uid_t user; @@ -173,6 +175,8 @@ struct qemud_driver { char *saveImageFormat; char *dumpImageFormat;
+ char *autoDumpPath; +
Memory leak if you don't add VIR_FREE(qemu_driver->autoDumpPath) to qemudShutdown().
pciDeviceList *activePciHostdevs;
virBitmapPtr reservedVNCPorts; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80ce9f6..550c6da 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -55,6 +55,7 @@ #include "qemu_driver.h" #include "qemu_conf.h" #include "qemu_monitor.h" +#include "qemu_monitor_json.h"
Nope. The whole point of qemu_monitor.h is that it should isolate text vs. json so that the rest of qemu_driver doesn't need to care about the difference. That means you need to refactor things so that you provide a qemu_monitor.c shim that either calls the json variant or gracefully fails for the text variant.
#include "qemu_bridge_filter.h" #include "c-ctype.h" #include "event.h" @@ -85,6 +86,7 @@ #include "files.h" #include "fdstream.h" #include "configmake.h" +#include "threadpool.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -137,8 +139,15 @@ struct _qemuDomainObjPrivate { int persistentAddrs; };
+struct watchdogEvent +{ + virDomainObjPtr vm; + int action; +}; + static int getCompressionType(struct qemud_driver *driver); static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress); +static void processWatchdogEvent(void *data);
static int qemudShutdown(void);
@@ -1206,6 +1215,16 @@ qemuHandleDomainWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) VIR_WARN("Unable to save status on vm %s after IO error", vm->def->name); } + + if (vm->def->watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) { + struct watchdogEvent *wdEvent; + if (VIR_ALLOC(wdEvent) == 0) { + wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP; + wdEvent->vm = vm; + virWorkerPoolSendJob(driver->workerPool, wdEvent); + } + }
You also need to handle allocation failure, by reporting the out-of-memory condition rather than silently ignoring the watchdog.
+ virDomainObjUnlock(vm);
if (watchdogEvent || lifecycleEvent) { @@ -1788,6 +1807,9 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->snapshotDir, "%s/lib/libvirt/qemu/snapshot", LOCALSTATEDIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->autoDumpPath, + "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) == -1)
At first glance, I'm not quite sure why autoDumpPath is configurable but not snapshotDir. I guess it has to do with the fact that snapshots are under libvirt control (the user does not need to know that they exist), but dump files are intended to be consumed by the user (so the user should be able to specify an alternate location).
+ goto out_of_memory; } else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -1816,6 +1838,8 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->snapshotDir, "%s/qemu/snapshot", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump", base) == -1) + goto out_of_memory;
However, it does raise an issue. Is qemu.conf only for privileged users, or do we have to worry about allowing non-privileged users also be able to pick up an alternate directory (especially since they can't dump to /var/log/...)?
}
if (virFileMakePath(qemu_driver->stateDir) != 0) { @@ -1848,6 +1872,12 @@ qemudStartup(int privileged) { qemu_driver->snapshotDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } + if (virFileMakePath(qemu_driver->autoDumpPath) != 0) { + char ebuf[1024]; + VIR_ERROR(_("Failed to create dump dir '%s': %s"), + qemu_driver->autoDumpPath, virStrerror(errno, ebuf, sizeof ebuf)); + goto error; + }
/* Configuration paths are either ~/.libvirt/qemu/... (session) or * /etc/libvirt/qemu/... (system). @@ -1973,6 +2003,10 @@ qemudStartup(int privileged) {
qemudAutostartConfigs(qemu_driver);
+ qemu_driver->workerPool = virWorkerPoolNew(0, 1, processWatchdogEvent); + if (!qemu_driver->workerPool) + goto error; + if (conn) virConnectClose(conn);
@@ -2108,6 +2142,8 @@ qemudShutdown(void) {
qemuDriverUnlock(qemu_driver); virMutexDestroy(&qemu_driver->lock); + if (qemu_driver->workerPool) + virWorkerPoolFree(qemu_driver->workerPool);
virWorkerPoolFree() should behave like free() and be a no-op if passed NULL, in which case this if was not necessary. There's a list of free()-like functions in cfg.mk, and virWorkerPoolFree should be added to it.
VIR_FREE(qemu_driver);
return 0; @@ -4433,6 +4469,45 @@ retry: } }
+static void processWatchdogEvent(void *data) +{ + struct watchdogEvent *wdEvent = data; + + switch (wdEvent->action) { + case VIR_DOMAIN_WATCHDOG_ACTION_DUMP: + { + char *dumpfile; + int i; + + qemuDomainObjPrivatePtr priv = wdEvent->vm->privateData; + + i = virAsprintf(&dumpfile, "%s/%s-%u", qemu_driver->autoDumpPath, wdEvent->vm->def->name, (unsigned int)time(NULL));
Wrap to 80 columns.
+ dumpfile[i] = '\0';
Not necessary; virAsprintf guarantees a NUL-terminated string.
+ + qemuDriverLock(qemu_driver); + virDomainObjLock(wdEvent->vm); + + if (qemuDomainObjBeginJobWithDriver(qemu_driver, wdEvent->vm) < 0) + break;
You need to check that vm is still active here, if your changes for patch 3/5 move the active vm check out of doCoreDump.
+ + doCoreDump(qemu_driver, wdEvent->vm, dumpfile, getCompressionType(qemu_driver));
You should log any failures to complete the dump.
+ qemuDomainObjEnterMonitorWithDriver(qemu_driver, wdEvent->vm); + qemuMonitorJSONStartCPUs(priv->mon, NULL);
This should call the shim in qemu_monitor.c, rather than directly calling into qemuMonitorJSON*.
+ qemuDomainObjExitMonitorWithDriver(qemu_driver, wdEvent->vm); + + qemuDomainObjEndJob(wdEvent->vm); + + virDomainObjUnlock(wdEvent->vm); + qemuDriverUnlock(qemu_driver); + + VIR_FREE(dumpfile); + } + break; + } + + VIR_FREE(wdEvent); +} + static virDrvOpenStatus qemudOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) {
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Nov 30, 2010 at 03:21:36PM -0700, Eric Blake wrote: <...snip...>
-libvirt_qemu_la_SOURCES = libvirt-qemu.c +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
Why is this change necessary? Shouldn't libvirt_util.la already include threadpool.c, and the qemu driver already be linking with libvirt_util.la?
Is this ok? -libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) +libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) ../src/libvirt_util.la Or link will fail: CCLD libvirtd libvirtd-libvirtd.o: In function `qemudRunLoop': /mnt/data/kernel/libvirt/daemon/libvirtd.c:2229: undefined reference to `virWorkerPoolNew' /mnt/data/kernel/libvirt/daemon/libvirtd.c:2287: undefined reference to `virWorkerPoolFree' libvirtd-libvirtd.o: In function `qemudDispatchClientRead': /mnt/data/kernel/libvirt/daemon/libvirtd.c:1778: undefined reference to `virWorkerPoolSendJob' ../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function `qemuHandleDomainWatchdog': /mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:1224: undefined reference to `virWorkerPoolSendJob' ../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function `qemudShutdown': /mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:2147: undefined reference to `virWorkerPoolFree' ../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function `qemudStartup': /mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:2007: undefined reference to `virWorkerPoolNew' collect2: ld returned 1 exit status <...snip...>
+ virDomainObjUnlock(vm);
if (watchdogEvent || lifecycleEvent) { @@ -1788,6 +1807,9 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->snapshotDir, "%s/lib/libvirt/qemu/snapshot", LOCALSTATEDIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->autoDumpPath, + "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) == -1)
At first glance, I'm not quite sure why autoDumpPath is configurable but not snapshotDir. I guess it has to do with the fact that snapshots are under libvirt control (the user does not need to know that they exist), but dump files are intended to be consumed by the user (so the user should be able to specify an alternate location).
Yes.
+ goto out_of_memory; } else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -1816,6 +1838,8 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->snapshotDir, "%s/qemu/snapshot", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump", base) == -1) + goto out_of_memory;
However, it does raise an issue. Is qemu.conf only for privileged users, or do we have to worry about allowing non-privileged users also be able to pick up an alternate directory (especially since they can't dump to /var/log/...)?
qemu.conf is only for privileged users, but non-privileged users who need to analyze dump files should ask admin to specify an auto-dump directory they have right to read. Or do you have a better idea? -- Thanks, Hu Tao
On 12/01/2010 05:50 PM, Hu Tao wrote:
On Tue, Nov 30, 2010 at 03:21:36PM -0700, Eric Blake wrote: <...snip...>
-libvirt_qemu_la_SOURCES = libvirt-qemu.c +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
Why is this change necessary? Shouldn't libvirt_util.la already include threadpool.c, and the qemu driver already be linking with libvirt_util.la?
Is this ok?
-libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) +libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) ../src/libvirt_util.la
Nope; rather...
Or link will fail:
CCLD libvirtd libvirtd-libvirtd.o: In function `qemudRunLoop': /mnt/data/kernel/libvirt/daemon/libvirtd.c:2229: undefined reference to `virWorkerPoolNew' /mnt/data/kernel/libvirt/daemon/libvirtd.c:2287: undefined reference to `virWorkerPoolFree'
That means you need to modify src/libvirt_private.syms to export the new public interfaces from threadpool.h (it should be pretty easy to figure out what edits to make, the tricky part is realizing you need to touch that file in the first place).
+ if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump", base) == -1) + goto out_of_memory;
However, it does raise an issue. Is qemu.conf only for privileged users, or do we have to worry about allowing non-privileged users also be able to pick up an alternate directory (especially since they can't dump to /var/log/...)?
qemu.conf is only for privileged users, but non-privileged users who need to analyze dump files should ask admin to specify an auto-dump directory they have right to read.
Or do you have a better idea?
Is it better to dump a non-privileged log into ~/.libvirt/qemu/dump, so that it's automatically user-accessible? We already use ~/.libvirt for other non-privileged files. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Dec 01, 2010 at 06:39:14PM -0700, Eric Blake wrote:
On 12/01/2010 05:50 PM, Hu Tao wrote:
On Tue, Nov 30, 2010 at 03:21:36PM -0700, Eric Blake wrote: <...snip...>
-libvirt_qemu_la_SOURCES = libvirt-qemu.c +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
Why is this change necessary? Shouldn't libvirt_util.la already include threadpool.c, and the qemu driver already be linking with libvirt_util.la?
Is this ok?
-libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) +libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) ../src/libvirt_util.la
Nope; rather...
Or link will fail:
CCLD libvirtd libvirtd-libvirtd.o: In function `qemudRunLoop': /mnt/data/kernel/libvirt/daemon/libvirtd.c:2229: undefined reference to `virWorkerPoolNew' /mnt/data/kernel/libvirt/daemon/libvirtd.c:2287: undefined reference to `virWorkerPoolFree'
That means you need to modify src/libvirt_private.syms to export the new public interfaces from threadpool.h (it should be pretty easy to figure out what edits to make, the tricky part is realizing you need to touch that file in the first place).
+ if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump", base) == -1) + goto out_of_memory;
However, it does raise an issue. Is qemu.conf only for privileged users, or do we have to worry about allowing non-privileged users also be able to pick up an alternate directory (especially since they can't dump to /var/log/...)?
qemu.conf is only for privileged users, but non-privileged users who need to analyze dump files should ask admin to specify an auto-dump directory they have right to read.
Or do you have a better idea?
Is it better to dump a non-privileged log into ~/.libvirt/qemu/dump, so that it's automatically user-accessible? We already use ~/.libvirt for other non-privileged files.
I think you're mixing up unprivileged users using the privileged qemu:///system, with unprivileged users using the unprivileged driver qemu://session. Only the latter uses ~/.libvirt and this patch should already be using ~/.libvirt/qemu/dump in that scenario. Daniel
--- AUTHORS | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/AUTHORS b/AUTHORS index 449a450..3534dcb 100644 --- a/AUTHORS +++ b/AUTHORS @@ -135,6 +135,7 @@ Patches have also been contributed by: John Morrissey <jwm@horde.net> Osier Yang <jyang@redhat.com> Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> + Hu Tao <hutao@cn.fujitsu.com> [....send patches to get your name here....] -- 1.7.3 -- Thanks, Hu Tao
--- src/Makefile.am | 1 + src/util/threadpool.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++ src/util/threadpool.h | 35 ++++++++++++ 3 files changed, 176 insertions(+), 0 deletions(-) create mode 100644 src/util/threadpool.c create mode 100644 src/util/threadpool.h diff --git a/src/Makefile.am b/src/Makefile.am index a9a1986..5febd76 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -76,6 +76,7 @@ UTIL_SOURCES = \ util/uuid.c util/uuid.h \ util/util.c util/util.h \ util/xml.c util/xml.h \ + util/threadpool.c util/threadpool.h \ util/virtaudit.c util/virtaudit.h \ util/virterror.c util/virterror_internal.h diff --git a/src/util/threadpool.c b/src/util/threadpool.c new file mode 100644 index 0000000..4bf0f8d --- /dev/null +++ b/src/util/threadpool.c @@ -0,0 +1,140 @@ +#include <config.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include "threadpool.h" + +static void *workerHandleJob(void *data) +{ + struct virData *localData = NULL; + struct virWorkerPool *pool = data; + + pthread_mutex_lock(&pool->mutex); + + while (1) { + while (!pool->quit && !pool->dataList) { + pool->nFreeWorker++; + pthread_cond_signal(&pool->worker_cond); + pthread_cond_wait(&pool->cond, &pool->mutex); + pool->nFreeWorker--; + + if (pool->nWorker > pool->nMaxWorker) + goto out; + } + + while ((localData = pool->dataList) != NULL) { + pool->dataList = pool->dataList->next; + localData->next = NULL; + + pthread_mutex_unlock(&pool->mutex); + + (pool->func)(localData->data); + free(localData); + + pthread_mutex_lock(&pool->mutex); + } + + if (pool->quit) + break; + } + +out: + pool->nWorker--; + if (pool->nWorker == 0) + pthread_cond_signal(&pool->quit_cond); + pthread_mutex_unlock(&pool->mutex); + + return NULL; +} + +struct virWorkerPool *virWorkerPoolNew(int nWorker, int maxWorker, virWorkerFunc func) +{ + struct virWorkerPool *pool; + pthread_t pid; + int i; + + if (nWorker < 0) + return NULL; + + if (nWorker > maxWorker) + return NULL; + + pool = malloc(sizeof(*pool)); + if (!pool) + return NULL; + + memset(pool, 0, sizeof(*pool)); + pool->func = func; + pthread_mutex_init(&pool->mutex, NULL); + pthread_cond_init(&pool->cond, NULL); + pthread_cond_init(&pool->worker_cond, NULL); + pthread_cond_init(&pool->quit_cond, NULL); + + for (i = 0; i < nWorker; i++) { + pthread_create(&pid, NULL, workerHandleJob, pool); + } + + pool->nFreeWorker = 0; + pool->nWorker = nWorker; + pool->nMaxWorker = maxWorker; + + return pool; +} + +void virWorkerPoolFree(struct virWorkerPool *pool) +{ + pthread_mutex_lock(&pool->mutex); + pool->quit = 1; + if (pool->nWorker > 0) { + pthread_cond_broadcast(&pool->cond); + pthread_cond_wait(&pool->quit_cond, &pool->mutex); + } + pthread_mutex_unlock(&pool->mutex); + free(pool); +} + +int virWorkerPoolSendJob(struct virWorkerPool *pool, void *data) +{ + pthread_t pid; + struct virData *localData; + + localData = malloc(sizeof(*localData)); + if (!localData) + return -1; + + localData->data = data; + + pthread_mutex_lock(&pool->mutex); + if (pool->quit) { + pthread_mutex_unlock(&pool->mutex); + free(localData); + return -1; + } + + localData->next = pool->dataList; + pool->dataList = localData; + + if (pool->nFreeWorker == 0 && pool->nWorker < pool->nMaxWorker) { + pthread_create(&pid, NULL, workerHandleJob, pool); + pool->nWorker++; + } + + pthread_cond_signal(&pool->cond); + + pthread_mutex_unlock(&pool->mutex); + + return 0; +} + +int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker) +{ + if (maxWorker < 0) + return -1; + + pthread_mutex_lock(&pool->mutex); + pool->nMaxWorker = maxWorker; + pthread_mutex_unlock(&pool->mutex); + + return 0; +} diff --git a/src/util/threadpool.h b/src/util/threadpool.h new file mode 100644 index 0000000..5ff3a6b --- /dev/null +++ b/src/util/threadpool.h @@ -0,0 +1,35 @@ +#ifndef __THREADPOOL_H__ +#define __THREADPOOL_H__ + +#include <pthread.h> + +typedef void (*virWorkerFunc)(void *); + +struct virData { + struct virData *next; + + void *data; +}; + +struct virWorkerPool { + int nWorker; + int nMaxWorker; + int nFreeWorker; + + int quit; + + virWorkerFunc func; + struct virData *dataList; + + pthread_mutex_t mutex; + pthread_cond_t cond; + pthread_cond_t worker_cond; + pthread_cond_t quit_cond; +}; + +struct virWorkerPool *virWorkerPoolNew(int nWorker, int nMaxWorker, virWorkerFunc func); +void virWorkerPoolFree(struct virWorkerPool *pool); +int virWorkerPoolSendJob(struct virWorkerPool *wp, void *data); +int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker); + +#endif -- 1.7.3 -- Thanks, Hu Tao
On 11/30/2010 12:14 AM, Hu Tao wrote:
--- src/Makefile.am | 1 + src/util/threadpool.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++ src/util/threadpool.h | 35 ++++++++++++ 3 files changed, 176 insertions(+), 0 deletions(-) create mode 100644 src/util/threadpool.c create mode 100644 src/util/threadpool.h
diff --git a/src/Makefile.am b/src/Makefile.am index a9a1986..5febd76 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -76,6 +76,7 @@ UTIL_SOURCES = \ util/uuid.c util/uuid.h \ util/util.c util/util.h \ util/xml.c util/xml.h \ + util/threadpool.c util/threadpool.h \ util/virtaudit.c util/virtaudit.h \ util/virterror.c util/virterror_internal.h
diff --git a/src/util/threadpool.c b/src/util/threadpool.c new file mode 100644 index 0000000..4bf0f8d --- /dev/null +++ b/src/util/threadpool.c @@ -0,0 +1,140 @@
Copyright header?
+#include <config.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include "threadpool.h" + +static void *workerHandleJob(void *data) +{ + struct virData *localData = NULL; + struct virWorkerPool *pool = data; + + pthread_mutex_lock(&pool->mutex);
We should be using virMutexLock here, so as to also be portable to mingw.
+ + while (1) { + while (!pool->quit && !pool->dataList) { + pool->nFreeWorker++; + pthread_cond_signal(&pool->worker_cond);
Likewise, virCondSignal here.
+ pthread_cond_wait(&pool->cond, &pool->mutex);
and virCondWait.
+ pool->nFreeWorker--; + + if (pool->nWorker > pool->nMaxWorker) + goto out; + } + + while ((localData = pool->dataList) != NULL) { + pool->dataList = pool->dataList->next; + localData->next = NULL; + + pthread_mutex_unlock(&pool->mutex); + + (pool->func)(localData->data); + free(localData);
VIR_FREE().
+ + pthread_mutex_lock(&pool->mutex); + } + + if (pool->quit) + break; + } + +out: + pool->nWorker--; + if (pool->nWorker == 0) + pthread_cond_signal(&pool->quit_cond); + pthread_mutex_unlock(&pool->mutex); + + return NULL; +} + +struct virWorkerPool *virWorkerPoolNew(int nWorker, int maxWorker, virWorkerFunc func) +{ + struct virWorkerPool *pool; + pthread_t pid; + int i; + + if (nWorker < 0) + return NULL; + + if (nWorker > maxWorker) + return NULL;
daemon/libvirtd.c already has a notion of worker threads; I'm wondering how much overlap there is between your implementation and that one. A better proof that this would be a useful API addition would be to have the next patch in the series convert libvirtd.c over to using this API.
+ + pool = malloc(sizeof(*pool));
Run 'make syntax-check' - it would have complained about this. Use VIR_ALLOC or VIR_ALLOC_N instead of malloc.
+ if (!pool) + return NULL; + + memset(pool, 0, sizeof(*pool)); + pool->func = func; + pthread_mutex_init(&pool->mutex, NULL);
virMutexInit()
+ pthread_cond_init(&pool->cond, NULL); + pthread_cond_init(&pool->worker_cond, NULL); + pthread_cond_init(&pool->quit_cond, NULL);
virCondInit()
+ + for (i = 0; i < nWorker; i++) { + pthread_create(&pid, NULL, workerHandleJob, pool);
virThreadCreate()
+ } + + pool->nFreeWorker = 0; + pool->nWorker = nWorker; + pool->nMaxWorker = maxWorker; + + return pool; +} + +void virWorkerPoolFree(struct virWorkerPool *pool) +{ + pthread_mutex_lock(&pool->mutex); + pool->quit = 1;
Use <stdbool.h> and bool if a value will only ever be 0 or 1.
+ if (pool->nWorker > 0) { + pthread_cond_broadcast(&pool->cond); + pthread_cond_wait(&pool->quit_cond, &pool->mutex); + } + pthread_mutex_unlock(&pool->mutex); + free(pool);
VIR_FREE()
+} + +int virWorkerPoolSendJob(struct virWorkerPool *pool, void *data) +{ + pthread_t pid; + struct virData *localData; + + localData = malloc(sizeof(*localData));
VIR_ALLOC()
+ if (!localData) + return -1; + + localData->data = data; + + pthread_mutex_lock(&pool->mutex); + if (pool->quit) { + pthread_mutex_unlock(&pool->mutex); + free(localData); + return -1; + } + + localData->next = pool->dataList; + pool->dataList = localData; + + if (pool->nFreeWorker == 0 && pool->nWorker < pool->nMaxWorker) { + pthread_create(&pid, NULL, workerHandleJob, pool); + pool->nWorker++; + } + + pthread_cond_signal(&pool->cond); + + pthread_mutex_unlock(&pool->mutex); + + return 0; +} + +int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker) +{ + if (maxWorker < 0) + return -1; + + pthread_mutex_lock(&pool->mutex); + pool->nMaxWorker = maxWorker; + pthread_mutex_unlock(&pool->mutex);
Does this do the right thing if maxWorker < pool->nMaxWorker, or does it silently lose existing workers?
+ + return 0; +} diff --git a/src/util/threadpool.h b/src/util/threadpool.h new file mode 100644 index 0000000..5ff3a6b --- /dev/null +++ b/src/util/threadpool.h @@ -0,0 +1,35 @@
Copyright header?
+#ifndef __THREADPOOL_H__ +#define __THREADPOOL_H__
Use of the __ namespace risks collision with the system; I'd feel better if this were __VIR_THREADPOOL_H__.
+ +#include <pthread.h>
"threads.h", not <pthread.h>, so we can support mingw
+ +typedef void (*virWorkerFunc)(void *);
pthread_create() takes a function that can return void*. Should worker functions be allowed to return a value?
+ +struct virData { + struct virData *next; + + void *data; +};
We've typically used typedefs to avoid having to type 'struct virData' everywhere else.
+ +struct virWorkerPool { + int nWorker; + int nMaxWorker; + int nFreeWorker;
s/int/size_t/ when dealing with non-zero counts.
+ + int quit;
s/int/bool/
+ + virWorkerFunc func; + struct virData *dataList; + + pthread_mutex_t mutex; + pthread_cond_t cond; + pthread_cond_t worker_cond; + pthread_cond_t quit_cond;
virMutex, virCond
+}; + +struct virWorkerPool *virWorkerPoolNew(int nWorker, int nMaxWorker, virWorkerFunc func);
needs ATTRIBUTE_RETURN_CHECK.
+void virWorkerPoolFree(struct virWorkerPool *pool); +int virWorkerPoolSendJob(struct virWorkerPool *wp, void *data);
ATTRIBUTE_NONNULL(1)
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker);
ATTRIBUTE_NONNULL(1)
+ +#endif
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hi Eric, Thanks for your careful review of these patches. I'll post v4 patches tomorrow fixing all problems you pointed out.
daemon/libvirtd.c already has a notion of worker threads; I'm wondering how much overlap there is between your implementation and that one. A better proof that this would be a useful API addition would be to have the next patch in the series convert libvirtd.c over to using this API.
OK. Will be in v4. <...snip...>
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker) +{ + if (maxWorker < 0) + return -1; + + pthread_mutex_lock(&pool->mutex); + pool->nMaxWorker = maxWorker; + pthread_mutex_unlock(&pool->mutex);
Does this do the right thing if maxWorker < pool->nMaxWorker, or does it silently lose existing workers?
In the case maxWorker < pool->nMaxWorker and there are pool->nMaxWorker threads running, (pool->nMaxWorker - maxWorker) threads will exit after the new nMaxWorker set. <...snip...>
+ +typedef void (*virWorkerFunc)(void *);
pthread_create() takes a function that can return void*. Should worker functions be allowed to return a value?
threadpool doesn't care the return value, neither it has no way to pass the return value to threadpool creator, so it's meaningless for worker functions to return a value. Another example is virThreadFunc which does't return a value neither. -- Thanks, Hu Tao
On Wed, Dec 01, 2010 at 05:32:44PM +0800, Hu Tao wrote:
Hi Eric,
Thanks for your careful review of these patches. I'll post v4 patches tomorrow fixing all problems you pointed out.
daemon/libvirtd.c already has a notion of worker threads; I'm wondering how much overlap there is between your implementation and that one. A better proof that this would be a useful API addition would be to have the next patch in the series convert libvirtd.c over to using this API.
OK. Will be in v4.
<...snip...>
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker) +{ + if (maxWorker < 0) + return -1; + + pthread_mutex_lock(&pool->mutex); + pool->nMaxWorker = maxWorker; + pthread_mutex_unlock(&pool->mutex);
Does this do the right thing if maxWorker < pool->nMaxWorker, or does it silently lose existing workers?
In the case maxWorker < pool->nMaxWorker and there are pool->nMaxWorker threads running, (pool->nMaxWorker - maxWorker) threads will exit after the new nMaxWorker set.
<...snip...>
+ +typedef void (*virWorkerFunc)(void *);
pthread_create() takes a function that can return void*. Should worker functions be allowed to return a value?
threadpool doesn't care the return value, neither it has no way to pass the return value to threadpool creator, so it's meaningless for worker functions to return a value.
Another example is virThreadFunc which does't return a value neither.
I've needed a thread pool implementation for an unrelated piece of work I'm doing on libvirt. I took your impl here, and updated it to follow libvirt naming style, use appropriate internals APIs, and hide the struct definitions from the header. Take a look at the files attached. Regards, Daniel
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao