[PATCH] tools/misc/xenpm: fix getting info when some CPUs are offline

[PATCH] tools/misc/xenpm: fix getting info when some CPUs are offline

[Marek Marczykowski-Górecki]

Use physinfo.max_cpu_id instead of physinfo.nr_cpus to get max CPU id.
This fixes for example 'xenpm get-cpufreq-para' with smt=off, which
otherwise would miss half of the cores.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Reported by @tfm1:
https://github.com/QubesOS/qubes-issues/isues/4456
---
 tools/misc/xenpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 86c12ea5fb..112590fb67 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -1231,7 +1231,7 @@ int main(int argc, char *argv[])
         xc_interface_close(xc_handle);
         return ret;
     }
-    max_cpu_nr = physinfo.nr_cpus;
+    max_cpu_nr = physinfo.max_cpu_id;
 
     /* calculate how many options match with user's input */
     for ( i = 0; i < ARRAY_SIZE(main_options); i++ )
-- 
2.17.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools/misc/xenpm: fix getting info when some CPUs are offline

[Andrew Cooper]

On 30/10/2018 22:16, Marek Marczykowski-Górecki wrote:
> Use physinfo.max_cpu_id instead of physinfo.nr_cpus to get max CPU id.
> This fixes for example 'xenpm get-cpufreq-para' with smt=off, which
> otherwise would miss half of the cores.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Reported by @tfm1:
> https://github.com/QubesOS/qubes-issues/isues/4456

Thats a 404, but /issues/ looks to work.

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  tools/misc/xenpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
> index 86c12ea5fb..112590fb67 100644
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -1231,7 +1231,7 @@ int main(int argc, char *argv[])
>          xc_interface_close(xc_handle);
>          return ret;
>      }
> -    max_cpu_nr = physinfo.nr_cpus;
> +    max_cpu_nr = physinfo.max_cpu_id;
>  
>      /* calculate how many options match with user's input */
>      for ( i = 0; i < ARRAY_SIZE(main_options); i++ )


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools/misc/xenpm: fix getting info when some CPUs are offline

[Jan Beulich]

>>> On 30.10.18 at 23:16, <marmarek@invisiblethingslab.com> wrote:
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -1231,7 +1231,7 @@ int main(int argc, char *argv[])
>          xc_interface_close(xc_handle);
>          return ret;
>      }
> -    max_cpu_nr = physinfo.nr_cpus;
> +    max_cpu_nr = physinfo.max_cpu_id;

Isn't this off by 1 then? max_cpu_nr is misnamed, all loops using it
are of the form

    for ( i = 0; i < max_cpu_nr; i++ )

I'm also afraid there are further quirks here, with various constructs
along the lines of (as bodies of aforementioned for())

        if ( show_cxstat_by_cpuid(xc_handle, i) == -ENODEV )
            break;

which I suspect would terminate processing early when hitting a true
gap (i.e. not one resulting from a parked CPU). But I guess it wouldn't
be appropriate to ask you to deal with this at the same time.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools/misc/xenpm: fix getting info when some CPUs are offline

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1488 bytes --]

On Wed, Oct 31, 2018 at 03:33:58AM -0600, Jan Beulich wrote:
> >>> On 30.10.18 at 23:16, <marmarek@invisiblethingslab.com> wrote:
> > --- a/tools/misc/xenpm.c
> > +++ b/tools/misc/xenpm.c
> > @@ -1231,7 +1231,7 @@ int main(int argc, char *argv[])
> >          xc_interface_close(xc_handle);
> >          return ret;
> >      }
> > -    max_cpu_nr = physinfo.nr_cpus;
> > +    max_cpu_nr = physinfo.max_cpu_id;
> 
> Isn't this off by 1 then? max_cpu_nr is misnamed, all loops using it
> are of the form
> 
>     for ( i = 0; i < max_cpu_nr; i++ )

Oh, you're right. This doesn't affect smt=off case, because the last cpu
is offline anyway...

> I'm also afraid there are further quirks here, with various constructs
> along the lines of (as bodies of aforementioned for())
> 
>         if ( show_cxstat_by_cpuid(xc_handle, i) == -ENODEV )
>             break;
> 
> which I suspect would terminate processing early when hitting a true
> gap (i.e. not one resulting from a parked CPU). 

How that would happen? Hotplug?
Also I've tried to look at error codes to distinguish offline cpu and
avoid printing that error, but looks EINVAL is used for a lot of cases.

> But I guess it wouldn't
> be appropriate to ask you to deal with this at the same time.
> 
> Jan
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] tools/misc/xenpm: fix getting info when some CPUs are offline

[Jan Beulich]

>>> On 31.10.18 at 11:53, <marmarek@invisiblethingslab.com> wrote:
> On Wed, Oct 31, 2018 at 03:33:58AM -0600, Jan Beulich wrote:
>> I'm also afraid there are further quirks here, with various constructs
>> along the lines of (as bodies of aforementioned for())
>> 
>>         if ( show_cxstat_by_cpuid(xc_handle, i) == -ENODEV )
>>             break;
>> 
>> which I suspect would terminate processing early when hitting a true
>> gap (i.e. not one resulting from a parked CPU). 
> 
> How that would happen? Hotplug?

xen-hptool on AMD might trigger this, afaict.

> Also I've tried to look at error codes to distinguish offline cpu and
> avoid printing that error, but looks EINVAL is used for a lot of cases.

Yeah, the common problem with EINVAL.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel