[PATCH 0/2] fix smt=0 fallout

[PATCH 0/2] fix smt=0 fallout

[Juergen Gross]

With smt=0 some output of xl is wrong. Fix it.

Juergen Gross (2):
  tools/libxl: correct vcpu affinity output with sparse physical cpu map
  xen: fill topology info for online cpus only

 tools/xl/xl_vcpu.c  | 4 ++--
 xen/common/sysctl.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.16.4


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

[PATCH 1/2] tools/libxl: correct vcpu affinity output with sparse physical cpu map

[Juergen Gross]

With not all physical cpus online (i.e. with smt=0) the output of hte
vcpu affinities is wrong, as the affinity bitmaps are capped after
nr_cpus bits, instead of using max_cpu_id.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xl/xl_vcpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/xl/xl_vcpu.c b/tools/xl/xl_vcpu.c
index 3384eeed06..c877f2595f 100644
--- a/tools/xl/xl_vcpu.c
+++ b/tools/xl/xl_vcpu.c
@@ -144,13 +144,13 @@ static void vcpulist(int argc, char **argv)
         }
 
         for (i = 0; i<nb_domain; i++)
-            print_domain_vcpuinfo(dominfo[i].domid, physinfo.nr_cpus);
+            print_domain_vcpuinfo(dominfo[i].domid, physinfo.max_cpu_id + 1);
 
         libxl_dominfo_list_free(dominfo, nb_domain);
     } else {
         for (; argc > 0; ++argv, --argc) {
             uint32_t domid = find_domain(*argv);
-            print_domain_vcpuinfo(domid, physinfo.nr_cpus);
+            print_domain_vcpuinfo(domid, physinfo.max_cpu_id + 1);
         }
     }
   vcpulist_out:
-- 
2.16.4


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

Re: [PATCH 1/2] tools/libxl: correct vcpu affinity output with sparse physical cpu map

[Wei Liu]

On Wed, Aug 29, 2018 at 08:23:01PM +0200, Juergen Gross wrote:
> With not all physical cpus online (i.e. with smt=0) the output of hte
> vcpu affinities is wrong, as the affinity bitmaps are capped after
> nr_cpus bits, instead of using max_cpu_id.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 1/2] tools/libxl: correct vcpu affinity output with sparse physical cpu map

[Jan Beulich]

>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
> With not all physical cpus online (i.e. with smt=0) the output of hte

I think you mean "e.g." instead of "i.e." here, as there are other
means to have some CPUs offline.

Jan

> vcpu affinities is wrong, as the affinity bitmaps are capped after
> nr_cpus bits, instead of using max_cpu_id.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>



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

Re: [PATCH 1/2] tools/libxl: correct vcpu affinity output with sparse physical cpu map

[Juergen Gross]

On 30/08/18 10:07, Jan Beulich wrote:
>>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
>> With not all physical cpus online (i.e. with smt=0) the output of hte
> 
> I think you mean "e.g." instead of "i.e." here, as there are other
> means to have some CPUs offline.

I used it in the meaning of "namely", in order to highlight the recent
change to Xen making it more clear why the change is IMO important.

In the end I don't mind this being changed to "e.g." when committing.


Juergen


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

Re: [PATCH 1/2] tools/libxl: correct vcpu affinity output with sparse physical cpu map

[Jan Beulich]

>>> On 30.08.18 at 10:11, <jgross@suse.com> wrote:
> On 30/08/18 10:07, Jan Beulich wrote:
>>>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
>>> With not all physical cpus online (i.e. with smt=0) the output of hte
>> 
>> I think you mean "e.g." instead of "i.e." here, as there are other
>> means to have some CPUs offline.
> 
> I used it in the meaning of "namely", in order to highlight the recent
> change to Xen making it more clear why the change is IMO important.

