[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/5] vcpupin: add query option to virsh vcpupin command



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 jp fujitsu com>
> ---
>  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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]