conf: Check for NUMA distances in validity check

[libvirt] [PATCH] conf: Check for NUMA distances in validity check

Posted by Michal Privoznik 6 years, 2 months ago

NUMA distances are part of guest ABI (guests can read it
directly!) and therefore as such shouldn't change throughout the
lifetime of domain.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/numa_conf.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index c906a53de..7d3f9e661 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1073,7 +1073,7 @@ bool
 virDomainNumaCheckABIStability(virDomainNumaPtr src,
                                virDomainNumaPtr tgt)
 {
-    size_t i;
+    size_t i, j;
 
     if (virDomainNumaGetNodeCount(src) != virDomainNumaGetNodeCount(tgt)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1102,6 +1102,17 @@ virDomainNumaCheckABIStability(virDomainNumaPtr src,
                              "match source"), i);
             return false;
         }
+
+        for (j = 0; j < virDomainNumaGetNodeCount(src); j++) {
+            if (virDomainNumaGetNodeDistance(src, i, j) !=
+                virDomainNumaGetNodeDistance(tgt, i, j)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("Target NUMA distance from %zu to %zu "
+                                 "doesn't match source"), i, j);
+
+                return false;
+            }
+        }
     }
 
     return true;
-- 
2.13.6

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

Re: [libvirt] [PATCH] conf: Check for NUMA distances in validity check

Posted by Peter Krempa 6 years, 2 months ago

On Tue, Feb 06, 2018 at 17:23:47 +0100, Michal Privoznik wrote:
> NUMA distances are part of guest ABI (guests can read it
> directly!) and therefore as such shouldn't change throughout the
> lifetime of domain.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/conf/numa_conf.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> index c906a53de..7d3f9e661 100644
> --- a/src/conf/numa_conf.c
> +++ b/src/conf/numa_conf.c
> @@ -1073,7 +1073,7 @@ bool
>  virDomainNumaCheckABIStability(virDomainNumaPtr src,
>                                 virDomainNumaPtr tgt)
>  {
> -    size_t i;
> +    size_t i, j;

Please declare j on a separate line.

>      if (virDomainNumaGetNodeCount(src) != virDomainNumaGetNodeCount(tgt)) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -1102,6 +1102,17 @@ virDomainNumaCheckABIStability(virDomainNumaPtr src,
>                               "match source"), i);
>              return false;
>          }
> +
> +        for (j = 0; j < virDomainNumaGetNodeCount(src); j++) {

Since the matrix of distances is symetrical along the diagonal, do we
really need to check both parts? As in .. how about initializing 'j' to
'i' in this loop?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: Check for NUMA distances in validity check

Posted by Michal Privoznik 6 years, 2 months ago

On 02/06/2018 09:34 PM, Peter Krempa wrote:
> On Tue, Feb 06, 2018 at 17:23:47 +0100, Michal Privoznik wrote:
>> NUMA distances are part of guest ABI (guests can read it
>> directly!) and therefore as such shouldn't change throughout the
>> lifetime of domain.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/conf/numa_conf.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
>> index c906a53de..7d3f9e661 100644
>> --- a/src/conf/numa_conf.c
>> +++ b/src/conf/numa_conf.c
>> @@ -1073,7 +1073,7 @@ bool
>>  virDomainNumaCheckABIStability(virDomainNumaPtr src,
>>                                 virDomainNumaPtr tgt)
>>  {
>> -    size_t i;
>> +    size_t i, j;
> 
> Please declare j on a separate line.

Okay.

> 
>>      if (virDomainNumaGetNodeCount(src) != virDomainNumaGetNodeCount(tgt)) {
>>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> @@ -1102,6 +1102,17 @@ virDomainNumaCheckABIStability(virDomainNumaPtr src,
>>                               "match source"), i);
>>              return false;
>>          }
>> +
>> +        for (j = 0; j < virDomainNumaGetNodeCount(src); j++) {
> 
> Since the matrix of distances is symetrical along the diagonal, do we
> really need to check both parts? As in .. how about initializing 'j' to
> 'i' in this loop?
> 

Is it symmetrical? Does it have to be? I mean, I've never seen
asymmetrical NUMA machine but I don't usually bother checking distance
matrix when I'm on a NUMA machine (very rarely actually).

Michal

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

Re: [libvirt] [PATCH] conf: Check for NUMA distances in validity check

Posted by Peter Krempa 6 years, 2 months ago

On Wed, Feb 07, 2018 at 10:45:39 +0100, Michal Privoznik wrote:
> On 02/06/2018 09:34 PM, Peter Krempa wrote:
> > On Tue, Feb 06, 2018 at 17:23:47 +0100, Michal Privoznik wrote:
> >> NUMA distances are part of guest ABI (guests can read it
> >> directly!) and therefore as such shouldn't change throughout the
> >> lifetime of domain.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/conf/numa_conf.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> >> index c906a53de..7d3f9e661 100644
> >> --- a/src/conf/numa_conf.c
> >> +++ b/src/conf/numa_conf.c
> >> @@ -1073,7 +1073,7 @@ bool
> >>  virDomainNumaCheckABIStability(virDomainNumaPtr src,
> >>                                 virDomainNumaPtr tgt)
> >>  {
> >> -    size_t i;
> >> +    size_t i, j;
> > 
> > Please declare j on a separate line.
> 
> Okay.
> 
> > 
> >>      if (virDomainNumaGetNodeCount(src) != virDomainNumaGetNodeCount(tgt)) {
> >>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >> @@ -1102,6 +1102,17 @@ virDomainNumaCheckABIStability(virDomainNumaPtr src,
> >>                               "match source"), i);
> >>              return false;
> >>          }
> >> +
> >> +        for (j = 0; j < virDomainNumaGetNodeCount(src); j++) {
> > 
> > Since the matrix of distances is symetrical along the diagonal, do we
> > really need to check both parts? As in .. how about initializing 'j' to
> > 'i' in this loop?
> > 
> 
> Is it symmetrical? Does it have to be? I mean, I've never seen
> asymmetrical NUMA machine but I don't usually bother checking distance
> matrix when I'm on a NUMA machine (very rarely actually).

Fair enough. I thought that symmetry was being enforced by the parser
code but looking through it now it looks like symmetry is only assumed
if the other direction is not given.

ACK with the variable declaration fixed. 'n' in case of the number of
numa nodes will be rather small, so it should not be a problem to use
O(n^2) algorithm here.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: Check for NUMA distances in validity check

Posted by Michal Privoznik 6 years, 2 months ago

On 02/07/2018 12:18 PM, Peter Krempa wrote:

> 
> ACK with the variable declaration fixed. 'n' in case of the number of
> numa nodes will be rather small, so it should not be a problem to use
> O(n^2) algorithm here.
> 

Agreed. We have plenty of O(n^2) algorithms in our code (even O(n^3) I
guess). Anyway, put the variable declaration on a separate line and
pushed. Thanks.

Michal

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