On 06/24/2011 03:02 AM, Taku Izumi wrote:
>
> This patch adds --query option to "virsh vcpupin" command.
> Its feature is to show CPU affnity information in more
s/affnity/affinity/
> reader-friendly way.
>
> # virsh vcpupin VM --query --config
> VCPU: CPU Affinity
> ----------------------------------
> 0: 1-6,9-20
> 1: 10
> 2: 5,9-11,15-20
> 3: 1,3,5,7,9,11,13,15
While it would be really cool to see strings like 0-15,^8, I don't think
it's worth the complexity of trying to figure that out. Your
representation looks nice enough, even if we parse more strings than we
generate.
>
> When --query is specified, cpulist is not required and vcpu number
> is optional. When vcpu number is provided, information of only specified
> vcpu is displayed.
But why do we need --query? If cpulist and --query are orthogonal, why
not just make the absence of a cpulist imply query, without having it be
an explicit option?
>
> Signed-off-by: Taku Izumi <izumi.taku@xxxxxxxxxxxxxx>
> ---
> tools/virsh.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++--------
> tools/virsh.pod | 11 +++++-
> 2 files changed, 93 insertions(+), 15 deletions(-)
>
> Index: libvirt/tools/virsh.c
> ===================================================================
> --- libvirt.orig/tools/virsh.c
> +++ libvirt/tools/virsh.c
> @@ -2992,15 +2992,16 @@ cmdVcpuinfo(vshControl *ctl, const vshCm
> * "vcpupin" command
> */
> static const vshCmdInfo info_vcpupin[] = {
> - {"help", N_("control domain vcpu affinity")},
> + {"help", N_("control or query domain vcpu affinity")},
> {"desc", N_("Pin domain VCPUs to host physical CPUs.")},
> {NULL, NULL}
> };
>
> static const vshCmdOptDef opts_vcpupin[] = {
> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> - {"vcpu", VSH_OT_INT, VSH_OFLAG_REQ, N_("vcpu number")},
> - {"cpulist", VSH_OT_DATA, VSH_OFLAG_REQ, N_("host cpu number(s) (comma separated)")},
> + {"vcpu", VSH_OT_INT, 0, N_("vcpu number")},
> + {"cpulist", VSH_OT_DATA, VSH_OFLAG_EMPTY_OK, N_("host cpu number(s)")},
Why is empty okay? That implies that:
virsh vcpupin dom 0 ''
is valid. Is that shorthand for undoing affinity (basically, restoring
that vcpu to use all possible host cpus)? Makes sense...
</me reads ahead...>
Indeed that's what the code does, but it means a tweak to virsh.pod.
> + {"query", VSH_OT_BOOL, 0, N_("query CPU affinitiy information")},
s/affinitiy/affinity/
> @@ -3049,14 +3054,22 @@ cmdVcpupin(vshControl *ctl, const vshCmd
> return false;
>
> if (vshCommandOptInt(cmd, "vcpu", &vcpu) <= 0) {
> - vshError(ctl, "%s", _("vcpupin: Invalid or missing vCPU number."));
> - virDomainFree(dom);
> - return false;
> + /* When query mode, "vcpu" is optional */
> + if (!query) {
Not quite the right logic. vshCommandOptInt() < 0 is always an error,
and vshCommandOptInt()==0 is an error if !query.
> + vshError(ctl, "%s",
> + _("vcpupin: Invalid or missing vCPU number."));
> + virDomainFree(dom);
> + return false;
> + }
> }
>
> if (vshCommandOptString(cmd, "cpulist", &cpulist) <= 0) {
> - virDomainFree(dom);
> - return false;
> + /* When query mode, "cpulist" is optional */
> + if (!query) {
Also not the right logic - --query and --cpulist are mutually exclusive.
> @@ -3078,8 +3091,65 @@ cmdVcpupin(vshControl *ctl, const vshCmd
>
> maxcpu = VIR_NODEINFO_MAXCPUS(nodeinfo);
> cpumaplen = VIR_CPU_MAPLEN(maxcpu);
> - cpumap = vshCalloc(ctl, 0, cpumaplen);
>
> + /* Query mode: show CPU affinity information then exit.*/
> + if (query) {
> + /* When query mode and neither "live", "config" nor "curent" is specified,
s/curent/current/, and long line
> + vshPrint(ctl, "%4d: ", i);
> + for (cpu = 0; cpu < maxcpu; cpu++) {
> +
> + if (VIR_CPU_USABLE(cpumaps, cpumaplen, i, cpu))
Indentation.
> + bit = 1;
> + else
> + bit = 0;
Can be made shorter by making 'bit' a bool.
> +
> + isInvert = (bit ^ lastbit) ? true : false;
Likewise.
> cleanup:
> - VIR_FREE(cpumap);
> + if (cpumap)
> + VIR_FREE(cpumap);
'make syntax-check' calls you on this one. This change is bogus, since
VIR_FREE is a safe no-op on NULL.
> -Pin domain VCPUs to host physical CPUs. The I<vcpu> number must be provided
> -and I<cpulist> is a list of physical CPU numbers. Its syntax is a comma
> +=item B<vcpupin> I<domain-id> I<--query> optional I<vcpu> I<--live> I<--config>
> +I<--current>
No need for a dual synopsis form once we get rid of --query.
> +
> +Pin domain VCPUs to host physical CPUs, or query CPU affinity information
> +(specify I<--query>). When pinning vCPU, the I<vcpu> number and I<cpulist> must
> +be provided. When querrying affinity information, I<cpulist> is not required
s/querrying/querying/
> +and I<vcpu> is optional.
> +
> +I<cpulist> is a list of physical CPU numbers. Its syntax is a comma
> separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can
> also be allowed. The '-' denotes the range and the '^' denotes exclusive.
> If you want to reset vcpupin setting, that is, to pin vcpu all physical cpus,
ACK with this squashed in, so I've pushed the series.
diff --git i/tools/virsh.c w/tools/virsh.c
index 508d97d..31fbeb7 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -3006,8 +3006,8 @@ static const vshCmdInfo info_vcpupin[] = {
static const vshCmdOptDef opts_vcpupin[] = {
{"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
{"vcpu", VSH_OT_INT, 0, N_("vcpu number")},
- {"cpulist", VSH_OT_DATA, VSH_OFLAG_EMPTY_OK, N_("host cpu number(s)")},
- {"query", VSH_OT_BOOL, 0, N_("query CPU affinitiy information")},
+ {"cpulist", VSH_OT_DATA, VSH_OFLAG_EMPTY_OK,
+ N_("host cpu number(s) to set, or omit option to query")},
{"config", VSH_OT_BOOL, 0, N_("affect next boot")},
{"live", VSH_OT_BOOL, 0, N_("affect running domain")},
{"current", VSH_OT_BOOL, 0, N_("affect current domain")},
@@ -3026,15 +3026,14 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
unsigned char *cpumap = NULL;
unsigned char *cpumaps = NULL;
int cpumaplen;
- int bit, lastbit;
- bool isInvert;
+ bool bit, lastbit, isInvert;
int i, cpu, lastcpu, maxcpu, ncpus;
bool unuse = false;
const char *cur;
- int query = vshCommandOptBool(cmd, "query");
int config = vshCommandOptBool(cmd, "config");
int live = vshCommandOptBool(cmd, "live");
int current = vshCommandOptBool(cmd, "current");
+ bool query = false; /* Query mode if no cpulist */
int flags = 0;
if (current) {
@@ -3059,23 +3058,19 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
return false;
- if (vshCommandOptInt(cmd, "vcpu", &vcpu) <= 0) {
- /* When query mode, "vcpu" is optional */
- if (!query) {
- vshError(ctl, "%s",
- _("vcpupin: Invalid or missing vCPU number."));
- virDomainFree(dom);
- return false;
- }
+ if (vshCommandOptString(cmd, "cpulist", &cpulist) < 0) {
+ vshError(ctl, "%s", _("vcpupin: Missing cpulist."));
+ virDomainFree(dom);
+ return false;
}
+ query = !cpulist;
- if (vshCommandOptString(cmd, "cpulist", &cpulist) <= 0) {
- /* When query mode, "cpulist" is optional */
- if (!query) {
- vshError(ctl, "%s", _("vcpupin: Missing cpulist."));
- virDomainFree(dom);
- return false;
- }
+ /* In query mode, "vcpu" is optional */
+ if (vshCommandOptInt(cmd, "vcpu", &vcpu) < !query) {
+ vshError(ctl, "%s",
+ _("vcpupin: Invalid or missing vCPU number."));
+ virDomainFree(dom);
+ return false;
}
if (virNodeGetInfo(ctl->conn, &nodeinfo) != 0) {
@@ -3084,7 +3079,7 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
}
if (virDomainGetInfo(dom, &info) != 0) {
- vshError(ctl, "%s", _("vcpupin: failed to get domain
informations."));
+ vshError(ctl, "%s", _("vcpupin: failed to get domain
information."));
virDomainFree(dom);
return false;
}
@@ -3100,8 +3095,8 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
/* Query mode: show CPU affinity information then exit.*/
if (query) {
- /* When query mode and neither "live", "config" nor "curent" is
specified,
- * set VIR_DOMAIN_AFFECT_CURRENT as flags */
+ /* When query mode and neither "live", "config" nor "current"
+ * is specified, set VIR_DOMAIN_AFFECT_CURRENT as flags */
if (flags == -1)
flags = VIR_DOMAIN_AFFECT_CURRENT;
@@ -3116,29 +3111,25 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
if (vcpu != -1 && i != vcpu)
continue;
- bit = lastbit = 0;
- isInvert = false;
+ bit = lastbit = isInvert = false;
lastcpu = -1;
vshPrint(ctl, "%4d: ", i);
for (cpu = 0; cpu < maxcpu; cpu++) {
- if (VIR_CPU_USABLE(cpumaps, cpumaplen, i, cpu))
- bit = 1;
- else
- bit = 0;
-
- isInvert = (bit ^ lastbit) ? true : false;
- if (bit && isInvert) {
- if (lastcpu == -1)
- vshPrint(ctl, "%d", cpu);
- else
- vshPrint(ctl, ",%d", cpu);
- lastcpu = cpu;
- }
- if (!bit && isInvert && lastcpu != cpu - 1)
- vshPrint(ctl, "-%d", cpu - 1);
- lastbit = bit;
+ bit = VIR_CPU_USABLE(cpumaps, cpumaplen, i, cpu);
+
+ isInvert = (bit ^ lastbit);
+ if (bit && isInvert) {
+ if (lastcpu == -1)
+ vshPrint(ctl, "%d", cpu);
+ else
+ vshPrint(ctl, ",%d", cpu);
+ lastcpu = cpu;
+ }
+ if (!bit && isInvert && lastcpu != cpu - 1)
+ vshPrint(ctl, "-%d", cpu - 1);
+ lastbit = bit;
}
if (bit && !isInvert) {
vshPrint(ctl, "-%d", maxcpu - 1);
@@ -3237,8 +3228,7 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd)
}
cleanup:
- if (cpumap)
- VIR_FREE(cpumap);
+ VIR_FREE(cpumap);
virDomainFree(dom);
return ret;
diff --git i/tools/virsh.pod w/tools/virsh.pod
index c72a960..fc06075 100644
--- i/tools/virsh.pod
+++ w/tools/virsh.pod
@@ -827,16 +827,12 @@ values; these two flags cannot both be specified.
Returns basic information about the domain virtual CPUs, like the number of
vCPUs, the running time, the affinity to physical processors.
-=item B<vcpupin> I<domain-id> I<vcpu> I<cpulist> optional I<--live>
I<--config>
+=item B<vcpupin> I<domain-id> optional I<vcpu> I<cpulist> I<--live>
I<--config>
I<--current>
-=item B<vcpupin> I<domain-id> I<--query> optional I<vcpu> I<--live>
I<--config>
-I<--current>
-
-Pin domain VCPUs to host physical CPUs, or query CPU affinity information
-(specify I<--query>). When pinning vCPU, the I<vcpu> number and
I<cpulist> must
-be provided. When querrying affinity information, I<cpulist> is not
required
-and I<vcpu> is optional.
+Query or change the pinning of domain VCPUs to host physical CPUs. To
+pin a single I<vcpu>, specify I<cpulist>; otherwise, you can query one
+I<vcpu> or omit I<vcpu> to list all at once.
I<cpulist> is a list of physical CPU numbers. Its syntax is a comma
separated list and a special markup using '-' and '^' (ex. '0-4',
'0-3,^2') can
@@ -846,11 +842,12 @@ simply specify 'r' as a cpulist.
If I<--live> is specified, affect a running guest.
If I<--config> is specified, affect the next boot of a persistent guest.
If I<--current> is specified, affect the current guest state.
-Both I<--live> and I<--config> flags may be given, but I<--current> is
exclusive.
+Both I<--live> and I<--config> flags may be given if I<cpulist> is present,
+but I<--current> is exclusive.
If no flag is specified, behavior is different depending on hypervisor.
-B<Note>: The expression is sequentially evaluated, so "0-15,^8" is not
identical
-to "^8,0-15".
+B<Note>: The expression is sequentially evaluated, so "0-15,^8" is
+identical to "9-14,0-7,15" but not identical to "^8,0-15".
=item B<vncdisplay> I<domain-id>
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list