10:50 a.m.
Ideally, we would have the security driver relabelling the paths qemu is going
to touch. And this indeed is how I approached this problem firstly. But there
are couple of problems:
a) we generate the paths, add them onto the cmd line and forget them. You won't
find them in virDomainDef. The secdriver cannot reconstruct them either as they
may depend on qemu.conf (admin can whitelist just a few hugetlbfs mount points).
b) Storing the paths in virDomainDef (which is all the the secriver sees) turns
out to be not trivial too: where would you store them? In function that
constructs the paths (qemuBuildMemoryBackendStr) we don't know what 'device'
are we working with. All the callers either pass memory dev directly (for
<memory/> cells), or construct a dummy one (for <memoryBacking/> and
<numa/>
cells) and pass it. This we wouldn't know where to store the constructed path
anyway.
This solution presented here turned out to be the least painful (yet not very
clear from design POV, I give you that).
Michal Privoznik (2):
qemuProcessBuildDestroyHugepagesPath: create path more frequently
qemuDomainAttachMemory: Crate hugepage dir if needed
src/qemu/qemu_hotplug.c | 3 +++
src/qemu/qemu_process.c | 44 +++++++++++++++++++++++++++++++++++++++-----
src/qemu/qemu_process.h | 5 +++++
3 files changed, 47 insertions(+), 5 deletions(-)
--
2.13.0
10:50 a.m.
New subject: [libvirt] [PATCH 1/2] qemuProcessBuildDestroyHugepagesPath: create path more frequently
https://bugzilla.redhat.com/show_bug.cgi?id=1455819
Currently, the per-domain path for huge pages mmap() for qemu is
created iff domain has memoryBacking and hugepages in it
configured. However, this alone is not enough because there can
be a DIMM module with hugepages configured too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 32ba8e373..a219a8080 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3289,11 +3289,33 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
bool build)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ const long system_pagesize = virGetSystemPageSizeKB();
char *hugepagePath = NULL;
size_t i;
+ bool shouldBuild = false;
int ret = -1;
- if (vm->def->mem.nhugepages) {
+ if (vm->def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE)
+ shouldBuild = true;
+
+ for (i = 0; !shouldBuild && i < vm->def->mem.nhugepages; i++) {
+ if (vm->def->mem.hugepages[i].size != system_pagesize) {
+ shouldBuild = true;
+ break;
+ }
+ }
+
+ for (i = 0; !shouldBuild && vm->def->nmems; i++) {
+ if (vm->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
+ vm->def->mems[i]->pagesize &&
+ vm->def->mems[i]->pagesize != system_pagesize) {
+ shouldBuild = true;
+ break;
+ }
+ }
+
+ if (!build ||
+ (build && shouldBuild)) {
for (i = 0; i < cfg->nhugetlbfs; i++) {
VIR_FREE(hugepagePath);
hugepagePath = qemuGetDomainHugepagePath(vm->def,
&cfg->hugetlbfs[i]);
--
2.13.0
6:38 a.m.
New subject: [libvirt] [PATCH 1/2] qemuProcessBuildDestroyHugepagesPath: create path more frequently
On 06/07/2017 11:50 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1455819
Currently, the per-domain path for huge pages mmap() for qemu is
created iff domain has memoryBacking and hugepages in it
configured. However, this alone is not enough because there can
be a DIMM module with hugepages configured too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 32ba8e373..a219a8080 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3289,11 +3289,33 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
bool build)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ const long system_pagesize = virGetSystemPageSizeKB();
char *hugepagePath = NULL;
size_t i;
+ bool shouldBuild = false;
int ret = -1;
None of the subsequent new checks would be necessary if build was
false... Maybe it'd be better to have a helper function...
if (build)
shouldBuild = qemuProcessNeedHugepagesPath(...);
if (shouldBuild || !build) {
...
}
- if (vm->def->mem.nhugepages) {
+ if (vm->def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE)
+ shouldBuild = true;
Not part of the commit message, but that's an easy fix. Actually it
seems to me the algorithm prior to these changes is solely based on
whether hugepages were defined in the guest missing that there's
multiple ways to configure...
+
+ for (i = 0; !shouldBuild && i < vm->def->mem.nhugepages; i++) {
+ if (vm->def->mem.hugepages[i].size != system_pagesize) {
+ shouldBuild = true;
+ break;
I would think because of the !shouldBuild as an end condition that the
break; wouldn't be necessary... Even more unnecessary if you have a
helper and it returns true right here.
FWIW: This particular check seems to be "additional" to the commit
message as well and is the much shorter check than found in other places
that use virGetSystemPageSizeKB to compare pagesize values.
+ }
+ }
+
+ for (i = 0; !shouldBuild && vm->def->nmems; i++) {
^^
Reduce one space...
+ if (vm->def->mems[i]->model ==
VIR_DOMAIN_MEMORY_MODEL_DIMM &&
+ vm->def->mems[i]->pagesize &&
+ vm->def->mems[i]->pagesize != system_pagesize) {
+ shouldBuild = true;
+ break;
ditto on break; and helper comment.
John
+ }
+ }
+
+ if (!build ||
+ (build && shouldBuild)) {
for (i = 0; i < cfg->nhugetlbfs; i++) {
VIR_FREE(hugepagePath);
hugepagePath = qemuGetDomainHugepagePath(vm->def,
&cfg->hugetlbfs[i]);
10:50 a.m.
New subject: [libvirt] [PATCH 2/2] qemuDomainAttachMemory: Crate hugepage dir if needed
https://bugzilla.redhat.com/show_bug.cgi?id=1455819
It may happen that a domain is started without any huge pages.
However, user might try to attach a DIMM module later. DIMM
backed by huge pages (why would somebody want to mix regular and
huge pages is beyond me). Therefore we have to create the dir if
we haven't done so far.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 3 +++
src/qemu/qemu_process.c | 20 ++++++++++++++++----
src/qemu/qemu_process.h | 5 +++++
3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4a7d99725..62472aef8 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2254,6 +2254,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
priv->qemuCaps, vm->def, mem, NULL, true) <
0)
goto cleanup;
+ if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
+ goto cleanup;
+
if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
goto cleanup;
teardowndevice = true;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a219a8080..823a1385f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3283,9 +3283,10 @@ qemuProcessReconnectCheckMemAliasOrderMismatch(virDomainObjPtr vm)
}
-static int
+int
qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
bool build)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -3314,6 +3315,12 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
}
}
+ if (mem &&
+ mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
+ mem->pagesize &&
+ mem->pagesize != system_pagesize)
+ shouldBuild = true;
+
if (!build ||
(build && shouldBuild)) {
for (i = 0; i < cfg->nhugetlbfs; i++) {
@@ -3324,6 +3331,11 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
goto cleanup;
if (build) {
+ if (virFileExists(hugepagePath)) {
+ ret = 0;
+ goto cleanup;
+ }
+
if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
virReportSystemError(errno,
_("Unable to create %s"),
@@ -3497,7 +3509,7 @@ qemuProcessReconnect(void *opaque)
goto cleanup;
}
- if (qemuProcessBuildDestroyHugepagesPath(driver, obj, true) < 0)
+ if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0)
goto error;
if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps,
@@ -5541,7 +5553,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
NULL) < 0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, true) < 0)
+ if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
goto cleanup;
/* Ensure no historical cgroup for this VM is lying around bogus
@@ -6225,7 +6237,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
goto endjob;
}
- qemuProcessBuildDestroyHugepagesPath(driver, vm, false);
+ qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
vm->def->id = -1;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 830d8cef8..b63784152 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -192,4 +192,9 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob);
+int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build);
+
#endif /* __QEMU_PROCESS_H__ */
--
2.13.0
7:03 a.m.
New subject: [libvirt] [PATCH 2/2] qemuDomainAttachMemory: Crate hugepage dir if needed
On 06/07/2017 11:50 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1455819
It may happen that a domain is started without any huge pages.
However, user might try to attach a DIMM module later. DIMM
backed by huge pages (why would somebody want to mix regular and
huge pages is beyond me). Therefore we have to create the dir if
we haven't done so far.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 3 +++
src/qemu/qemu_process.c | 20 ++++++++++++++++----
src/qemu/qemu_process.h | 5 +++++
3 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4a7d99725..62472aef8 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2254,6 +2254,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
priv->qemuCaps, vm->def, mem, NULL, true) <
0)
goto cleanup;
+ if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
+ goto cleanup;
+
If this call was done after virDomainMemoryInsert, then the new check
[1] and parameter in qemuProcessBuildDestroyHugepagesPath wouldn't be
necessary, but the goto here would need to be removedef, true?
if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
goto cleanup;
teardowndevice = true;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a219a8080..823a1385f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3283,9 +3283,10 @@ qemuProcessReconnectCheckMemAliasOrderMismatch(virDomainObjPtr
vm)
}
-static int
+int
qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
bool build)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -3314,6 +3315,12 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
}
}
+ if (mem &&
+ mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
+ mem->pagesize &&
+ mem->pagesize != system_pagesize)
+ shouldBuild = true;
+
[1]
if (!build ||
(build && shouldBuild)) {
for (i = 0; i < cfg->nhugetlbfs; i++) {
@@ -3324,6 +3331,11 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
goto cleanup;
if (build) {
+ if (virFileExists(hugepagePath)) {
+ ret = 0;
+ goto cleanup;
I was initially wondering why this wasn't an "if (build &&
virFileExists()) continue;", then it dawned on me that this code will
create the path and label it for all hugetlbfs sizes if any one of those
sizes exists, so there's no need...
+ }
+
if (virFileMakePathWithMode(hugepagePath, 0700) < 0) {
virReportSystemError(errno,
_("Unable to create %s"),
@@ -3497,7 +3509,7 @@ qemuProcessReconnect(void *opaque)
goto cleanup;
}
- if (qemuProcessBuildDestroyHugepagesPath(driver, obj, true) < 0)
+ if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0)
goto error;
if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps,
@@ -5541,7 +5553,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
NULL) < 0)
goto cleanup;
- if (qemuProcessBuildDestroyHugepagesPath(driver, vm, true) < 0)
+ if (qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, true) < 0)
goto cleanup;
/* Ensure no historical cgroup for this VM is lying around bogus
@@ -6225,7 +6237,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
goto endjob;
}
- qemuProcessBuildDestroyHugepagesPath(driver, vm, false);
+ qemuProcessBuildDestroyHugepagesPath(driver, vm, NULL, false);
vm->def->id = -1;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 830d8cef8..b63784152 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -192,4 +192,9 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob);
+int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem,
+ bool build);
+
Perhaps a personal pet peeve of mine, but order wise in qemu_process.c
the API is between qemuProcessStopCPUs and qemuProcessIncomingDefNew, so
why not list it thusly in the .h file...
John
#endif /* __QEMU_PROCESS_H__ */
7:59 a.m.
New subject: [libvirt] [PATCH 2/2] qemuDomainAttachMemory: Crate hugepage dir if needed
On 06/13/2017 02:03 PM, John Ferlan wrote:
On 06/07/2017 11:50 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1455819
>
> It may happen that a domain is started without any huge pages.
> However, user might try to attach a DIMM module later. DIMM
> backed by huge pages (why would somebody want to mix regular and
> huge pages is beyond me). Therefore we have to create the dir if
> we haven't done so far.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 3 +++
> src/qemu/qemu_process.c | 20 ++++++++++++++++----
> src/qemu/qemu_process.h | 5 +++++
> 3 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4a7d99725..62472aef8 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2254,6 +2254,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> priv->qemuCaps, vm->def, mem, NULL, true)
< 0)
> goto cleanup;
>
> + if (qemuProcessBuildDestroyHugepagesPath(driver, vm, mem, true) < 0)
> + goto cleanup;
> +
If this call was done after virDomainMemoryInsert, then the new check
[1] and parameter in qemuProcessBuildDestroyHugepagesPath wouldn't be
necessary, but the goto here would need to be removedef, true?
It can't go there because we need to mkdir() before telling qemu about
the path. virDomainMemoryInsert is done after that. But yes, if we could
do that, it'd be awesome.
> if (qemuDomainNamespaceSetupMemory(driver, vm, mem) < 0)
> goto cleanup;
> teardowndevice = true;
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 830d8cef8..b63784152 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -192,4 +192,9 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> qemuDomainAsyncJob asyncJob);
>
> +int qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainMemoryDefPtr mem,
> + bool build);
> +
Perhaps a personal pet peeve of mine, but order wise in qemu_process.c
the API is between qemuProcessStopCPUs and qemuProcessIncomingDefNew, so
why not list it thusly in the .h file...
Ah, okay then.
Michal
2719
days inactive
2725
days old
5 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik