[PATCH] x86/dom0: Add log for dom0_nodes and dom0_max_vcpus_max conflict

Jane Malalane posted 1 patch 2 years, 2 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220209103153.11391-1-jane.malalane@citrix.com
xen/arch/x86/dom0_build.c | 5 +++++
1 file changed, 5 insertions(+)
[PATCH] x86/dom0: Add log for dom0_nodes and dom0_max_vcpus_max conflict
Posted by Jane Malalane 2 years, 2 months ago
This is not a bug. The xen cmdline can request both a NUMA restriction
and a vcpu count restriction for Dom0. The node restriction wil always
be respected which might mean either using dom0_max_vcpus <
opt_dom0_max_vcpus_max or using more vCPUs than pCPUs on a node. In
the case where dom0_max_vcpus gets capped at the maximum number of
pCPUs for the number of nodes chosen, it can be useful particularly
for debugging to print a message in the serial log.

Suggested-by: Edwin Torok <edvin.torok@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/dom0_build.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index fe24e11b37..e57cc80ef0 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -240,6 +240,11 @@ unsigned int __init dom0_max_vcpus(void)
     if ( max_vcpus > limit )
         max_vcpus = limit;
 
+    if ( max_vcpus < opt_dom0_max_vcpus_max && max_vcpus > opt_dom0_max_vcpus_min )
+        printk(XENLOG_INFO "Dom0 using %d vCPUs conflicts with request to use"
+               " %d node(s), using up to %d vCPUs\n", opt_dom0_max_vcpus_max,
+               dom0_nr_pxms, max_vcpus);
+
     return max_vcpus;
 }
 
-- 
2.11.0


Re: [PATCH] x86/dom0: Add log for dom0_nodes and dom0_max_vcpus_max conflict
Posted by Jane Malalane 2 years, 2 months ago
On 09/02/2022 10:31, Jane Malalane wrote:
> This is not a bug. The xen cmdline can request both a NUMA restriction
> and a vcpu count restriction for Dom0. The node restriction wil always
> be respected which might mean either using dom0_max_vcpus <
> opt_dom0_max_vcpus_max or using more vCPUs than pCPUs on a node. In
> the case where dom0_max_vcpus gets capped at the maximum number of
> pCPUs for the number of nodes chosen, it can be useful particularly
> for debugging to print a message in the serial log.
> 
> Suggested-by: Edwin Torok <edvin.torok@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>   xen/arch/x86/dom0_build.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index fe24e11b37..e57cc80ef0 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -240,6 +240,11 @@ unsigned int __init dom0_max_vcpus(void)
>       if ( max_vcpus > limit )
>           max_vcpus = limit;
>   
> +    if ( max_vcpus < opt_dom0_max_vcpus_max && max_vcpus > opt_dom0_max_vcpus_min )
> +        printk(XENLOG_INFO "Dom0 using %d vCPUs conflicts with request to use"
> +               " %d node(s), using up to %d vCPUs\n", opt_dom0_max_vcpus_max,
> +               dom0_nr_pxms, max_vcpus);
> +
>       return max_vcpus;
>   }
>   
Here I was debating whether to use a printk or a dprintk, as although 
this would be useful for debugging, it gives general info on dom0 vCPU 
topology, as does for e.g. 'Dom0 has maximimum ... vCPUS'. However, I 
defer to the maintainers, as I'm still getting acquainted with Xen (I 
also understand this patch is of minor importance).

Thanks,

Jane.
Re: [PATCH] x86/dom0: Add log for dom0_nodes and dom0_max_vcpus_max conflict
Posted by Jan Beulich 2 years, 2 months ago
On 09.02.2022 11:31, Jane Malalane wrote:
> This is not a bug. The xen cmdline can request both a NUMA restriction
> and a vcpu count restriction for Dom0. The node restriction wil always
> be respected which might mean either using dom0_max_vcpus <
> opt_dom0_max_vcpus_max

This is quite normal a case if a range was specified, or did you mean
opt_dom0_max_vcpus_min? But min and max get applied last anyway, so
those always override what was derived from dom0_nr_pxms.

