11:03 a.m.
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(a)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...
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