[v1] qemu: Support setting NUMA distances

[libvirt] [PATCH 2/5] numa: Introduce virDomainNumaNodeDistanceSpecified

Posted by Michal Privoznik 7 years, 6 months ago

The function returns true/false depending on distance
configuration being present in the domain XML.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/numa_conf.c     | 13 +++++++++++++
 src/conf/numa_conf.h     |  4 ++++
 src/libvirt_private.syms |  1 +
 3 files changed, 18 insertions(+)

diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index 5f0b3f9ed..6a42777e2 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1137,6 +1137,19 @@ virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
     return numa->nmem_nodes;
 }
 
+bool
+virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
+                                   size_t node,
+                                   size_t sibling)
+{
+    return node < numa->nmem_nodes &&
+        sibling < numa->nmem_nodes &&
+        numa->mem_nodes[node].distances &&
+        numa->mem_nodes[node].distances[sibling].value != LOCAL_DISTANCE &&
+        numa->mem_nodes[node].distances[sibling].value != REMOTE_DISTANCE;
+}
+
+
 size_t
 virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
                              size_t node,
diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
index 4655de3aa..1d2e605b6 100644
--- a/src/conf/numa_conf.h
+++ b/src/conf/numa_conf.h
@@ -87,6 +87,10 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune,
 
 size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa);
 
+bool virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
+                                        size_t node,
+                                        size_t sibling)
+    ATTRIBUTE_NONNULL(1);
 size_t virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
                                     size_t node,
                                     size_t sibling)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5a4d50471..779bab7a3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -725,6 +725,7 @@ virDomainNumaGetNodeDistance;
 virDomainNumaGetNodeMemoryAccessMode;
 virDomainNumaGetNodeMemorySize;
 virDomainNumaNew;
+virDomainNumaNodeDistanceSpecified;
 virDomainNumaSetNodeCount;
 virDomainNumaSetNodeCpumask;
 virDomainNumaSetNodeDistance;
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] numa: Introduce virDomainNumaNodeDistanceSpecified

Posted by John Ferlan 7 years, 5 months ago


On 11/14/2017 09:47 AM, Michal Privoznik wrote:
> The function returns true/false depending on distance
> configuration being present in the domain XML.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/conf/numa_conf.c     | 13 +++++++++++++
>  src/conf/numa_conf.h     |  4 ++++
>  src/libvirt_private.syms |  1 +
>  3 files changed, 18 insertions(+)
> 
> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> index 5f0b3f9ed..6a42777e2 100644
> --- a/src/conf/numa_conf.c
> +++ b/src/conf/numa_conf.c
> @@ -1137,6 +1137,19 @@ virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
>      return numa->nmem_nodes;
>  }
>  

Two blank lines here too.

> +bool
> +virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
> +                                   size_t node,
> +                                   size_t sibling)
> +{
> +    return node < numa->nmem_nodes &&
> +        sibling < numa->nmem_nodes &&
> +        numa->mem_nodes[node].distances &&
> +        numa->mem_nodes[node].distances[sibling].value != LOCAL_DISTANCE &&
> +        numa->mem_nodes[node].distances[sibling].value != REMOTE_DISTANCE;
> +}
> +
> +

According to how I read commit message '74119a03' it *is* possible to
set the @value to the same value as LOCAL_DISTANCE(10) or
REMOTE_DISTANCE(20) - so using that as a comparison for whether it was
specified would seem to be wrong.

Still if a distance *is* provided, then it seems that 'id' and 'value'
are also required to be provided or defaulted.  That means what seems to
matter regarding whether something was provided is if the *.value and/or
"*.cellid are zero

At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML

John


>  size_t
>  virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
>                               size_t node,
> diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
> index 4655de3aa..1d2e605b6 100644
> --- a/src/conf/numa_conf.h
> +++ b/src/conf/numa_conf.h
> @@ -87,6 +87,10 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune,
>  
>  size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa);
>  
> +bool virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
> +                                        size_t node,
> +                                        size_t sibling)
> +    ATTRIBUTE_NONNULL(1);
>  size_t virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
>                                      size_t node,
>                                      size_t sibling)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5a4d50471..779bab7a3 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -725,6 +725,7 @@ virDomainNumaGetNodeDistance;
>  virDomainNumaGetNodeMemoryAccessMode;
>  virDomainNumaGetNodeMemorySize;
>  virDomainNumaNew;
> +virDomainNumaNodeDistanceSpecified;
>  virDomainNumaSetNodeCount;
>  virDomainNumaSetNodeCpumask;
>  virDomainNumaSetNodeDistance;
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] numa: Introduce virDomainNumaNodeDistanceSpecified

Posted by Michal Privoznik 7 years, 5 months ago

On 11/22/2017 12:38 AM, John Ferlan wrote:
> 
> 
> On 11/14/2017 09:47 AM, Michal Privoznik wrote:
>> The function returns true/false depending on distance
>> configuration being present in the domain XML.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/conf/numa_conf.c     | 13 +++++++++++++
>>  src/conf/numa_conf.h     |  4 ++++
>>  src/libvirt_private.syms |  1 +
>>  3 files changed, 18 insertions(+)
>>
>> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
>> index 5f0b3f9ed..6a42777e2 100644
>> --- a/src/conf/numa_conf.c
>> +++ b/src/conf/numa_conf.c
>> @@ -1137,6 +1137,19 @@ virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
>>      return numa->nmem_nodes;
>>  }
>>  
> 
> Two blank lines here too.
> 
>> +bool
>> +virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
>> +                                   size_t node,
>> +                                   size_t sibling)
>> +{
>> +    return node < numa->nmem_nodes &&
>> +        sibling < numa->nmem_nodes &&
>> +        numa->mem_nodes[node].distances &&
>> +        numa->mem_nodes[node].distances[sibling].value != LOCAL_DISTANCE &&
>> +        numa->mem_nodes[node].distances[sibling].value != REMOTE_DISTANCE;
>> +}
>> +
>> +
> 
> According to how I read commit message '74119a03' it *is* possible to
> set the @value to the same value as LOCAL_DISTANCE(10) or
> REMOTE_DISTANCE(20) - so using that as a comparison for whether it was
> specified would seem to be wrong.

