On Wed, Jul 11, 2018 at 05:47:58PM +0200, Michal Privoznik wrote: > On 07/11/2018 05:25 PM, Pavel Hrdina wrote: > > 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 <phrd...@redhat.com> > >>> --- > >>> 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. > > I don't think the assumption is limited to Linux only. Even Windows > behave the same. For instance the following example shows that on > non-NUMA machine there is NUMA node #0. > > https://docs.microsoft.com/en-us/windows/desktop/Memory/allocating-memory-from-a-numa-node
Well if we can change the assumption into a fact I'm definitely for that change to consider all guest to have at least one NUMA node. I was trying to lookup some documentation/specification but failed to do so. > > > > >> > >>> + 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. > > Yes because we have a bug in the code. So when you introduced the test > it was doomed to fail. This test case should fail every time because it is invalid configuration. You have non-NUMA guest with two different pages and also specific node configured for one page. > > > > 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. > > Well, maybe. I'm not saying your patches are wrong. Apart from allowing > nodeset='0' (which I think we should do, but I don't have that much of a > strong opinion there). To make it clear I'll summarize all the possible combinations and how it should work so we are on the same page. Pavel
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list