Re: [PATCH v2 10/13] qemu: avoid deadlock in qemuDomainObjStopWorker
On 10.08.2020 20:40, Daniel Henrique Barboza wrote:
>
>
> On 7/23/20 7:14 AM, Nikolay Shirokovskiy wrote:
>> We are dropping the only reference here so that the event loop thread
>> is going to be exited synchronously. In order to avoid deadlocks we
>> need to unlock the VM so that any handler being called can finish
>> execution and thus even loop thread be finished too.
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> src/qemu/qemu_domain.c | 18 ++
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 5b22eb2..82b3d11 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1637,11 +1637,21 @@ void
>> qemuDomainObjStopWorker(virDomainObjPtr dom)
>> {
>> qemuDomainObjPrivatePtr priv = dom->privateData;
>> + virEventThread *eventThread;
>> - if (priv->eventThread) {
>> - g_object_unref(priv->eventThread);
>> - priv->eventThread = NULL;
>> - }
>> + if (!priv->eventThread)
>> + return;
>> +
>> + /*
>> + * We are dropping the only reference here so that the event loop thread
>> + * is going to be exited synchronously. In order to avoid deadlocks we
>> + * need to unlock the VM so that any handler being called can finish
>> + * execution and thus even loop thread be finished too.
>> + */
>> + eventThread = g_steal_pointer(&priv->eventThread);
>> + virObjectUnlock(dom);
>> + g_object_unref(eventThread);
>> + virObjectLock(dom);
>
> I understand the intention of the code thanks to the comment just before
> it, but this is not robust. This unlocking -> do stuff -> lock will only
> work if the caller of qemuDomainObjStopWorker() is holding the mutex.
>
> I see that this is the case for qemuDomainObjStopWorkerIter(), but
> qemuDomainObjStopWorker() is also called by qemuProcessStop().
> qemuProcessStop()
> does not make any mutex lock/unlock, so you'll be assuming that all callers of
> qemuProcessStop() will hold the mutex before calling it. One of them is
> qemuProcessInit(),
> which calls qemuProcessStop() in the 'stop' jump, and there is no mutex lock
> beforehand as well.
>
> Now we're assuming that all callers of qemuProcessInit() are holding the mutex
> lock beforehand. In a quick glance in the code I saw at least 2 instances that
> this isn't the case, then we'll need to assume that the callers of those
> functions
> are holding the mutex lock. So on and so forth.
>
>
> Given that this code only makes sense when called from
> qemuDomainObjStopWorkerIter(),
> I'd suggest removing the lock/unlock of this function (turning it into just a
> call
> to qemuDomainObjStopWorker()) and move them inside the body of
> qemuDomainObjStopWorker(),
> locking and unlocking the mutex inside the scope of the same function.
>
Hi, Daniel.
Actually all callers of qemuProcessStop hold the lock. Moreover they even hold
job condition. And calling qemuProcessStop without lock/job condition would be
a mistake AFIU (qemuConnectDomainXMLToNative is the only exception I see that
holds the lock but not the job condition. But this function creates new vm
object that is not shared with other threads) I understand you concern but
there are precedents. Take a look for example virDomainObjListRemove. The
lock requirements are documented for virDomainObjListRemove and I can do it for
qemuDomainObjStopWorker too but it looks a bit over documenting to me.
Nikolay
[PATCH] Xen: Improve parsing of PCI addresses in config converter
There was a report on libvirt-users [1] about the domxml to/from
native converter in the Xen driver not handling PCI addresses
without a domain specification. This patch improves parsing of PCI
addresses in the converter and allows PCI addresses with only
bb:ss.f. xl.cfg(5) also allows either the :bb:ss.f or bb:ss.f
format. A test has been added to check the conversion from xl.cfg
to domXML.
[1] https://www.redhat.com/archives/libvirt-users/2020-August/msg00040.html
Signed-off-by: Jim Fehlig
---
src/libxl/xen_common.c | 81 +-
tests/xmconfigdata/test-pci-dev-syntax.cfg | 26 +++
tests/xmconfigdata/test-pci-dev-syntax.xml | 71 +++
tests/xmconfigtest.c | 1 +
4 files changed, 129 insertions(+), 50 deletions(-)
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
index 75fe7e0644..73ce412fe7 100644
--- a/src/libxl/xen_common.c
+++ b/src/libxl/xen_common.c
@@ -372,63 +372,44 @@ static virDomainHostdevDefPtr
xenParsePCI(char *entry)
{
virDomainHostdevDefPtr hostdev = NULL;
-char domain[5];
-char bus[3];
-char slot[3];
-char func[2];
-char *key, *nextkey;
-int domainID;
-int busID;
-int slotID;
-int funcID;
+VIR_AUTOSTRINGLIST tokens = NULL;
+size_t ntokens = 0;
+size_t nexttoken = 0;
+char *slotstr;
+char *funcstr;
+int domain = 0x0;
+int bus;
+int slot;
+int func;
-domain[0] = bus[0] = slot[0] = func[0] = '\0';
-
-/* pci=[':00:1b.0',':00:13.0'] */
-if (!(key = entry))
-return NULL;
-if (!(nextkey = strchr(key, ':')))
-return NULL;
-if (virStrncpy(domain, key, (nextkey - key), sizeof(domain)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Domain %s too big for destination"), key);
+/* pci=['00:1b.0',':00:13.0'] */
+if (!(tokens = virStringSplitCount(entry, ":", 3, &ntokens)))
return NULL;
-}
-key = nextkey + 1;
-if (!(nextkey = strchr(key, ':')))
-return NULL;
-if (virStrncpy(bus, key, (nextkey - key), sizeof(bus)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Bus %s too big for destination"), key);
-return NULL;
+/* domain */
+if (ntokens == 3) {
+if (virStrToLong_i(tokens[nexttoken], NULL, 16, &domain) < 0)
+return NULL;
+nexttoken++;
}
-key = nextkey + 1;
-if (!(nextkey = strchr(key, '.')))
+/* bus */
+if (virStrToLong_i(tokens[nexttoken], NULL, 16, &bus) < 0)
return NULL;
-if (virStrncpy(slot, key, (nextkey - key), sizeof(slot)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Slot %s too big for destination"), key);
-return NULL;
-}
+nexttoken++;
-key = nextkey + 1;
-if (strlen(key) != 1)
-return NULL;
-if (virStrncpy(func, key, 1, sizeof(func)) < 0) {
+/* slot and function */
+slotstr = tokens[nexttoken];
+if (!(funcstr = strchr(slotstr, '.'))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Function %s too big for destination"), key);
+ _("Malformed PCI address %s"), slotstr);
return NULL;
}
-
-if (virStrToLong_i(domain, NULL, 16, &domainID) < 0)
-return NULL;
-if (virStrToLong_i(bus, NULL, 16, &busID) < 0)
-return NULL;
-if (virStrToLong_i(slot, NULL, 16, &slotID) < 0)
+*funcstr = '\0';
+funcstr++;
+if (virStrToLong_i(slotstr, NULL, 16, &slot) < 0)
return NULL;
-if (virStrToLong_i(func, NULL, 16, &funcID) < 0)
+if (virStrToLong_i(funcstr, NULL, 16, &func) < 0)
return NULL;
if (!(hostdev = virDomainHostdevDefNew()))
@@ -436,10 +417,10 @@ xenParsePCI(char *entry)
hostdev->managed = false;
hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI;
-hostdev->source.subsys.u.pci.addr.domain = domainID;
-hostdev->source.subsys.u.pci.addr.bus = busID;
-hostdev->source.subsys.u.pci.addr.slot = slotID;
-hostdev->source.subsys.u.pci.addr.function = funcID;
+hostdev->source.subsys.u.pci.addr.domain = domain;
+hostdev->source.subsys.u.pci.addr.bus = bus;
+hostdev->source.subsys.u.pci.addr.slot = slot;
+hostdev->source.subsys.u.pci.addr.function = func;
return hostdev;
}
diff --git a/tests/xmconfigdata/test-pci-dev-syntax.cfg
b/tests/xmconfigdata/test-pci-dev-syntax.cfg
new file mode 100644
index 00..e0f00d9bd7
--- /dev/null
+++ b/tests/xmconfigdata/test-pci-dev-syntax.cfg
@@ -0,0 +1,26 @@
+name = "test"
+uuid = "cc2315e7-d26a-307a-438c-6d188ec4c09c"
+maxmem = 382
+memory = 350
+vcpus = 1
+pae = 1
+acpi = 1
+apic = 1
+viridian = 0
+rtc_timeoffset = 0
+localtime = 0
+on_poweroff = "destroy"
+on_reboot = "destroy"
+on_crash = "destroy"
+device_model = "/usr/lib/xen/bin/qemu-system-i386"
+sdl = 0
+vnc = 1
+vncunused = 1
Re: [PATCH v2 10/13] qemu: avoid deadlock in qemuDomainObjStopWorker
On 7/23/20 7:14 AM, Nikolay Shirokovskiy wrote:
We are dropping the only reference here so that the event loop thread
is going to be exited synchronously. In order to avoid deadlocks we
need to unlock the VM so that any handler being called can finish
execution and thus even loop thread be finished too.
Signed-off-by: Nikolay Shirokovskiy
---
src/qemu/qemu_domain.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5b22eb2..82b3d11 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1637,11 +1637,21 @@ void
qemuDomainObjStopWorker(virDomainObjPtr dom)
{
qemuDomainObjPrivatePtr priv = dom->privateData;
+virEventThread *eventThread;
-if (priv->eventThread) {
-g_object_unref(priv->eventThread);
-priv->eventThread = NULL;
-}
+if (!priv->eventThread)
+return;
+
+/*
+ * We are dropping the only reference here so that the event loop thread
+ * is going to be exited synchronously. In order to avoid deadlocks we
+ * need to unlock the VM so that any handler being called can finish
+ * execution and thus even loop thread be finished too.
+ */
+eventThread = g_steal_pointer(&priv->eventThread);
+virObjectUnlock(dom);
+g_object_unref(eventThread);
+virObjectLock(dom);
I understand the intention of the code thanks to the comment just before
it, but this is not robust. This unlocking -> do stuff -> lock will only
work if the caller of qemuDomainObjStopWorker() is holding the mutex.
I see that this is the case for qemuDomainObjStopWorkerIter(), but
qemuDomainObjStopWorker() is also called by qemuProcessStop(). qemuProcessStop()
does not make any mutex lock/unlock, so you'll be assuming that all callers of
qemuProcessStop() will hold the mutex before calling it. One of them is
qemuProcessInit(),
which calls qemuProcessStop() in the 'stop' jump, and there is no mutex lock
beforehand as well.
Now we're assuming that all callers of qemuProcessInit() are holding the mutex
lock beforehand. In a quick glance in the code I saw at least 2 instances that
this isn't the case, then we'll need to assume that the callers of those
functions
are holding the mutex lock. So on and so forth.
Given that this code only makes sense when called from
qemuDomainObjStopWorkerIter(),
I'd suggest removing the lock/unlock of this function (turning it into just a
call
to qemuDomainObjStopWorker()) and move them inside the body of
qemuDomainObjStopWorker(),
locking and unlocking the mutex inside the scope of the same function.
Thanks,
DHB
}
Re: [PATCH v2 09/13] vireventthread: exit thread synchronously on finalize
On 7/23/20 7:14 AM, Nikolay Shirokovskiy wrote:
It useful to be sure no thread is running after we drop all references to
Nit: "It is useful to ..."
Reviewed-by: Daniel Henrique Barboza
virEventThread. Otherwise in order to avoid crashes we need to synchronize some
other way or we make extra references in event handler callbacks to all the
object in use. And some of them are not prepared to be refcounted.
Signed-off-by: Nikolay Shirokovskiy
---
src/util/vireventthread.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
index cf86592..136a550 100644
--- a/src/util/vireventthread.c
+++ b/src/util/vireventthread.c
@@ -43,6 +43,7 @@ vir_event_thread_finalize(GObject *object)
if (evt->thread) {
g_main_loop_quit(evt->loop);
+g_thread_join(evt->thread);
g_thread_unref(evt->thread);
}
Re: [PATCH v2 05/13] rpc: add virNetDaemonSetShutdownCallbacks
On 7/23/20 7:14 AM, Nikolay Shirokovskiy wrote:
The function is used to set shutdown prepare and wait callbacks. Prepare
callback is used to inform other threads of the daemon that the daemon will be
closed soon so that they can start to shutdown. Wait callback is used to wait
for other threads to actually finish.
Signed-off-by: Nikolay Shirokovskiy
---
Reviewed-by: Daniel Henrique Barboza
src/libvirt_remote.syms | 1 +
src/rpc/virnetdaemon.c | 15 +++
src/rpc/virnetdaemon.h | 6 ++
3 files changed, 22 insertions(+)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0018a0c..335e431 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -87,6 +87,7 @@ virNetDaemonPreExecRestart;
virNetDaemonQuit;
virNetDaemonRemoveShutdownInhibition;
virNetDaemonRun;
+virNetDaemonSetShutdownCallbacks;
virNetDaemonUpdateServices;
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index bb81a43..5a5643c 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -69,6 +69,8 @@ struct _virNetDaemon {
virHashTablePtr servers;
virJSONValuePtr srvObject;
+virNetDaemonShutdownCallback shutdownPrepareCb;
+virNetDaemonShutdownCallback shutdownWaitCb;
bool quit;
unsigned int autoShutdownTimeout;
@@ -922,3 +924,16 @@ virNetDaemonHasClients(virNetDaemonPtr dmn)
return ret;
}
+
+void
+virNetDaemonSetShutdownCallbacks(virNetDaemonPtr dmn,
+ virNetDaemonShutdownCallback prepareCb,
+ virNetDaemonShutdownCallback waitCb)
+{
+virObjectLock(dmn);
+
+dmn->shutdownPrepareCb = prepareCb;
+dmn->shutdownWaitCb = waitCb;
+
+virObjectUnlock(dmn);
+}
diff --git a/src/rpc/virnetdaemon.h b/src/rpc/virnetdaemon.h
index c2c7767..9f7a282 100644
--- a/src/rpc/virnetdaemon.h
+++ b/src/rpc/virnetdaemon.h
@@ -82,3 +82,9 @@ virNetServerPtr virNetDaemonGetServer(virNetDaemonPtr dmn,
ssize_t virNetDaemonGetServers(virNetDaemonPtr dmn, virNetServerPtr
**servers);
bool virNetDaemonHasServer(virNetDaemonPtr dmn,
const char *serverName);
+
+typedef int (*virNetDaemonShutdownCallback)(void);
+
+void virNetDaemonSetShutdownCallbacks(virNetDaemonPtr dmn,
+ virNetDaemonShutdownCallback prepareCb,
+ virNetDaemonShutdownCallback waitCb);
Re: [PATCH v2 08/13] qemu: don't shutdown event thread in monitor EOF callback
On 7/23/20 7:14 AM, Nikolay Shirokovskiy wrote: This hunk was introduced in [1] in order to avoid loosing events from monitor on stopping qemu process. But as explained in [2] on destroy we won't get neither EOF nor any other events as monitor is just closed. In case of crash/shutdown we won't get any more events as well and qemuDomainObjStopWorker will be called by qemuProcessStop eventually. Thus let's remove qemuDomainObjStopWorker from qemuProcessHandleMonitorEOF as it is not useful anymore. [1] e6afacb0f: qemu: start/stop an event loop thread for domains [2] d2954c072: qemu: ensure domain event thread is always stopped Signed-off-by: Nikolay Shirokovskiy --- Reviewed-by: Daniel Henrique Barboza src/qemu/qemu_process.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e8b15ee..44098fe 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -322,9 +322,6 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon, qemuDomainDestroyNamespace(driver, vm); cleanup: -/* Now we got EOF we're not expecting more I/O, so we - * can finally kill the event thread */ -qemuDomainObjStopWorker(vm); virObjectUnlock(vm); }
Re: [PATCH] meson: fix some FreeBSD checks regressions
Pavel Hrdina wrote: > On Sat, Aug 08, 2020 at 01:22:09PM +0400, Roman Bogorodskiy wrote: > > * Add missing prerequisite headers for checking link_addr(3) > >in net/if_dl.h, > > * Add missing prerequisite headers for checking BRDGSFD, BRDGADD, > >BRDGDEL in net/if_bridgevar.h, > > * When checking for ifconfig(8), set not only IFCONFIG value, > >but also IFCONFIG_PATH as it's used in util/virnetdevip.c. > > > > Signed-off-by: Roman Bogorodskiy > > --- > > meson.build | 14 +- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > It would be probably better to split this into 3 patches as it fixes > three different issues but it's good enough. Thanks for the reviews, pushed as 3 separate patches. > > I really hate a lot the fact that in order to use some headers you have > to include some other headers. It's so annoying and ridiculous. > Indeed. It's also frustrating that there's no way (I guess) to differentiate cases when the symbol is not present, or there's an error in the check itself. > > Thanks for addressing these regressions! > > Reviewed-by: Pavel Hrdina Roman Bogorodskiy signature.asc Description: PGP signature
Re: [GSoC][PATCH 2/7] qemu_domainjob: added maxQueuedJobs and jobs_queued to `qemuDomainJob`
On Tue, Aug 04, 2020 at 08:06:44PM +0530, Prathamesh Chavan wrote: > Since the attribute `jobs_queued` was specific to jobs, > we decided to move this from `qemuDomainObjPrivate` > to `qemuDomainJobObj` structure. > > Also, reference to `maxQueuedJobs` required us to access > config of the qemu-driver. And creating its copy in > the `qemuDomainJob` helped us access the variable > without referencing the driver's config. Just like you split the paragraphs in the commit message, this patch should have been split in 2 as well even though both changes lead to a common goal. Erik
Re: [GSoC][PATCH 1/7] qemu_domain: Added `qemuDomainJobInfo` to domainJob's `privateData`
On Tue, Aug 04, 2020 at 08:06:43PM +0530, Prathamesh Chavan wrote:
> As `qemuDomainJobInfo` had attributes specific to qemu hypervisor's
> jobs, we moved the attribute `current` and `completed` from
> `qemuDomainJobObj` to its `privateData` structure.
>
> In this process, two callback functions: `setJobInfoOperation`
> and `currentJobInfoInit` were introduced to qemuDomainJob's
> callback structure.
>
> Signed-off-by: Prathamesh Chavan
> ---
> src/qemu/qemu_backup.c | 22 +-
> src/qemu/qemu_domain.c | 498 +++
> src/qemu/qemu_domain.h | 74 +
> src/qemu/qemu_domainjob.c| 483 +-
> src/qemu/qemu_domainjob.h| 81 +
> src/qemu/qemu_driver.c | 49 +--
> src/qemu/qemu_migration.c| 62 ++--
> src/qemu/qemu_migration_cookie.c | 8 +-
> src/qemu/qemu_process.c | 32 +-
> 9 files changed, 680 insertions(+), 629 deletions(-)
This patch does IMO too much, moving qemuDomainJobInfo struct to qemu_domain.h
moving a bunch of functions that depend on the qemuDomainJobInfo structure to
qemu_domain.c, moving attributes "current" and "completely" to a different
structure, and introducing new callbacks. This caused the moved code to be
changed in the same step in order to reflect the attribute movement.
To illustrate this:
...
> +void
> +qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver,
> +virDomainObjPtr vm)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +qemuDomainJobPrivatePtr jobPriv = priv->job.privateData;
^This line was changed during the code movement
> +virObjectEventPtr event;
> +virTypedParameterPtr params = NULL;
> +int nparams = 0;
> +int type;
> +
> +if (!jobPriv->completed)
> +return;
^These 2 as well...
> +
> +if (qemuDomainJobInfoToParams(jobPriv->completed, &type,
^This one too...
When doing code movements, it's a better idea to first move the code and then
perform the changes, it's easier for the reviewer as well as the one looking at
the commit history.
You should be able to move the affected functions that need the
qemuDomainJobInfo structure along with the structure in one patch and then in
another patch move the attributes "current" and "completely" to a different
place and adjust the code accordingly.
If for some reason moving the qemuDomainJobInfo structure in the first patch
caused issues for the follow-up patch moving the attributes, then since
qemu_domain.h includes qemu_domainjob.h you could leave the qemuDomainJobInfo
structure movement out of the first patch and move in the second one.
> + ¶ms, &nparams) < 0) {
> +VIR_WARN("Could not get stats for completed job; domain %s",
> + vm->def->name);
> +}
> +
> +event = virDomainEventJobCompletedNewFromObj(vm, params, nparams);
> +virObjectEventStateQueue(driver->domainEventState, event);
> +}
...
> static int
> qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt,
> @@ -140,6 +636,8 @@ static qemuDomainObjPrivateJobCallbacks
> qemuPrivateJobCallbacks = {
> .resetJobPrivate = qemuJobResetPrivate,
> .formatJob = qemuDomainFormatJobPrivate,
> .parseJob = qemuDomainParseJobPrivate,
> +.setJobInfoOperation = qemuDomainJobInfoSetOperation,
^This would probably be better called jobInfoSetOperation
> +.currentJobInfoInit = qemuDomainCurrentJobInfoInit,
As you've established in the commit message itself "current" and "completed"
are QEMU specific, so the callback should therefore be called jobInfoInit or
maybe jobInfoNew.
Erik
Re: [libvirt PATCH 00/20] Use SPDX-License-Identifier
On Tue, Aug 4, 2020 at 1:23 PM Ján Tomko wrote: > > Replace the license blurb in every single file with: > SPDX-License-Identifier: > Coincidentally, this is also machine readable. > > This identifies the few places that use GPL-3.0 (syntax-check), > some places that mistakenly changed the blurb > (patches 1/20 and 4/20). > > The other variations were period vs. semicolon at various places > and double vs. single space before 'If not, write...' > I'm really not a fan of this. While I don't mind the idea of having the machine-readable identifier, you're eliminating one more venue in which someone would *actually* learn about the license and have the opportunity to learn about the philosophy of Free Software. I know that Linux and systemd have already done this, but I feel like this is a huge mistake. One of the reasons why the license header exists the way it does is to inform people of the concept of Free Software as a whole whenever they look at the sources, in the hope that someone would be interested and make their own. Finally, SPDX-License-Identifier tag is not stable, as the SPDX license identifiers themselves are not stable (they are versioned, and change across versions). I would really prefer that versioned tags be used if we used it, since that allows for the correct reference to work. For example, I have done this in one project: # Fedora-License-Identifier: GPLv2+ # SPDX-2.0-License-Identifier: GPL-2.0+ # SPDX-3.0-License-Identifier: GPL-2.0-or-later -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/5] remove NVDIMM auto-alignment for pSeries guests
Ping On 7/30/20 4:47 PM, Daniel Henrique Barboza wrote: Hi, In [1] Andrea proposed that we should *not* auto-align down the NVDIMM size for pSeries guests. Instead we should error out and provide users with a suggested aligned value. This patch implements it. [1] https://www.redhat.com/archives/libvir-list/2020-July/msg01471.html Daniel Henrique Barboza (5): qemu_domain.c: make qemuDomainGetMemorySizeAlignment() public qemu_validate.c: add pSeries NVDIMM size alignment validation qemu_domain.c: do not auto-align ppc64 NVDIMMs qemu_domain.c: change qemuDomainMemoryDeviceAlignSize() return type Revert "formatdomain.html.in: mention pSeries NVDIMM 'align down' mechanic" docs/formatdomain.html.in | 6 +- src/qemu/qemu_domain.c| 57 ++- src/qemu/qemu_domain.h| 7 ++- src/qemu/qemu_hotplug.c | 6 +- src/qemu/qemu_validate.c | 43 -- ...ory-hotplug-nvdimm-ppc64.ppc64-latest.args | 2 +- .../memory-hotplug-nvdimm-ppc64.xml | 2 +- .../memory-hotplug-nvdimm-ppc64.xml | 2 +- 8 files changed, 55 insertions(+), 70 deletions(-)
Re: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies
On Thu, Aug 6, 2020 at 4:21 PM Andrea Bolognani wrote: > On Thu, 2020-08-06 at 15:45 +0200, Marc Hartmayer wrote: > > On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani < > abolo...@redhat.com> wrote: > > > This patch broke libvirt in Debian for certain setups. > > > > > > With AppArmor enabled (the default), the error is > > > > > > $ virsh start cirros > > > error: Failed to start domain cirros > > > error: internal error: Process exited prior to exec: libvirt: > > > error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev': > > > Device or resource busy > > > > > > If I disable AppArmor by passing security='' on the kernel command > > > line, the error message changes to > > > > > > $ virsh start cirros > > > error: Failed to start domain cirros > > > error: internal error: Process exited prior to exec: libvirt: > > > QEMU Driver error : Unable to get devmapper targets for > > > /var/lib/libvirt/images/cirros.qcow2: Success > > > > > > An effective workaround is to set namespaces=[] in qemu.conf, but > > > that's of course not something that we want users doing :) > > > > > > The underlying issue seems to be caused by the fact that, on a Debian > > > installation that uses plain partitions instead of LVM, /proc/devices > > > doesn't contain an entry for device-mapper right after boot, which... > > > ... this code expects. > > > > > > Running > > > > > > $ sudo dmsetup info > > > No devices found > > > > We see the same problem as mentioned by Andrea. The host kernel > > configuration used: > > > > … > > CONFIG_BLK_DEV_DM_BUILTIN=y > > CONFIG_BLK_DEV_DM=m > > … > > > > As soon as we load the kernel module ‘dm-mod‘ everything works because > > then ‘device-mapper‘ is listed in /proc/devices. > > Thanks Marc! I have confirmed that Debian also uses the same kernel > configuration as the one you have reported above, and that running > dmsetup(8) causes the dm-mod kernel module to be loaded. > > For comparison Fedora, where everything works fine, uses > > CONFIG_BLK_DEV_DM_BUILTIN=y > CONFIG_BLK_DEV_DM=y > FYI even having the module loaded isn't the ultimate workaround as there is a further twist of the same issue. Ubuntu has: CONFIG_BLK_DEV_DM_BUILTIN=y CONFIG_BLK_DEV_DM=y And there is an entry in devices (in host and in the container) $ cat /proc/devices | grep map 253 device-mapper But libvirt 6.6 in this case running in a LXD system container (working before) now fails related to this with what seems to be the same high level symptom. # virsh start kvmguest-groovy-normal3 error: Failed to start domain kvmguest-groovy-normal3 error: internal error: Process exited prior to exec: libvirt: QEMU Driver error : Unable to get devmapper targets for /var/lib/uvtool/libvirt/images/kvmguest-groovy-normal3.qcow: No such file or directory > -- > Andrea Bolognani / Red Hat / Virtualization > > -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
Re: [PATCH] meson: fix some FreeBSD checks regressions
On Sat, Aug 08, 2020 at 01:22:09PM +0400, Roman Bogorodskiy wrote: > * Add missing prerequisite headers for checking link_addr(3) >in net/if_dl.h, > * Add missing prerequisite headers for checking BRDGSFD, BRDGADD, >BRDGDEL in net/if_bridgevar.h, > * When checking for ifconfig(8), set not only IFCONFIG value, >but also IFCONFIG_PATH as it's used in util/virnetdevip.c. > > Signed-off-by: Roman Bogorodskiy > --- > meson.build | 14 +- > 1 file changed, 9 insertions(+), 5 deletions(-) It would be probably better to split this into 3 patches as it fixes three different issues but it's good enough. I really hate a lot the fact that in order to use some headers you have to include some other headers. It's so annoying and ridiculous. Thanks for addressing these regressions! Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: device compatibility interface for live migration with assigned devices
On Wed, Aug 05, 2020 at 12:53:19PM +0200, Jiri Pirko wrote: > Wed, Aug 05, 2020 at 11:33:38AM CEST, yan.y.z...@intel.com wrote: > >On Wed, Aug 05, 2020 at 04:02:48PM +0800, Jason Wang wrote: > >> > >> On 2020/8/5 下午3:56, Jiri Pirko wrote: > >> > Wed, Aug 05, 2020 at 04:41:54AM CEST, jasow...@redhat.com wrote: > >> > > On 2020/8/5 上午10:16, Yan Zhao wrote: > >> > > > On Wed, Aug 05, 2020 at 10:22:15AM +0800, Jason Wang wrote: > >> > > > > On 2020/8/5 上午12:35, Cornelia Huck wrote: > >> > > > > > [sorry about not chiming in earlier] > >> > > > > > > >> > > > > > On Wed, 29 Jul 2020 16:05:03 +0800 > >> > > > > > Yan Zhao wrote: > >> > > > > > > >> > > > > > > On Mon, Jul 27, 2020 at 04:23:21PM -0600, Alex Williamson > >> > > > > > > wrote: > >> > > > > > (...) > >> > > > > > > >> > > > > > > > Based on the feedback we've received, the previously > >> > > > > > > > proposed interface > >> > > > > > > > is not viable. I think there's agreement that the user > >> > > > > > > > needs to be > >> > > > > > > > able to parse and interpret the version information. Using > >> > > > > > > > json seems > >> > > > > > > > viable, but I don't know if it's the best option. Is there > >> > > > > > > > any > >> > > > > > > > precedent of markup strings returned via sysfs we could > >> > > > > > > > follow? > >> > > > > > I don't think encoding complex information in a sysfs file is a > >> > > > > > viable > >> > > > > > approach. Quoting Documentation/filesystems/sysfs.rst: > >> > > > > > > >> > > > > > "Attributes should be ASCII text files, preferably with only one > >> > > > > > value > >> > > > > > per file. It is noted that it may not be efficient to contain > >> > > > > > only one > >> > > > > > value per file, so it is socially acceptable to express an array > >> > > > > > of > >> > > > > > values of the same type. > >> > > > > > Mixing types, expressing multiple lines of data, and doing fancy > >> > > > > > formatting of data is heavily frowned upon." > >> > > > > > > >> > > > > > Even though this is an older file, I think these restrictions > >> > > > > > still > >> > > > > > apply. > >> > > > > +1, that's another reason why devlink(netlink) is better. > >> > > > > > >> > > > hi Jason, > >> > > > do you have any materials or sample code about devlink, so we can > >> > > > have a good > >> > > > study of it? > >> > > > I found some kernel docs about it but my preliminary study didn't > >> > > > show me the > >> > > > advantage of devlink. > >> > > > >> > > CC Jiri and Parav for a better answer for this. > >> > > > >> > > My understanding is that the following advantages are obvious (as I > >> > > replied > >> > > in another thread): > >> > > > >> > > - existing users (NIC, crypto, SCSI, ib), mature and stable > >> > > - much better error reporting (ext_ack other than string or errno) > >> > > - namespace aware > >> > > - do not couple with kobject > >> > Jason, what is your use case? > >> > >> > >> I think the use case is to report device compatibility for live migration. > >> Yan proposed a simple sysfs based migration version first, but it looks not > >> sufficient and something based on JSON is discussed. > >> > >> Yan, can you help to summarize the discussion so far for Jiri as a > >> reference? > >> > >yes. > >we are currently defining an device live migration compatibility > >interface in order to let user space like openstack and libvirt knows > >which two devices are live migration compatible. > >currently the devices include mdev (a kernel emulated virtual device) > >and physical devices (e.g. a VF of a PCI SRIOV device). > > > >the attributes we want user space to compare including > >common attribues: > >device_api: vfio-pci, vfio-ccw... > >mdev_type: mdev type of mdev or similar signature for physical device > > It specifies a device's hardware capability. e.g. > >i915-GVTg_V5_4 means it's of 1/4 of a gen9 Intel graphics > >device. > >software_version: device driver's version. > > in .[.bugfix] scheme, where there is no > >compatibility across major versions, minor versions have > >forward compatibility (ex. 1-> 2 is ok, 2 -> 1 is not) and > >bugfix version number indicates some degree of internal > >improvement that is not visible to the user in terms of > >features or compatibility, > > > >vendor specific attributes: each vendor may define different attributes > > device id : device id of a physical devices or mdev's parent pci device. > > it could be equal to pci id for pci devices > > aggregator: used together with mdev_type. e.g. aggregator=2 together > > with i915-GVTg_V5_4 means 2*1/4=1/2 of a gen9 Intel > >graphics device. > > remote_url: for a local NVMe VF, it may be configured with a remote > > url of a remote storage and all data is stored in the > >remote side specified