Sure it is possible. But see my reply to 4/5 why I need this function.

> 
> Still if a distance *is* provided, then it seems that 'id' and 'value'
> are also required to be provided or defaulted.  That means what seems to
> matter regarding whether something was provided is if the *.value and/or
> "*.cellid are zero
> 
> At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML

Okay, so the name is confusing. Would you be okay if I rename this to
virDomainNumaNodeDistanceSpecifiedAndNotDefault()? Ugrh, long name. Any
suggestion is welcome.

Alternatively, I can move LOCAL/REMOTE_DISTANCE macros to numa_conf.h,
give them proper prefix and call virDomainNumaGetNodeDistance() from
qemu and check if returned value is LOCAL/REMOTE.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] numa: Introduce virDomainNumaNodeDistanceSpecified

Posted by John Ferlan 7 years, 5 months ago


On 11/22/2017 04:45 AM, Michal Privoznik wrote:
> On 11/22/2017 12:38 AM, John Ferlan wrote:
>>
>>
>> On 11/14/2017 09:47 AM, Michal Privoznik wrote:
>>> The function returns true/false depending on distance
>>> configuration being present in the domain XML.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/conf/numa_conf.c     | 13 +++++++++++++
>>>  src/conf/numa_conf.h     |  4 ++++
>>>  src/libvirt_private.syms |  1 +
>>>  3 files changed, 18 insertions(+)
>>>
>>> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
>>> index 5f0b3f9ed..6a42777e2 100644
>>> --- a/src/conf/numa_conf.c
>>> +++ b/src/conf/numa_conf.c
>>> @@ -1137,6 +1137,19 @@ virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
>>>      return numa->nmem_nodes;
>>>  }
>>>  
>>
>> Two blank lines here too.
>>
>>> +bool
>>> +virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
>>> +                                   size_t node,
>>> +                                   size_t sibling)
>>> +{
>>> +    return node < numa->nmem_nodes &&
>>> +        sibling < numa->nmem_nodes &&
>>> +        numa->mem_nodes[node].distances &&
>>> +        numa->mem_nodes[node].distances[sibling].value != LOCAL_DISTANCE &&
>>> +        numa->mem_nodes[node].distances[sibling].value != REMOTE_DISTANCE;
>>> +}
>>> +
>>> +
>>
>> According to how I read commit message '74119a03' it *is* possible to
>> set the @value to the same value as LOCAL_DISTANCE(10) or
>> REMOTE_DISTANCE(20) - so using that as a comparison for whether it was
>> specified would seem to be wrong.
> 
> Sure it is possible. But see my reply to 4/5 why I need this function.
> 
>>
>> Still if a distance *is* provided, then it seems that 'id' and 'value'
>> are also required to be provided or defaulted.  That means what seems to
>> matter regarding whether something was provided is if the *.value and/or
>> "*.cellid are zero
>>
>> At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML
> 
> Okay, so the name is confusing. Would you be okay if I rename this to
> virDomainNumaNodeDistanceSpecifiedAndNotDefault()? Ugrh, long name. Any
> suggestion is welcome.

Sometimes comments to the function that state what it's returning help -
especially the long single boolean condition.

Just remember, naming is hard! How about
virDomainNumaNodeDistanceIsUsingDefaults?  The prefix
virDomainNumaNodeDistance makes names longer too...

Not sure I was as hung up over the name as I was the functionality that
at this point in time didn't make as much sense. Although paired with
your comments in patch 4, perhaps make more sense.

I'm also not a fan of the large single condition for the bool function -
especially where some conditions are positive and some are negative, but
that probably just me... If I try to decompose it...

/* Out of range, not created, or not supplied */
if (node >= numa->nmem_nodes || sibling >= numa->nmem_nodes ||
    !numa->mem_nodes[node].distances ||
    numa->mem_nodes[node].distances[sibling].value == 0)
    return false;

/* Supplied using default distances */
if (numa->mem_nodes[node].distances[sibling].value == LOCAL_DISTANCE ||
    numa->mem_nodes[node].distances[sibling].value == REMOTE_DISTANCE)
    return true;

return false;

> 
> Alternatively, I can move LOCAL/REMOTE_DISTANCE macros to numa_conf.h,
> give them proper prefix and call virDomainNumaGetNodeDistance() from
> qemu and check if returned value is LOCAL/REMOTE.

Hiding comparisons behind some other function that requires me to jump
elsewhere which can be painful too.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/5] virDomainNumaGetNodeDistance: Fix input arguments validation
[libvirt] [PATCH 2/5] numa: Introduce virDomainNumaNodeDistanceSpecified
[libvirt] [PATCH 3/5] qemu_capabilities: Introcude QEMU_CAPS_NUMA_DIST
[libvirt] [PATCH 4/5] qemu: Support setting NUMA distances
[libvirt] [PATCH 5/5] news: Document which drivers support NUMA distances