1:44 a.m.
From 8123c5d64f940fa0fb0de32fc5e68035980b6b01 Mon Sep 17 00:00:00 2001
From: WangYufei <james.wangyufei(a)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(a)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
4:08 a.m.
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(a)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
4:42 a.m.
-----Original Message-----
From: Martin Kletzander [mailto:mkletzan@redhat.com]
Sent: Thursday, February 13, 2014 6:08 PM
To: Wangyufei (James)
Cc: libvir-list(a)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(a)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
4:57 a.m.
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(a)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(a)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 :|
10:29 a.m.
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(a)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
3924
days inactive
3935
days old
4 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Martin Kletzander -
Wangyufei (James)