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

[Pavel Hrdina]

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 <phrdina at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180711/47762ab1/attachment-0001.sig>