> or using more vCPUs than pCPUs on a node. In
> the case where dom0_max_vcpus gets capped at the maximum number of
> pCPUs for the number of nodes chosen, it can be useful particularly
> for debugging to print a message in the serial log.

The number of vCPU-s Dom0 gets is logged in all cases. And the
reasons why a certain value is uses depends on more than just
the number-of-nodes restriction. I therefor wonder whether the
wording as you've chosen it is potentially misleading, and
properly expressing everything in a single message is going to
be quite a bit too noisy. Furthermore ...

> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -240,6 +240,11 @@ unsigned int __init dom0_max_vcpus(void)
>      if ( max_vcpus > limit )
>          max_vcpus = limit;
>  
> +    if ( max_vcpus < opt_dom0_max_vcpus_max && max_vcpus > opt_dom0_max_vcpus_min )
> +        printk(XENLOG_INFO "Dom0 using %d vCPUs conflicts with request to use"
> +               " %d node(s), using up to %d vCPUs\n", opt_dom0_max_vcpus_max,
> +               dom0_nr_pxms, max_vcpus);

... the function can be called more than once, whereas such a
message (if we really want it) would better be issued just once.

To answer your later reply to yourself: I think printk() is fine
here (again assuming we want such a message in the first place);
it's a boot-time-only message after all.

Jan


Re: [PATCH] x86/dom0: Add log for dom0_nodes and dom0_max_vcpus_max conflict
Posted by Jane Malalane 2 years, 2 months ago
On 09/02/2022 11:37, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 09.02.2022 11:31, Jane Malalane wrote:
>> This is not a bug. The xen cmdline can request both a NUMA restriction
>> and a vcpu count restriction for Dom0. The node restriction wil always
>> be respected which might mean either using dom0_max_vcpus <
>> opt_dom0_max_vcpus_max
> 
> This is quite normal a case if a range was specified, or did you mean
> opt_dom0_max_vcpus_min? But min and max get applied last anyway, so
> those always override what was derived from dom0_nr_pxms.
Yes, I was just giving context here for what I say in the following 
sentence. Maybe this became more confusing than helpful.
> 
>> or using more vCPUs than pCPUs on a node. In
>> the case where dom0_max_vcpus gets capped at the maximum number of
>> pCPUs for the number of nodes chosen, it can be useful particularly
>> for debugging to print a message in the serial log.
> > The number of vCPU-s Dom0 gets is logged in all cases. And the
> reasons why a certain value is uses depends on more than just
> the number-of-nodes restriction. 
Maybe I should have said 'Dom0 "receiving" %d vCPUS' instead of "using" 
in the serial log, in which case I can amend that to make it clearer 
(that ofc if we still want this change)?
I therefor wonder whether the
> wording as you've chosen it is potentially misleading, and
> properly expressing everything in a single message is going to
> be quite a bit too noisy. Furthermore ...
> 
>> --- a/xen/arch/x86/dom0_build.c
>> +++ b/xen/arch/x86/dom0_build.c
>> @@ -240,6 +240,11 @@ unsigned int __init dom0_max_vcpus(void)
>>       if ( max_vcpus > limit )
>>           max_vcpus = limit;
>>   
>> +    if ( max_vcpus < opt_dom0_max_vcpus_max && max_vcpus > opt_dom0_max_vcpus_min )
>> +        printk(XENLOG_INFO "Dom0 using %d vCPUs conflicts with request to use"
>> +               " %d node(s), using up to %d vCPUs\n", opt_dom0_max_vcpus_max,
>> +               dom0_nr_pxms, max_vcpus);
> 
> ... the function can be called more than once, whereas such a
> message (if we really want it) would better be issued just once.
Yes, that is true and this code would have to live outside of dom0_build.c.
> 
> To answer your later reply to yourself: I think printk() is fine
> here (again assuming we want such a message in the first place);
> it's a boot-time-only message after all.
> 
Okay.

Thank you,

Jane.