Skip to contentSkip to conversation

use full-size cpumask for vcpu-pin

PATCH In review Juergen Gross 3 participants 13 messages Aug 2, 2010 – Aug 13, 2010 Arm/actions semi architecture

lore
Share & tools
lore

Juergen Gross

author
#0 lore raw

PATCH 1/1 use full-size cpumask for vcpu-pin

Hi,

attached patch solves a problem with vcpu-pinning and hot-plug of cpus:

If a vcpu is unpinned via

xl vcpu-pin <domain> <vcpu> all

on a system with 64 cpus and later other cpus are plugged in, this vcpu will be restricted to the first 64 cpus of the system.

The reason is the allocation of the cpumap for pinning: the size is only for the ACTUAL number of physical cpus in the system, not the possible number. The solution is to allocate a cpumap for up to NR_CPUS.

Repairing xm vcpu-pin is much harder and not covered by this patch, but the problem can be avoided by calling

xm vcpu-pin <domain> <vcpu> 0-255

instead (all is hard-wired to 0-63 now).

Juergen

Signature · 5 lines
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: [email protected]
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html
Signed-off-by [email protected]
tools/libxl/libxl.c +2 -1
diff -r 3263d0ff9476 tools/libxl/libxl.c
@@ -2479,6 +2479,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, l
     physinfo->max_cpu_id = xcphysinfo.max_cpu_id;
     physinfo->nr_cpus = xcphysinfo.nr_cpus;
     physinfo->cpu_khz = xcphysinfo.cpu_khz;
+    physinfo->max_phys_cpus = xcphysinfo.max_phys_cpus;
     physinfo->total_pages = xcphysinfo.total_pages;
     physinfo->free_pages = xcphysinfo.free_pages;
     physinfo->scrub_pages = xcphysinfo.scrub_pages;

@@ -2550,7 +2551,7 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting physinfo");
         return NULL;
     }
-    *cpusize = physinfo.max_cpu_id + 1;
+    *cpusize = physinfo.max_phys_cpus + 1;
     ptr = libxl_calloc(ctx, domaininfo.max_vcpu_id + 1, sizeof (libxl_vcpuinfo));
     if (!ptr) {
         return NULL;
tools/libxl/libxl.h +1 -0
diff -r 3263d0ff9476 tools/libxl/libxl.h
@@ -581,6 +581,7 @@ typedef struct {
     uint32_t max_cpu_id;
     uint32_t nr_cpus;
     uint32_t cpu_khz;
+    uint32_t max_phys_cpus;
 
     uint64_t total_pages;
     uint64_t free_pages;
tools/libxl/xl_cmdimpl.c +4 -4
diff -r 3263d0ff9476 tools/libxl/xl_cmdimpl.c
@@ -3297,7 +3297,7 @@ void vcpupin(char *d, const char *vcpu, 
         goto vcpupin_out1;
     }
 
-    cpumap = calloc(physinfo.max_cpu_id + 1, sizeof (uint64_t));
+    cpumap = calloc(physinfo.max_phys_cpus + 1, sizeof (uint64_t));
     if (!cpumap) {
         goto vcpupin_out1;
     }

@@ -3325,12 +3325,12 @@ void vcpupin(char *d, const char *vcpu, 
         }
     }
     else {
-        memset(cpumap, -1, sizeof (uint64_t) * (physinfo.max_cpu_id + 1));
+        memset(cpumap, -1, sizeof (uint64_t) * (physinfo.max_phys_cpus + 1));
     }
 
     if (vcpuid != -1) {
         if (libxl_set_vcpuaffinity(&ctx, domid, vcpuid,
-                                   cpumap, physinfo.max_cpu_id + 1) == -1) {
+                                   cpumap, physinfo.max_phys_cpus + 1) == -1) {
             fprintf(stderr, "Could not set affinity for vcpu `%u'.\n", vcpuid);
         }
     }

@@ -3341,7 +3341,7 @@ void vcpupin(char *d, const char *vcpu, 
         }
         for (; nb_vcpu > 0; --nb_vcpu, ++vcpuinfo) {
             if (libxl_set_vcpuaffinity(&ctx, domid, vcpuinfo->vcpuid,
-                                       cpumap, physinfo.max_cpu_id + 1) == -1) {
+                                       cpumap, physinfo.max_phys_cpus + 1) == -1) {
                 fprintf(stderr, "libxl_list_vcpu failed on vcpu `%u'.\n", vcpuinfo->vcpuid);
             }
         }
