[libvirt] [PATCH 1/1] Remove contiguous CPU indexes assumption

[Li Zhang]

On 2013年02月28日 07:13, Eric Blake wrote:
> On 02/27/2013 05:13 AM, Li Zhang wrote:
>> Hi Eric,
>>
>> This should belong to bug-fix, could it be pushed to 1.0.3?
> Without any test case under 'make check' that exposes the failure, I'm a
> bit worried that this might cause regressions on other setups. I'm not
> seven sure I even understand the scope of the problem.  Is it something
> specific to running a qemu-system-ppc64 emulator (but can be reproduced
> on any host architecture), or is it specific to running on a powerpc
> host (but happens for any version of qemu-* target architecture), or is
> it something else altogether?
So sorry to see the failure.
I tried to 'make check' with my patch on ppc64, all of these cases pass.
For x86, I tried it with on x86 server, and pulled the latest version of 
libvirt.
Even without my patch, it also fails during execute ./virshtest and the 
tests with cpuset.

+++ out    2013-02-28 13:02:14.290794080 +0800
@@ -1,2 +1,3 @@
  error: vcpupin: Invalid or missing vCPU number.

--- exp    2013-02-28 13:02:14.321794080 +0800
+++ out    2013-02-28 13:02:14.320794080 +0800
@@ -1,2 +1,3 @@
  error: vcpupin: Invalid vCPU number.
FAIL: vcpupin

I think this problem is specific ppc64.

I did some experiment on X86.
X86 machine:  4 CPUs (0~3)

I disable CPU1 and CPU2 by writing 0 to /sys/.../cpuX/online.
Only  0 and 3 are online.

Although only 2 cpus are online, but after executing "query-cpus", it 
can get all the information of them.
  [{"return": [{"current": true, "CPU": 0, "pc": 4294967280, "halted": 
false, "thread_id": 31831}, {"current": false, "CPU": 1, "pc": 
4294967280, "halted": false, "thread_id": 31832}, {"current": false, 
"CPU": 2, "pc": 4294967280, "halted": false, "thread_id": 31833}, 
{"current": false, "CPU": 3, "pc": 4294967280, "halted": false, 
"thread_id": 31834}], "id": "libvirt-3"}]

Qemu can expose all of these CPUs offline for X86.
But for ppc64, it can't expose these offline CPUs. The following is the 
return value from ppc64.
[{"current": true, "CPU": 0, "nip": 256, "halted": false, "thread_id": 
25964}, {"current": false, "CPU": 4, "nip": 256, "halted": true, 
"thread_id": 25965}, {"current": false, "CPU": 8, "nip": 256, "halted": 
true, "thread_id": 25966}, {"current": false, "CPU": 12, "nip": 256, 
"halted": true, "thread_id": 25967}], "id": "libvirt-3"}

>
> We have test framework in place to allow replaying of a QMP JSON
> response and seeing how qemu_monitor_json.c deals with it; what I'd
> really like to see is a side-by-side comparison of what the QMP
> 'query-cpus' command returns for a guest with multiple vcpus on a setup
> where you are seeing the problem, when compared to a setup that does not
> have the issue.  You can get at this with virsh qemu-monitor-command
> $dom '{"execute":"query-cpus"}', if that helps.
Thanks a lot.  Tried with ppc64,  it is different from x86 even with 
some CPUs disabled.
>
> To help you out, here's what I got for a guest using 3 vcpus on my
> x86_64 host machine and using the qemu-kvm binary:
>
> # virsh qemu-monitor-command guest '{"execute":"query-cpus"}'
> {"return":[{"current":true,"CPU":0,"pc":1025829,"halted":false,"thread_id":5849},{"current":false,"CPU":1,"pc":1005735,"halted":true,"thread_id":5850},{"current":false,"CPU":2,"pc":1005735,"halted":true,"thread_id":5851}],"id":"libvirt-9"}
>
> That is, your patch might be right, but I have not yet been convinced
> that it is right; and while things may currently be broken on ppc, it is
> not a recent breakage, so being conservative and either proving the fix
> is right (via testsuite addition) or deferring the fix until post 1.0.3
> seems like safer alternatives.  Or maybe someone else will chime in and
> agree to take it now, without waiting for my desired burden of proof.
>
OK, I will tried to see the testsuite problems for x86 even without my 
patch.
I couldn't think of what kind of problems will this patch cause now.

Really thank you, Eric for your review and suggestion.
I don't know whether others have suggestions about my patch.

Any suggestion is appreciated.

Thanks.
  --Li