[libvirt] [PATCH 0/2] Fix the display of *virsh vcpuinfo* command.
Hi, I found some confusion in display of commad *virsh vcpuinfo domname* . There is no diffrence between a cpu offline and a cpu online but not be set for the affinity of vcpu, both are '-'. But users will find the difference of them in using them for a same action (such as vcpupin) and be confused. Example: [root@yds-pc ~]# echo "0" >/sys/devices/system/cpu/cpu2/online [root@yds-pc ~]# cat /proc/cpuinfo |grep processor processor : 0 processor : 1 processor : 3 [root@yds-pc ~]# virsh vcpupin virt-tests-vm1 0 0,3 [root@yds-pc ~]# virsh vcpuinfo virt-tests-vm1 VCPU: 0 CPU: 0 State: running CPU time: 14.8s CPU Affinity: y--y *There is no diff between cpu1 and cpu2 (both '-')* BUT: [root@yds-pc ~]# virsh vcpupin virt-tests-vm1 0 1 [root@yds-pc ~]# echo $? 0 [root@yds-pc ~]# virsh vcpupin virt-tests-vm1 0 2 error: Requested operation is not valid: failed to set cpuset.cpus in cgroup for vcpu 0 [root@yds-pc ~]# echo $? 1 So, I think we should show the cpu in different state with different symbol. In this patchset, I introduce a symbol 'x' (maybe there is a better one :)) to display the cpu is offline on host. Just like: [root@yds-pc ~]# virsh vcpuinfo virt-tests-vm1 VCPU: 0 CPU: 0 State: running CPU time: 14.8s CPU Affinity: y-xy yangdongsheng (2): tool/virsh-domain.c: Add a vshNodeGetCPUMap function to get cpumap of host. tool/virsh-domain.c: Fix the display of Affinity in function cmdVcpuinfo. tools/virsh-domain.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 3 deletions(-) -- 1.7.10.1
Get CPU map of host node CPUs by trying virNodeGetCPUMap and falling back to virNodeGetCPUStats. Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- tools/virsh-domain.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index bc42408..ebaca2d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -149,6 +149,61 @@ vshNodeGetCPUCount(virConnectPtr conn) } /* + * Get CPU map of host node CPUs by trying virNodeGetCPUMap + * and falling back to virNodeGetCPUStats. + */ +static int +vshNodeGetCPUMap(virConnectPtr conn, + unsigned char **cpumap) +{ + int ret = -1; + int cpuNum = VIR_NODE_CPU_STATS_ALL_CPUS; + int nparams = 0; + virNodeCPUStatsPtr params; + int i = 0; + int dummy; + virBitmapPtr cpus = NULL; + + if ((ret = virNodeGetCPUMap(conn, cpumap, NULL, 0)) < 0 ) { + /* fall back to virNodeCpuStats */ + vshResetLibvirtError(); + if (virNodeGetCPUStats(conn, cpuNum, NULL, &nparams, 0) != 0) { + goto cleanup; + } + + if ((params = vshCalloc(NULL, nparams, sizeof(*params))) == NULL) + goto cleanup; + + if ((cpus = virBitmapNew(nparams)) == NULL){ + goto cleanup; + } + + for (i=0; i < nparams; i++) { + if ((ret = virNodeGetCPUStats(conn, i, params, + &nparams, 0)) != 0) { + if (virBitmapClearBit(cpus, i) < 0) + goto cleanup; + } + else { + if (virBitmapSetBit(cpus, i) < 0) + goto cleanup; + } + } + if (cpumap && virBitmapToData(cpus, cpumap, &dummy) < 0){ + goto cleanup; + } + ret = virBitmapCountBits(cpus); + } +cleanup: + if (ret < 0 && params) + VIR_FREE(params); + if (ret < 0 && cpumap) + VIR_FREE(*cpumap); + virBitmapFree(cpus); + return ret; +} + +/* * "attach-device" command */ static const vshCmdInfo info_attach_device[] = { -- 1.7.10.1
On 05/24/13 11:21, yangdongsheng wrote:
Get CPU map of host node CPUs by trying virNodeGetCPUMap and falling back to virNodeGetCPUStats.
Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- tools/virsh-domain.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
NACK. This functionality is already implemented as "nodecpumap" command defined in cmdNodeCpuMap in virsh-host.c If you wish to add the fallback code to that function, the patch is welcome. Peter
On 05/24/2013 08:18 PM, Peter Krempa wrote:
On 05/24/13 11:21, yangdongsheng wrote:
Get CPU map of host node CPUs by trying virNodeGetCPUMap and falling back to virNodeGetCPUStats.
Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- tools/virsh-domain.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
NACK. This functionality is already implemented as "nodecpumap" command defined in cmdNodeCpuMap in virsh-host.c
If you wish to add the fallback code to that function, the patch is welcome.
Peter
Aha, I see it. But now, I think the fallback is not nessary. We can use virNodeGetCPUMap directly. Thanx for your reply. :)
(1).Introduce a symbol 'x' to mean the physical cpu on host is offline. (2).And the symbol '-' means the physical cpu on host is online but the affinity of domain for this cpu is not set. There was no diffrence in display between the two kinds of cpu state before this patch, both are '-'. Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- tools/virsh-domain.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ebaca2d..3a8da00 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5325,6 +5325,7 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; virVcpuInfoPtr cpuinfo; unsigned char *cpumaps; + unsigned char *hostcpumap; int ncpus, maxcpu; size_t cpumaplen; bool ret = true; @@ -5338,6 +5339,11 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) return false; } + if ((vshNodeGetCPUMap(ctl->conn, &hostcpumap)) < 0) { + virDomainFree(dom); + return false; + } + if (virDomainGetInfo(dom, &info) != 0) { virDomainFree(dom); return false; @@ -5364,7 +5370,13 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) } vshPrint(ctl, "%-15s ", _("CPU Affinity:")); for (m = 0; m < maxcpu; m++) { - vshPrint(ctl, "%c", VIR_CPU_USABLE(cpumaps, cpumaplen, n, m) ? 'y' : '-'); + if VIR_CPU_USED(hostcpumap, m) { + vshPrint(ctl, "%c", VIR_CPU_USABLE(cpumaps, + cpumaplen, n, m) ? 'y' : '-'); + } + else { + vshPrint(ctl, "%c", 'x'); + } } vshPrint(ctl, "\n"); if (n < (ncpus - 1)) { @@ -5386,8 +5398,13 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A")); vshPrint(ctl, "%-15s ", _("CPU Affinity:")); for (m = 0; m < maxcpu; m++) { - vshPrint(ctl, "%c", - VIR_CPU_USABLE(cpumaps, cpumaplen, n, m) ? 'y' : '-'); + if VIR_CPU_USED(hostcpumap, m) { + vshPrint(ctl, "%c", VIR_CPU_USABLE(cpumaps, + cpumaplen, n, m) ? 'y' : '-'); + } + else { + vshPrint(ctl, "%c", 'x'); + } } vshPrint(ctl, "\n"); if (n < (ncpus - 1)) { -- 1.7.10.1
On 05/24/2013 03:21 AM, yangdongsheng wrote:
Hi,
[uggh; yet another victim of git's stupidity in using "In-reply-to: <y>" if you answer the 'git send-email' question incorrectly - just hit enter rather than filling in a response to that question] Would you mind resending this as a top-level thread, so that it gets properly threaded? My mail client split 0/2 and 2/2 into an old series, and 1/2 somewhere else.
I found some confusion in display of commad *virsh vcpuinfo domname* . There is no diffrence between a cpu offline and a cpu online but not be set for the affinity of vcpu, both are '-'. But users will find the difference of them in using them for a same action (such as vcpupin) and be confused.
I'm a bit worried about backward compatibility; if there are existing scripts that parse the output, will they be confused if we introduce a third symbol? Then again, offline host cpus are indeed a different beast than cpus that the guest has been assigned to, so I think we can make this change.
So, I think we should show the cpu in different state with different symbol.
In this patchset, I introduce a symbol 'x' (maybe there is a better one :)) to display the cpu is offline on host.
Just like: [root@yds-pc ~]# virsh vcpuinfo virt-tests-vm1 VCPU: 0 CPU: 0 State: running CPU time: 14.8s CPU Affinity: y-xy
yangdongsheng (2): tool/virsh-domain.c: Add a vshNodeGetCPUMap function to get cpumap of host. tool/virsh-domain.c: Fix the display of Affinity in function cmdVcpuinfo.
tools/virsh-domain.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 3 deletions(-)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/24/2013 10:03 PM, Eric Blake wrote:
On 05/24/2013 03:21 AM, yangdongsheng wrote:
Hi, [uggh; yet another victim of git's stupidity in using "In-reply-to:<y>" if you answer the 'git send-email' question incorrectly - just hit enter rather than filling in a response to that question]
Would you mind resending this as a top-level thread, so that it gets properly threaded? My mail client split 0/2 and 2/2 into an old series, and 1/2 somewhere else. :( Sorry to trouble you ! Of course I will do some fix for this patch and resend it.
I found some confusion in display of commad *virsh vcpuinfo domname* . There is no diffrence between a cpu offline and a cpu online but not be set for the affinity of vcpu, both are '-'. But users will find the difference of them in using them for a same action (such as vcpupin) and be confused.
I'm a bit worried about backward compatibility; if there are existing scripts that parse the output, will they be confused if we introduce a third symbol? Then again, offline host cpus are indeed a different beast than cpus that the guest has been assigned to, so I think we can make this change. Thanx for your approvement.
So, I think we should show the cpu in different state with different symbol.
In this patchset, I introduce a symbol 'x' (maybe there is a better one :)) to display the cpu is offline on host.
Just like: [root@yds-pc ~]# virsh vcpuinfo virt-tests-vm1 VCPU: 0 CPU: 0 State: running CPU time: 14.8s CPU Affinity: y-xy
yangdongsheng (2): tool/virsh-domain.c: Add a vshNodeGetCPUMap function to get cpumap of host. tool/virsh-domain.c: Fix the display of Affinity in function cmdVcpuinfo.
tools/virsh-domain.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 3 deletions(-)
participants (4)
-
Eric Blake -
Peter Krempa -
Yang Dongsheng -
yangdongsheng