On 10/10/2012 09:18 AM, Viktor Mihajlovski wrote: > Hi, > > in order to use the APIs listed below it is necessary for a client to > know the maximum number of node CPUs which is passed via the maplen > argument. > > virDomainGetEmulatorPinInfo > virDomainGetVcpuPinInfo > virDomainGetVcpus > virDomainPinEmulator > virDomainPinVcpu > virDomainPinVcpuFlags > > The current approach uses virNodeGetInfo to determine the maximum CPU > number. This can lead to incorrect results if not all node CPUs are > online. The maximum CPU number should always be the number of CPUs > present on the host, regardless of their online/offline state. > > This is not a display problem only, because it is also not possible to > explicitly pin the virtual CPU to host CPUs 0 and 2, due to the > truncated CPU mask. Indeed, offline host cpus introduce lots of oddities. Your idea of a new API makes sense, although there's still some details to work out. > > PROPOSAL: > > To help solve the issue above I suggest two new public API functions: > > int > virNodeGetCpuNum(virConnectPtr conn); > > returning the number of present CPUs on the host or -1 upon failure > and > > int > virNodeGetCpuMap(virConnectPtr conn, unsigned char * cpumap, int maplen); Lately, we have been favoring APIs that auto-allocate, instead of making the user call two separate APIs and risk a race where the answer from the first call is no longer matching reality during the second call. That is, if I hot[un]plug a CPU in between the two proposed API calls, will my maplen of the second call as learned from the first call still be accurate? And why should I have to call two functions? I think it might be better to write: int virNodeGetCpuMap(virConnectPtr conn, unsigned char **cpumap); where *cpumap will be malloc'd by libvirt and the return value be the maplen (also allow cpumap==NULL on entry to just query the map len without also allocating). Then the result is -1/maplen instead of -1/0 for failure/success. > > returning the number of present CPUs or -1 on failure and storing a bit > map of real CPUs as described in virDomainPinVcpu in cpumap. The bits in > the bit map are set to 1 for online CPUs and set to 0 for offline CPUs. > > Implementation is facilitated by the function nodeGetCPUmap in nodeinfo.c. > > Clients can use virNodeGetCpuNum to properly determine the maximum > number of node CPUs and the online/offline information. > > Thanks for your comments. Are you interested in writing patches to implement this new API, if others agree with my alternate single-API signature?

On 10/10/2012 10:02 PM, Osier Yang wrote: >> Are you interested in writing patches to implement this new API, if >> others agree with my alternate single-API signature? > > Agreed. A single-API and allocating the cpumap inside libvirt without > the need to known the maplen avoids the race. > > But a flags support won't bite us, assuming someone just want to get > the cpumap of the onlined CPUs, or offlined CPUs. Indeed, shame on me for not remember a flags argument for any possible future extensions. > > So how about: > > int > virNodeGetCpuMap(virConnectPtr conn, > unsigned char **cpumap, > unsigned int flags); Yes, that's better, even if flags must always be 0 for now.

That starts to feel rather complex.

On the other hand, you have a point that returning maplen (number of bytes allocated in the map) does NOT tell you how many cpus are online without doing a population count of the result; worse, it cannot tell you how many cpus are offline (a maplen of 1 is returned whether I have 4 cpus or 8 cpus, so if the map contains the value 0xb, does that mean I had a 4 cpu machine with cpu2 off, or an 8 cpu machine with cpu2 and cpu4-7 off?). So allowing the user to optionally ask how many cpus are online or offline might indeed also be useful, in addition to the maplen needed to return the bitmap.

Are you interested in writing patches to implement this new API, if others agree with my alternate single-API signature?

Hi, in order to use the APIs listed below it is necessary for a client to know the maximum number of node CPUs which is passed via the maplen argument. virDomainGetEmulatorPinInfo virDomainGetVcpuPinInfo virDomainGetVcpus virDomainPinEmulator virDomainPinVcpu virDomainPinVcpuFlags The current approach uses virNodeGetInfo to determine the maximum CPU number. This can lead to incorrect results if not all node CPUs are online. The maximum CPU number should always be the number of CPUs present on the host, regardless of their online/offline state.

The following example illustrates the issue: Host has 3 logical CPUs, 1 socket, 3 cores, 1 thread. Guest has 1 virtual CPU and is started while all 3 host CPUs are online. $ virsh vcpuinfo guest VCPU: 0 CPU: 0 State: running CPU time: 35.4s CPU Affinity: yyy $ echo 0 > /sys/devices/system/cpu/cpu1/online $ virsh vcpuinfo guest VCPU: 0 CPU: 0 State: running CPU time: 35.5s CPU Affinity: y- The correct display for CPU affinity would have been y-y, as the guest continues to use CPUs 0 and 2. This is not a display problem only, because it is also not possible to explicitly pin the virtual CPU to host CPUs 0 and 2, due to the truncated CPU mask.

PROPOSAL: To help solve the issue above I suggest two new public API functions: int virNodeGetCpuNum(virConnectPtr conn); returning the number of present CPUs on the host or -1 upon failure and int virNodeGetCpuMap(virConnectPtr conn, unsigned char * cpumap, int maplen); returning the number of present CPUs or -1 on failure and storing a bit map of real CPUs as described in virDomainPinVcpu in cpumap. The bits in the bit map are set to 1 for online CPUs and set to 0 for offline CPUs. Implementation is facilitated by the function nodeGetCPUmap in nodeinfo.c. Clients can use virNodeGetCpuNum to properly determine the maximum number of node CPUs and the online/offline information. Thanks for your comments.