tools/python/xen/lowlevel/xc/xc.c +2 -2
diff -r 3263d0ff9476 tools/python/xen/lowlevel/xc/xc.c
@@ -241,7 +241,7 @@ static PyObject *pyxc_vcpu_setaffinity(X
     if ( xc_physinfo(self->xc_handle, &info) != 0 )
         return pyxc_error_to_exception(self->xc_handle);
   
-    nr_cpus = info.nr_cpus;
+    nr_cpus = info.max_phys_cpus;
 
     size = (nr_cpus + cpumap_size * 8 - 1)/ (cpumap_size * 8);
     cpumap = malloc(cpumap_size * size);

@@ -400,7 +400,7 @@ static PyObject *pyxc_vcpu_getinfo(XcObj
 
     if ( xc_physinfo(self->xc_handle, &pinfo) != 0 ) 
         return pyxc_error_to_exception(self->xc_handle);
-    nr_cpus = pinfo.nr_cpus;
+    nr_cpus = pinfo.max_phys_cpus;
 
     rc = xc_vcpu_getinfo(self->xc_handle, dom, vcpu, &info);
     if ( rc < 0 )
xen/arch/x86/sysctl.c +1 -0
diff -r 3263d0ff9476 xen/arch/x86/sysctl.c
@@ -68,6 +68,7 @@ long arch_do_sysctl(
         pi->free_pages = avail_domheap_pages();
         pi->scrub_pages = 0;
         pi->cpu_khz = cpu_khz;
+        pi->max_phys_cpus = NR_CPUS;
         memcpy(pi->hw_cap, boot_cpu_data.x86_capability, NCAPINTS*4);
         if ( hvm_enabled )
             pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm;
xen/include/public/sysctl.h +1 -0
diff -r 3263d0ff9476 xen/include/public/sysctl.h
@@ -96,6 +96,7 @@ struct xen_sysctl_physinfo {
     uint32_t nr_cpus, max_cpu_id;
     uint32_t nr_nodes, max_node_id;
     uint32_t cpu_khz;
+    uint32_t max_phys_cpus;
     uint64_aligned_t total_pages;
     uint64_aligned_t free_pages;
     uint64_aligned_t scrub_pages;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw

Juergen Gross writes ("[Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 3 lines

The reason is the allocation of the cpumap for pinning: the size is only for the ACTUAL number of physical cpus in the system, not the possible number. The solution is to allocate a cpumap for up to NR_CPUS.

Thanks for this patch. However, this doesn't seem to be even slightly backwards-compatible, because it changes the layout of the physinfo sysctl response.

Ian.

#2 lore raw
Quoted 9 lines · Ian Jackson

Juergen Gross writes ("[Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 3 lines

The reason is the allocation of the cpumap for pinning: the size is only for the ACTUAL number of physical cpus in the system, not the possible number. The solution is to allocate a cpumap for up to NR_CPUS.

Thanks for this patch. However, this doesn't seem to be even slightly backwards-compatible, because it changes the layout of the physinfo sysctl response.

The domctl and sysctl hypercalls do not need to be backward compatible across major Xen releases. All other hypercalls absolutely do.

-- Keir

author
#3 lore raw
In reply to Ian Jackson
Quoted 8 lines · Ian Jackson

Juergen Gross writes ("[Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 3 lines

The reason is the allocation of the cpumap for pinning: the size is only for the ACTUAL number of physical cpus in the system, not the possible number. The solution is to allocate a cpumap for up to NR_CPUS.

Thanks for this patch. However, this doesn't seem to be even slightly backwards-compatible, because it changes the layout of the physinfo sysctl response.

No, it doesn't change the layout. The new structure member just fills a hole which was already there due to alignment of the next member.

Juergen

Signature · 5 lines
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: [email protected]
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html
Acked-by on v1 1/1
#4 lore raw
In reply to Juergen Gross

Juergen Gross writes ("[Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 1 line

attached patch solves a problem with vcpu-pinning and hot-plug of cpus:

Thanks. This is a mixed tools/hypervisor patch. We've discussed it and it seems good to me. Keir, would you care to apply it, or would you like it to go through the tools tree ?

Acked-by Ian Jackson <[email protected]>

Ian.

#5 lore raw
Quoted 9 lines · Ian Jackson

Juergen Gross writes ("[Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 1 line

attached patch solves a problem with vcpu-pinning and hot-plug of cpus:

Thanks. This is a mixed tools/hypervisor patch. We've discussed it and it seems good to me. Keir, would you care to apply it, or would you like it to go through the tools tree ?

Acked-by Ian Jackson <[email protected]>

Actually, now I look at it, I have to NACK the patch. Sorry I didn't look closely enough earlier. I think the bug can be addressed without any hypervisor changes: when vcpu-pinning, the tools can quite correctly pass a cpumask to Xen just big enough to express just the CPUs in the new affinity set. If the resulting mask is too narrow to address all CPUs in the system, then Xen will pad it out with zeroes. If the resulting mask is too wide, Xen will simply truncate it. All this is done silently at the time of the setvcpuaffinity hypercall. Hence, Juergen's hypervisor changes are really unnecessary, and a neater tools fix could probably be arrived at without the hypervisor changes.

-- Keir

Quoted 6 lines

Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#6 lore raw

Keir Fraser writes ("Re: [Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 8 lines

Actually, now I look at it, I have to NACK the patch. Sorry I didn't look closely enough earlier. I think the bug can be addressed without any hypervisor changes: when vcpu-pinning, the tools can quite correctly pass a cpumask to Xen just big enough to express just the CPUs in the new affinity set. If the resulting mask is too narrow to address all CPUs in the system, then Xen will pad it out with zeroes. If the resulting mask is too wide, Xen will simply truncate it. All this is done silently at the time of the setvcpuaffinity hypercall.

The difficulty is, as I understand it, how to express the mask "all CPUs present and future". Since the number of pcpus may be very large. The tools presumably shouldn't pass a kilobyte of 0xff :-).

Ian.

#7 lore raw
Quoted 11 lines · Ian Jackson

Keir Fraser writes ("Re: [Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 8 lines

Actually, now I look at it, I have to NACK the patch. Sorry I didn't look closely enough earlier. I think the bug can be addressed without any hypervisor changes: when vcpu-pinning, the tools can quite correctly pass a cpumask to Xen just big enough to express just the CPUs in the new affinity set. If the resulting mask is too narrow to address all CPUs in the system, then Xen will pad it out with zeroes. If the resulting mask is too wide, Xen will simply truncate it. All this is done silently at the time of the setvcpuaffinity hypercall.

Quoted 3 lines

The difficulty is, as I understand it, how to express the mask "all CPUs present and future". Since the number of pcpus may be very large. The tools presumably shouldn't pass a kilobyte of 0xff :-).

Since Xen discards any CPUs which are not online at the time of the setvcpuaffinity hypercall, expressing "all CPUs present and future" is a bit pointless.

-- Keir

#8 lore raw
Quoted 9 lines · Keir Fraser
Quoted 3 lines · Ian Jackson

The difficulty is, as I understand it, how to express the mask "all CPUs present and future". Since the number of pcpus may be very large. The tools presumably shouldn't pass a kilobyte of 0xff :-).

Since Xen discards any CPUs which are not online at the time of the setvcpuaffinity hypercall, expressing "all CPUs present and future" is a bit pointless.

Hm, no, I'm talking rubbish. Argh. Okay, his patch is fine then. :-D

You can check it in to xen-unstable-tools.hg.

-- Keir

Quoted 2 lines

-- Keir

#9 lore raw
Quoted 7 lines · Keir Fraser
Quoted 3 lines

Since Xen discards any CPUs which are not online at the time of the setvcpuaffinity hypercall, expressing "all CPUs present and future" is a bit pointless.

Hm, no, I'm talking rubbish. Argh. Okay, his patch is fine then. :-D

You can check it in to xen-unstable-tools.hg.

One suggestion: that we rename the syctl.physinfo field 'max_phys_cpus' to 'max_possible_cpus'. The 'phys' is kind of redundant since this is the physinfo sysctl, and 'possible' provides a better hint that this fields indicates maximum possible supported CPUs now and forever on this boot of the system.

-- Keir

#10 lore raw

Keir Fraser writes ("Re: [Xen-devel] [Patch] use full-size cpumask for vcpu-pin"):

Quoted 3 lines · Keir Fraser
Quoted 2 lines

Hm, no, I'm talking rubbish. Argh. Okay, his patch is fine then. :-D You can check it in to xen-unstable-tools.hg.

:-).

Quoted 5 lines

One suggestion: that we rename the syctl.physinfo field 'max_phys_cpus' to 'max_possible_cpus'. The 'phys' is kind of redundant since this is the physinfo sysctl, and 'possible' provides a better hint that this fields indicates maximum possible supported CPUs now and forever on this boot of the system.

Right, willdo.

Ian.

#11 lore raw
In reply to Keir Fraser
Quoted 8 lines · Keir Fraser
Quoted 6 lines · Keir Fraser
Quoted 3 lines

Since Xen discards any CPUs which are not online at the time of the setvcpuaffinity hypercall, expressing "all CPUs present and future" is a bit pointless.

Hm, no, I'm talking rubbish. Argh. Okay, his patch is fine then. :-D

Quoted 7 lines
Quoted 1 line

You can check it in to xen-unstable-tools.hg.

One suggestion: that we rename the syctl.physinfo field 'max_phys_cpus' to 'max_possible_cpus'. The 'phys' is kind of redundant since this is the physinfo sysctl, and 'possible' provides a better hint that this fields indicates maximum possible supported CPUs now and forever on this boot of the system.

Even better, let's not introduce a new field at all, and let's always set sysctl.phys_info.max_cpu_id to NR_CPUS-1. Then I don't think any tools changes are needed.

Yeah, that's what we should do. I will make a patch for that.

-- Keir

Quoted 2 lines

-- Keir

#12 lore raw
Quoted 11 lines · Keir Fraser
Quoted 5 lines

One suggestion: that we rename the syctl.physinfo field 'max_phys_cpus' to 'max_possible_cpus'. The 'phys' is kind of redundant since this is the physinfo sysctl, and 'possible' provides a better hint that this fields indicates maximum possible supported CPUs now and forever on this boot of the system.

Even better, let's not introduce a new field at all, and let's always set sysctl.phys_info.max_cpu_id to NR_CPUS-1. Then I don't think any tools changes are needed.

Yeah, that's what we should do. I will make a patch for that.

Done as xen-unstable:f6e1a597a92f. At this late stage it's probably a bit too subtle for backport to 4.0.1 unfortunately (I hate to wonder if there could be subtle side effects on xend's usage of these max_*_id fields).

-- Keir

Quoted 5 lines

-- Keir

Quoted 2 lines

-- Keir