Re: [PATCH 10 of 11 v4] xl: add node-affinity to the output of `xl list`

[George Dunlap <george.dunlap@eu.citrix.com>]

On 15/03/13 02:30, Dario Faggioli wrote:
> Node-affinity is now something that is under (some) control of the
> user, so show it upon request as part of the output of `xl list'
> by the `-n' option.
>
> Re the patch, the print_bitmap() related hunk is _mostly_ code motion,
> although there is a very minor change in the code, basically to allow
> using the function for printing both cpu and node bitmaps (as, in case
> all bits are sets, it used to print "any cpu", which doesn't fit the
> nodemap case).
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Acked-by: Juergen Gross <juergen.gross@ts.fujitsu.com>

Can we get a tools maintainer comment (either ack / recommendation for 
improvement) sometime in the next few days, so that the next time Dario 
submits this series it can be taken wholesale?

  -George

> ---
> Changes from v3:
>   * removed the pastebin.com link from the changelog. Will reply to this
>     same mail with another one with the output of various invocations
>     of `xl list' with different options to show how it looks like.
>
> Changes from v1:
>   * print_{cpu,node}map() functions added instead of 'state variable'-izing
>     print_bitmap().
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3030,14 +3030,95 @@ out:
>       }
>   }
>   
> -static void list_domains(int verbose, int context, const libxl_dominfo *info, int nb_domain)
> +/* If map is not full, prints it and returns 0. Returns 1 otherwise. */
> +static int print_bitmap(uint8_t *map, int maplen, FILE *stream)
> +{
> +    int i;
> +    uint8_t pmap = 0, bitmask = 0;
> +    int firstset = 0, state = 0;
> +
> +    for (i = 0; i < maplen; i++) {
> +        if (i % 8 == 0) {
> +            pmap = *map++;
> +            bitmask = 1;
> +        } else bitmask <<= 1;
> +
> +        switch (state) {
> +        case 0:
> +        case 2:
> +            if ((pmap & bitmask) != 0) {
> +                firstset = i;
> +                state++;
> +            }
> +            continue;
> +        case 1:
> +        case 3:
> +            if ((pmap & bitmask) == 0) {
> +                fprintf(stream, "%s%d", state > 1 ? "," : "", firstset);
> +                if (i - 1 > firstset)
> +                    fprintf(stream, "-%d", i - 1);
> +                state = 2;
> +            }
> +            continue;
> +        }
> +    }
> +    switch (state) {
> +        case 0:
> +            fprintf(stream, "none");
> +            break;
> +        case 2:
> +            break;
> +        case 1:
> +            if (firstset == 0)
> +                return 1;
> +        case 3:
> +            fprintf(stream, "%s%d", state > 1 ? "," : "", firstset);
> +            if (i - 1 > firstset)
> +                fprintf(stream, "-%d", i - 1);
> +            break;
> +    }
> +
> +    return 0;
> +}
> +
> +static void print_cpumap(uint8_t *map, int maplen, FILE *stream)
> +{
> +    if (print_bitmap(map, maplen, stream))
> +        fprintf(stream, "any cpu");
> +}
> +
> +static void print_nodemap(uint8_t *map, int maplen, FILE *stream)
> +{
> +    if (print_bitmap(map, maplen, stream))
> +        fprintf(stream, "any node");
> +}
> +
> +static void list_domains(int verbose, int context, int numa, const libxl_dominfo *info, int nb_domain)
>   {
>       int i;
>       static const char shutdown_reason_letters[]= "-rscw";
> +    libxl_bitmap nodemap;
> +    libxl_physinfo physinfo;
> +
> +    libxl_bitmap_init(&nodemap);
> +    libxl_physinfo_init(&physinfo);
>   
>       printf("Name                                        ID   Mem VCPUs\tState\tTime(s)");
>       if (verbose) printf("   UUID                            Reason-Code\tSecurity Label");
>       if (context && !verbose) printf("   Security Label");
> +    if (numa) {
> +        if (libxl_node_bitmap_alloc(ctx, &nodemap, 0)) {
> +            fprintf(stderr, "libxl_node_bitmap_alloc_failed.\n");
> +            exit(1);
> +        }
> +        if (libxl_get_physinfo(ctx, &physinfo) != 0) {
> +            fprintf(stderr, "libxl_physinfo failed.\n");
> +            libxl_bitmap_dispose(&nodemap);
> +            exit(1);
> +        }
> +
> +        printf(" NODE Affinity");
> +    }
>       printf("\n");
>       for (i = 0; i < nb_domain; i++) {
>           char *domname;
> @@ -3071,14 +3152,23 @@ static void list_domains(int verbose, in
>               rc = libxl_flask_sid_to_context(ctx, info[i].ssidref, &buf,
>                                               &size);
>               if (rc < 0)
> -                printf("  -");
> +                printf("                -");
>               else {
> -                printf("  %s", buf);
> +                printf(" %16s", buf);
>                   free(buf);
>               }
>           }
> +        if (numa) {
> +            libxl_domain_get_nodeaffinity(ctx, info[i].domid, &nodemap);
> +
> +            putchar(' ');
> +            print_nodemap(nodemap.map, physinfo.nr_nodes, stdout);
> +        }
>           putchar('\n');
>       }
> +
> +    libxl_bitmap_dispose(&nodemap);
> +    libxl_physinfo_dispose(&physinfo);
>   }
>   
>   static void list_vm(void)
> @@ -3945,10 +4035,12 @@ int main_list(int argc, char **argv)
>       int opt, verbose = 0;
>       int context = 0;
>       int details = 0;
> +    int numa = 0;
>       static struct option opts[] = {
>           {"long", 0, 0, 'l'},
>           {"verbose", 0, 0, 'v'},
>           {"context", 0, 0, 'Z'},
> +        {"numa", 0, 0, 'n'},
>           COMMON_LONG_OPTS,
>           {0, 0, 0, 0}
>       };
> @@ -3957,7 +4049,7 @@ int main_list(int argc, char **argv)
>       libxl_dominfo *info, *info_free=0;
>       int nb_domain, rc;
>   
> -    SWITCH_FOREACH_OPT(opt, "lvhZ", opts, "list", 0) {
> +    SWITCH_FOREACH_OPT(opt, "lvhZn", opts, "list", 0) {
>       case 'l':
>           details = 1;
>           break;
> @@ -3967,6 +4059,9 @@ int main_list(int argc, char **argv)
>       case 'Z':
>           context = 1;
>           break;
> +    case 'n':
> +        numa = 1;
> +        break;
>       }
>   
>       if (optind >= argc) {
> @@ -3998,7 +4093,7 @@ int main_list(int argc, char **argv)
>       if (details)
>           list_domains_details(info, nb_domain);
>       else
> -        list_domains(verbose, context, info, nb_domain);
> +        list_domains(verbose, context, numa, info, nb_domain);
>   
>       if (info_free)
>           libxl_dominfo_list_free(info, nb_domain);
> @@ -4247,56 +4342,6 @@ int main_button_press(int argc, char **a
>       return 0;
>   }
>   
> -static void print_bitmap(uint8_t *map, int maplen, FILE *stream)
> -{
> -    int i;
> -    uint8_t pmap = 0, bitmask = 0;
> -    int firstset = 0, state = 0;
> -
> -    for (i = 0; i < maplen; i++) {
> -        if (i % 8 == 0) {
> -            pmap = *map++;
> -            bitmask = 1;
> -        } else bitmask <<= 1;
> -
> -        switch (state) {
> -        case 0:
> -        case 2:
> -            if ((pmap & bitmask) != 0) {
> -                firstset = i;
> -                state++;
> -            }
> -            continue;
> -        case 1:
> -        case 3:
> -            if ((pmap & bitmask) == 0) {
> -                fprintf(stream, "%s%d", state > 1 ? "," : "", firstset);
> -                if (i - 1 > firstset)
> -                    fprintf(stream, "-%d", i - 1);
> -                state = 2;
> -            }
> -            continue;
> -        }
> -    }
> -    switch (state) {
> -        case 0:
> -            fprintf(stream, "none");
> -            break;
> -        case 2:
> -            break;
> -        case 1:
> -            if (firstset == 0) {
> -                fprintf(stream, "any cpu");
> -                break;
> -            }
> -        case 3:
> -            fprintf(stream, "%s%d", state > 1 ? "," : "", firstset);
> -            if (i - 1 > firstset)
> -                fprintf(stream, "-%d", i - 1);
> -            break;
> -    }
> -}
> -
>   static void print_vcpuinfo(uint32_t tdomid,
>                              const libxl_vcpuinfo *vcpuinfo,
>                              uint32_t nr_cpus)
> @@ -4320,7 +4365,7 @@ static void print_vcpuinfo(uint32_t tdom
>       /*      TIM */
>       printf("%9.1f  ", ((float)vcpuinfo->vcpu_time / 1e9));
>       /* CPU AFFINITY */
> -    print_bitmap(vcpuinfo->cpumap.map, nr_cpus, stdout);
> +    print_cpumap(vcpuinfo->cpumap.map, nr_cpus, stdout);
>       printf("\n");
>   }
>   
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -50,7 +50,8 @@ struct cmd_spec cmd_table[] = {
>         "[options] [Domain]\n",
>         "-l, --long              Output all VM details\n"
>         "-v, --verbose           Prints out UUIDs and security context\n"
> -      "-Z, --context           Prints out security context"
> +      "-Z, --context           Prints out security context\n"
> +      "-n, --numa              Prints out NUMA node affinity"
>       },
>       { "destroy",
>         &main_destroy, 0, 1,