Iirc it was Ian who told me a while ago that our German based use
of "namely" in cases like this isn't really appropriate; I'm not sure
I've fully understood where "namely" would be usable or not, so
I'm simply trying to avoid it now in most cases, and I'm therefore
not sure if it was appropriate to be used here. I'm also not sure
whether my assignment of meaning to "i.e." is really correct, but
I'd generally view it as an analogue of "that is", rather than the
"specifically" you appear to mean here.

Jan



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

[PATCH 2/2] xen: fill topology info for online cpus only

[Juergen Gross]

The topology information obtainable via XEN_SYSCTL_cputopoinfo is
filled rather weird: the size of the array is derived from the highest
online cpu number, while the data is set to "invalid" for not present
cpus only.

With smt=0 the information for parked threads is all zero, so it should
be best to return "invalid" information for offline cpus.

On a dual core system without this patch xl info -n will print:

cpu_topology           :
cpu:    core    socket     node
  0:       0        0        0
  1:       0        0        0
  2:       1        0        0

while with this patch the output is:

cpu_topology           :
cpu:    core    socket     node
  0:       0        0        0
  2:       1        0        0

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 8e83c33a16..6cef6d310b 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -358,7 +358,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
                 num_cpus = ti->num_cpus;
             for ( i = 0; i < num_cpus; ++i )
             {
-                if ( cpu_present(i) )
+                if ( cpu_online(i) )
                 {
                     cputopo.core = cpu_to_core(i);
                     cputopo.socket = cpu_to_socket(i);
-- 
2.16.4


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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Wei Liu]

On Wed, Aug 29, 2018 at 08:23:02PM +0200, Juergen Gross wrote:
> The topology information obtainable via XEN_SYSCTL_cputopoinfo is
> filled rather weird: the size of the array is derived from the highest
> online cpu number, while the data is set to "invalid" for not present
> cpus only.
> 
> With smt=0 the information for parked threads is all zero, so it should
> be best to return "invalid" information for offline cpus.
> 
> On a dual core system without this patch xl info -n will print:
> 
> cpu_topology           :
> cpu:    core    socket     node
>   0:       0        0        0
>   1:       0        0        0
>   2:       1        0        0
> 
> while with this patch the output is:
> 
> cpu_topology           :
> cpu:    core    socket     node
>   0:       0        0        0
>   2:       1        0        0
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

I have always wanted to fix this but never got around to it. Thank you!

Reviewed-by: Wei Liu <wei.liu2@citrix.com>


> ---
>  xen/common/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 8e83c33a16..6cef6d310b 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -358,7 +358,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>                  num_cpus = ti->num_cpus;
>              for ( i = 0; i < num_cpus; ++i )
>              {
> -                if ( cpu_present(i) )
> +                if ( cpu_online(i) )
>                  {
>                      cputopo.core = cpu_to_core(i);
>                      cputopo.socket = cpu_to_socket(i);
> -- 
> 2.16.4
> 

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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Wei Liu]

On Thu, Aug 30, 2018 at 08:48:40AM +0100, Wei Liu wrote:
> On Wed, Aug 29, 2018 at 08:23:02PM +0200, Juergen Gross wrote:
> > The topology information obtainable via XEN_SYSCTL_cputopoinfo is
> > filled rather weird: the size of the array is derived from the highest
> > online cpu number, while the data is set to "invalid" for not present
> > cpus only.
> > 
> > With smt=0 the information for parked threads is all zero, so it should
> > be best to return "invalid" information for offline cpus.
> > 
> > On a dual core system without this patch xl info -n will print:

BTW I think it would be clearer if you say 2c4t, or in full -- 2 cores 4
threads.

Wei.

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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Jan Beulich]

>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
> The topology information obtainable via XEN_SYSCTL_cputopoinfo is
> filled rather weird: the size of the array is derived from the highest
> online cpu number, while the data is set to "invalid" for not present
> cpus only.
> 
> With smt=0 the information for parked threads is all zero, so it should
> be best to return "invalid" information for offline cpus.
> 
> On a dual core system without this patch xl info -n will print:
> 
> cpu_topology           :
> cpu:    core    socket     node
>   0:       0        0        0
>   1:       0        0        0
>   2:       1        0        0

But there's nothing wrong here. The interesting part is what would be
printed for CPU 3 (perhaps on a more than two cores system). After
all topology is valid irrespective of whether a CPU is online - it all
depends on whether the hypervisor still has the information available.
It is for a reason that cpu_smpboot_free() invalidates certain fields
only upon CPU removal:

    if ( remove )
    {
        c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
        c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
        c[cpu].compute_unit_id = INVALID_CUID;

On a 6-core system I see

cpu:    core    socket     node
  0:       0        0        0
  1:       0        0        0
  2:       1        0        0
  3:       1        0        0
  4:       2        0        0
  5:       2        0        0
  6:       8        0        0
  7:       8        0        0
  8:       9        0        0
  9:       9        0        0
 10:      10        0        0

which looks fine to me, apart from the missing info on CPU 11.

Jan



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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Juergen Gross]

On 30/08/18 10:16, Jan Beulich wrote:
>>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
>> The topology information obtainable via XEN_SYSCTL_cputopoinfo is
>> filled rather weird: the size of the array is derived from the highest
>> online cpu number, while the data is set to "invalid" for not present
>> cpus only.
>>
>> With smt=0 the information for parked threads is all zero, so it should
>> be best to return "invalid" information for offline cpus.
>>
>> On a dual core system without this patch xl info -n will print:
>>
>> cpu_topology           :
>> cpu:    core    socket     node
>>   0:       0        0        0
>>   1:       0        0        0
>>   2:       1        0        0
> 
> But there's nothing wrong here. The interesting part is what would be
> printed for CPU 3 (perhaps on a more than two cores system). After
> all topology is valid irrespective of whether a CPU is online - it all
> depends on whether the hypervisor still has the information available.
> It is for a reason that cpu_smpboot_free() invalidates certain fields
> only upon CPU removal:
> 
>     if ( remove )
>     {
>         c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
>         c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
>         c[cpu].compute_unit_id = INVALID_CUID;
> 
> On a 6-core system I see
> 
> cpu:    core    socket     node
>   0:       0        0        0
>   1:       0        0        0
>   2:       1        0        0
>   3:       1        0        0
>   4:       2        0        0
>   5:       2        0        0
>   6:       8        0        0
>   7:       8        0        0
>   8:       9        0        0
>   9:       9        0        0
>  10:      10        0        0
> 
> which looks fine to me, apart from the missing info on CPU 11.

I can change the patch to print the information for the offline cpus
(including the now missing ones), too.

What is the preferred solution?


Juergen

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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Wei Liu]

On Thu, Aug 30, 2018 at 10:31:16AM +0200, Juergen Gross wrote:
> On 30/08/18 10:16, Jan Beulich wrote:
> >>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
> >> The topology information obtainable via XEN_SYSCTL_cputopoinfo is
> >> filled rather weird: the size of the array is derived from the highest
> >> online cpu number, while the data is set to "invalid" for not present
> >> cpus only.
> >>
> >> With smt=0 the information for parked threads is all zero, so it should
> >> be best to return "invalid" information for offline cpus.
> >>
> >> On a dual core system without this patch xl info -n will print:
> >>
> >> cpu_topology           :
> >> cpu:    core    socket     node
> >>   0:       0        0        0
> >>   1:       0        0        0
> >>   2:       1        0        0
> > 
> > But there's nothing wrong here. The interesting part is what would be
> > printed for CPU 3 (perhaps on a more than two cores system). After
> > all topology is valid irrespective of whether a CPU is online - it all
> > depends on whether the hypervisor still has the information available.
> > It is for a reason that cpu_smpboot_free() invalidates certain fields
> > only upon CPU removal:
> > 
> >     if ( remove )
> >     {
> >         c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
> >         c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
> >         c[cpu].compute_unit_id = INVALID_CUID;
> > 
> > On a 6-core system I see
> > 
> > cpu:    core    socket     node
> >   0:       0        0        0
> >   1:       0        0        0
> >   2:       1        0        0
> >   3:       1        0        0
> >   4:       2        0        0
> >   5:       2        0        0
> >   6:       8        0        0
> >   7:       8        0        0
> >   8:       9        0        0
> >   9:       9        0        0
> >  10:      10        0        0
> > 
> > which looks fine to me, apart from the missing info on CPU 11.
> 
> I can change the patch to print the information for the offline cpus
> (including the now missing ones), too.
> 

That is fine too. I just don't like inconsistent output. :p

P.S. you probably want to add a new field to the existing interface to
indicate if a cpu is online.

Wei.

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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Jan Beulich]

>>> On 30.08.18 at 10:37, <wei.liu2@citrix.com> wrote:
> On Thu, Aug 30, 2018 at 10:31:16AM +0200, Juergen Gross wrote:
>> On 30/08/18 10:16, Jan Beulich wrote:
>> >>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
>> >> The topology information obtainable via XEN_SYSCTL_cputopoinfo is
>> >> filled rather weird: the size of the array is derived from the highest
>> >> online cpu number, while the data is set to "invalid" for not present
>> >> cpus only.
>> >>
>> >> With smt=0 the information for parked threads is all zero, so it should
>> >> be best to return "invalid" information for offline cpus.
>> >>
>> >> On a dual core system without this patch xl info -n will print:
>> >>
>> >> cpu_topology           :
>> >> cpu:    core    socket     node
>> >>   0:       0        0        0
>> >>   1:       0        0        0
>> >>   2:       1        0        0
>> > 
>> > But there's nothing wrong here. The interesting part is what would be
>> > printed for CPU 3 (perhaps on a more than two cores system). After
>> > all topology is valid irrespective of whether a CPU is online - it all
>> > depends on whether the hypervisor still has the information available.
>> > It is for a reason that cpu_smpboot_free() invalidates certain fields
>> > only upon CPU removal:
>> > 
>> >     if ( remove )
>> >     {
>> >         c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
>> >         c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
>> >         c[cpu].compute_unit_id = INVALID_CUID;
>> > 
>> > On a 6-core system I see
>> > 
>> > cpu:    core    socket     node
>> >   0:       0        0        0
>> >   1:       0        0        0
>> >   2:       1        0        0
>> >   3:       1        0        0
>> >   4:       2        0        0
>> >   5:       2        0        0
>> >   6:       8        0        0
>> >   7:       8        0        0
>> >   8:       9        0        0
>> >   9:       9        0        0
>> >  10:      10        0        0
>> > 
>> > which looks fine to me, apart from the missing info on CPU 11.
>> 
>> I can change the patch to print the information for the offline cpus
>> (including the now missing ones), too.
>> 
> 
> That is fine too. I just don't like inconsistent output. :p
> 
> P.S. you probably want to add a new field to the existing interface to
> indicate if a cpu is online.

And if we extend the interface anyway, also the thread ID (as iirc
pointed out as missing recently by George).

Jan



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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Juergen Gross]

On 30/08/18 10:44, Jan Beulich wrote:
>>>> On 30.08.18 at 10:37, <wei.liu2@citrix.com> wrote:
>> On Thu, Aug 30, 2018 at 10:31:16AM +0200, Juergen Gross wrote:
>>> On 30/08/18 10:16, Jan Beulich wrote:
>>>>>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
>>>>> The topology information obtainable via XEN_SYSCTL_cputopoinfo is
>>>>> filled rather weird: the size of the array is derived from the highest
>>>>> online cpu number, while the data is set to "invalid" for not present
>>>>> cpus only.
>>>>>
>>>>> With smt=0 the information for parked threads is all zero, so it should
>>>>> be best to return "invalid" information for offline cpus.
>>>>>
>>>>> On a dual core system without this patch xl info -n will print:
>>>>>
>>>>> cpu_topology           :
>>>>> cpu:    core    socket     node
>>>>>   0:       0        0        0
>>>>>   1:       0        0        0
>>>>>   2:       1        0        0
>>>>
>>>> But there's nothing wrong here. The interesting part is what would be
>>>> printed for CPU 3 (perhaps on a more than two cores system). After
>>>> all topology is valid irrespective of whether a CPU is online - it all
>>>> depends on whether the hypervisor still has the information available.
>>>> It is for a reason that cpu_smpboot_free() invalidates certain fields
>>>> only upon CPU removal:
>>>>
>>>>     if ( remove )
>>>>     {
>>>>         c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
>>>>         c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
>>>>         c[cpu].compute_unit_id = INVALID_CUID;
>>>>
>>>> On a 6-core system I see
>>>>
>>>> cpu:    core    socket     node
>>>>   0:       0        0        0
>>>>   1:       0        0        0
>>>>   2:       1        0        0
>>>>   3:       1        0        0
>>>>   4:       2        0        0
>>>>   5:       2        0        0
>>>>   6:       8        0        0
>>>>   7:       8        0        0
>>>>   8:       9        0        0
>>>>   9:       9        0        0
>>>>  10:      10        0        0
>>>>
>>>> which looks fine to me, apart from the missing info on CPU 11.
>>>
>>> I can change the patch to print the information for the offline cpus
>>> (including the now missing ones), too.
>>>
>>
>> That is fine too. I just don't like inconsistent output. :p
>>
>> P.S. you probably want to add a new field to the existing interface to
>> indicate if a cpu is online.
> 
> And if we extend the interface anyway, also the thread ID (as iirc
> pointed out as missing recently by George).

Ha, yes!


Juergen

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

Re: [PATCH 2/2] xen: fill topology info for online cpus only

[Jan Beulich]

>>> On 30.08.18 at 10:31, <jgross@suse.com> wrote:
> On 30/08/18 10:16, Jan Beulich wrote:
>>>>> On 29.08.18 at 20:23, <jgross@suse.com> wrote:
>>> The topology information obtainable via XEN_SYSCTL_cputopoinfo is
>>> filled rather weird: the size of the array is derived from the highest
>>> online cpu number, while the data is set to "invalid" for not present
>>> cpus only.
>>>
>>> With smt=0 the information for parked threads is all zero, so it should
>>> be best to return "invalid" information for offline cpus.
>>>
>>> On a dual core system without this patch xl info -n will print:
>>>
>>> cpu_topology           :
>>> cpu:    core    socket     node
>>>   0:       0        0        0
>>>   1:       0        0        0
>>>   2:       1        0        0
>> 
>> But there's nothing wrong here. The interesting part is what would be
>> printed for CPU 3 (perhaps on a more than two cores system). After
>> all topology is valid irrespective of whether a CPU is online - it all
>> depends on whether the hypervisor still has the information available.
>> It is for a reason that cpu_smpboot_free() invalidates certain fields
>> only upon CPU removal:
>> 
>>     if ( remove )
>>     {
>>         c[cpu].phys_proc_id = XEN_INVALID_SOCKET_ID;
>>         c[cpu].cpu_core_id = XEN_INVALID_CORE_ID;
>>         c[cpu].compute_unit_id = INVALID_CUID;
>> 
>> On a 6-core system I see
>> 
>> cpu:    core    socket     node
>>   0:       0        0        0
>>   1:       0        0        0
>>   2:       1        0        0
>>   3:       1        0        0
>>   4:       2        0        0
>>   5:       2        0        0
>>   6:       8        0        0
>>   7:       8        0        0
>>   8:       9        0        0
>>   9:       9        0        0
>>  10:      10        0        0
>> 
>> which looks fine to me, apart from the missing info on CPU 11.
> 
> I can change the patch to print the information for the offline cpus
> (including the now missing ones), too.
> 
> What is the preferred solution?

Well, by implication from my earlier reply I think adding the missing
CPU's info would be better. Let's see what others think.

Jan



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