[libvirt] [PATCH] cpu: break out when a right cpuCandidate found
From 8123c5d64f940fa0fb0de32fc5e68035980b6b01 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Thu, 13 Feb 2014 07:17:11 +0000 Subject: [PATCH] cpu: break out when a right cpuCandidate found
In function x86Decode there's a code segment in while cycle like this: if (cpuModel == NULL || cpuModel->nfeatures > cpuCandidate->nfeatures) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; } else { virCPUDefFree(cpuCandidate); } when it finds the right cpuCandidate, it doesn't break out the cycle, but continues run in it, and cpuModel will never get a new value, it's meaningless. It should break out when a right cpuCndidate found. Signed-off-by: WangYufei <james.wangyufei@huawei.com> --- src/cpu/cpu_x86.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 56080ef..546b757 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1558,6 +1558,7 @@ x86Decode(virCPUDefPtr cpu, virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; + break; } else { virCPUDefFree(cpuCandidate); } -- 1.7.12.4 Best Regards, -WangYufei
On Thu, Feb 13, 2014 at 07:44:20AM +0000, Wangyufei (James) wrote:
From 8123c5d64f940fa0fb0de32fc5e68035980b6b01 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Thu, 13 Feb 2014 07:17:11 +0000 Subject: [PATCH] cpu: break out when a right cpuCandidate found
In function x86Decode there's a code segment in while cycle like this: if (cpuModel == NULL || cpuModel->nfeatures > cpuCandidate->nfeatures) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; } else { virCPUDefFree(cpuCandidate); } when it finds the right cpuCandidate, it doesn't break out the cycle, but continues run in it, and cpuModel will never get a new value, it's meaningless. It should break out when a right cpuCndidate found.
Inside this condition, the code doesn't always choose the perfect candidate. You don't consider a situation when the cycle continues and the next candidate model is the preferred one, thus satisfies previous condition, which looks like this: if (preferred && STREQ(cpuCandidate->model, preferred)) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; break; } Where the "perfect" cpuModel is found, used and the condition breaks (appropriately this time). But I could also misunderstood the code. Martin
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Thursday, February 13, 2014 6:08 PM To: Wangyufei (James) Cc: libvir-list@redhat.com; Wangrui (K); Zhaoyanbin (A) Subject: Re: [libvirt] [PATCH] cpu: break out when a right cpuCandidate found
On Thu, Feb 13, 2014 at 07:44:20AM +0000, Wangyufei (James) wrote:
From 8123c5d64f940fa0fb0de32fc5e68035980b6b01 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Thu, 13 Feb 2014 07:17:11 +0000 Subject: [PATCH] cpu: break out when a right cpuCandidate found
In function x86Decode there's a code segment in while cycle like this: if (cpuModel == NULL || cpuModel->nfeatures > cpuCandidate->nfeatures) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; } else { virCPUDefFree(cpuCandidate); } when it finds the right cpuCandidate, it doesn't break out the cycle, but continues run in it, and cpuModel will never get a new value, it's meaningless. It should break out when a right cpuCndidate found.
Inside this condition, the code doesn't always choose the perfect candidate. You don't consider a situation when the cycle continues and the next candidate model is the preferred one, thus satisfies previous condition, which looks like this:
if (preferred && STREQ(cpuCandidate->model, preferred)) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; break; }
Where the "perfect" cpuModel is found, used and the condition breaks (appropriately this time). But I could also misunderstood the code.
Well, thank you for your reply. I have seen preferred, but there's no where to modify the value of preferred in the cycle. So the preferred is a fixed value. In this case, I can make it better to mdify the patch like this: --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1558,6 +1558,7 @@ x86Decode(virCPUDefPtr cpu, virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; + if (!preferred) + break; } else { virCPUDefFree(cpuCandidate); } In my situation： virQEMUCapsInitCPU ->cpuDecode ->x86Decode if (!(data = cpuNodeData(arch)) || cpuDecode(cpu, data, NULL, 0, NULL) < 0) goto cleanup; preferred is always NULL. So we can do it better.
Martin
On Thu, Feb 13, 2014 at 10:42:01AM +0000, Wangyufei (James) wrote:
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Thursday, February 13, 2014 6:08 PM To: Wangyufei (James) Cc: libvir-list@redhat.com; Wangrui (K); Zhaoyanbin (A) Subject: Re: [libvirt] [PATCH] cpu: break out when a right cpuCandidate found
On Thu, Feb 13, 2014 at 07:44:20AM +0000, Wangyufei (James) wrote:
From 8123c5d64f940fa0fb0de32fc5e68035980b6b01 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Thu, 13 Feb 2014 07:17:11 +0000 Subject: [PATCH] cpu: break out when a right cpuCandidate found
In function x86Decode there's a code segment in while cycle like this: if (cpuModel == NULL || cpuModel->nfeatures > cpuCandidate->nfeatures) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; } else { virCPUDefFree(cpuCandidate); } when it finds the right cpuCandidate, it doesn't break out the cycle, but continues run in it, and cpuModel will never get a new value, it's meaningless. It should break out when a right cpuCndidate found.
Inside this condition, the code doesn't always choose the perfect candidate. You don't consider a situation when the cycle continues and the next candidate model is the preferred one, thus satisfies previous condition, which looks like this:
if (preferred && STREQ(cpuCandidate->model, preferred)) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; break; }
Where the "perfect" cpuModel is found, used and the condition breaks (appropriately this time). But I could also misunderstood the code.
Well, thank you for your reply. I have seen preferred, but there's no where to modify the value of preferred in the cycle. So the preferred is a fixed value. In this case, I can make it better to mdify the patch like this: --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1558,6 +1558,7 @@ x86Decode(virCPUDefPtr cpu, virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; + if (!preferred) + break; } else { virCPUDefFree(cpuCandidate); }
In my situation： virQEMUCapsInitCPU ->cpuDecode ->x86Decode if (!(data = cpuNodeData(arch)) || cpuDecode(cpu, data, NULL, 0, NULL) < 0) goto cleanup; preferred is always NULL. So we can do it better.
I think I'd like to see a test case which demonstrates the flawed behaviour and so proves the fix works. We already have a file tests/cputest.c in which to put such test cases. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Feb 13, 2014 at 07:44:20 +0000, Wangyufei (James) wrote:
From 8123c5d64f940fa0fb0de32fc5e68035980b6b01 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Thu, 13 Feb 2014 07:17:11 +0000 Subject: [PATCH] cpu: break out when a right cpuCandidate found
In function x86Decode there's a code segment in while cycle like this: if (cpuModel == NULL || cpuModel->nfeatures > cpuCandidate->nfeatures) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; cpuData = candidate->data; } else { virCPUDefFree(cpuCandidate); } when it finds the right cpuCandidate, it doesn't break out the cycle, but continues run in it, and cpuModel will never get a new value, it's meaningless. It should break out when a right cpuCndidate found.
That's mainly because we don't know we already have the right candidate. The next one may actually be better. On other words, the cpuCandidate we get in later iterations may satisfy the cpuModel->nfeatures > cpuCandidate->nfeatures and thus it may be better than cpuModel stored in earlier iterations. What are you trying to fix here? Are you just trying to optimize the code or do you see a wrong CPU model to be selected in some situations? Jirka
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Martin Kletzander -
Wangyufei (James)