Re: [libvirt] [PATCH 2/4] conf: Move hugepage XML validation check out of qemu_command

On Wed, Jul 11, 2018 at 05:05:07PM +0200, Michal Privoznik wrote: > On 07/11/2018 10:22 AM, Pavel Hrdina wrote: >> We can safely validate the hugepage nodeset attribute at a define time. >> This validation is not done for already existing domains when the daemon >> is restarted. >> >> All the changes to the tests are necessary because we move the error >> from domain start into XML parse. >> >> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 32 +++++++++++++++++ >> src/qemu/qemu_command.c | 34 ------------------- >> .../seclabel-dynamic-none-relabel.xml | 2 +- >> tests/qemuxml2argvtest.c | 16 +++++---- >> .../qemuxml2xmloutdata/hugepages-pages10.xml | 30 ---------------- >> tests/qemuxml2xmloutdata/hugepages-pages4.xml | 1 - >> tests/qemuxml2xmloutdata/hugepages-pages9.xml | 31 ----------------- >> .../seclabel-dynamic-none-relabel.xml | 2 +- >> tests/qemuxml2xmltest.c | 3 -- >> 9 files changed, 43 insertions(+), 108 deletions(-) >> delete mode 100644 tests/qemuxml2xmloutdata/hugepages-pages10.xml >> delete mode 120000 tests/qemuxml2xmloutdata/hugepages-pages4.xml >> delete mode 100644 tests/qemuxml2xmloutdata/hugepages-pages9.xml >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 7396616eda..20d67e7854 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -6104,6 +6104,35 @@ virDomainDefLifecycleActionValidate(const virDomainDef *def) >> } >> >> >> +static int >> +virDomainDefMemtuneValidate(const virDomainDef *def) >> +{ >> + const virDomainMemtune *mem = &(def->mem); >> + size_t i; >> + ssize_t pos = virDomainNumaGetNodeCount(def->numa) - 1; >> + >> + for (i = 0; i < mem->nhugepages; i++) { >> + ssize_t nextBit; >> + >> + if (!mem->hugepages[i].nodemask) { >> + /* This is the master hugepage to use. Skip it as it has no >> + * nodemask anyway. */ >> + continue; >> + } >> + >> + nextBit = virBitmapNextSetBit(mem->hugepages[i].nodemask, pos); >> + if (nextBit >= 0) { > > I think its fair to enable hugepages for node #0 which is always there > (even if not configured in domain XML). Just try to run 'numactl -H' > from a domain that has no <numa/> in its XML. Well yes, linux always assumes that there is at least one NUMA node but other systems might not consider it the same.

> >> + virReportError(VIR_ERR_XML_DETAIL, >> + _("hugepages: node %zd not found"), >> + nextBit); >> + return -1; >> + } >> + } > > Also, I see that you're removing hugepages-pages9 test from xml2xml > test. But that is needed only because you disallowed nodeset='0' for > nonnuma domain. The real problem there is that the default page size has That is already disallowed but only once you try to start such domain, I'm just moving this check from start time to parse time.

If you look into qemuxml2argvtest.c you will see that hugepages-pages9 is expected to fail. > no numa node to apply to, not nodeset='0'. I guess we need to check for > that too (or do we want to?) That is yet different issue that can be addressed but it should not block this patch.