From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 09 of 10 v2] xl: add node-affinity to the output of `xl list` Date: Fri, 21 Dec 2012 16:34:05 +0000 Message-ID: <50D48F7D.5010001@eu.citrix.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli Cc: Marcus Granado , Dan Magenheimer , Ian Campbell , Anil Madhavapeddy , Andrew Cooper , Juergen Gross , Ian Jackson , "xen-devel@lists.xen.org" , Jan Beulich , Daniel De Graaf , Matt Wilson List-Id: xen-devel@lists.xenproject.org On 19/12/12 19:07, 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. > > Signed-off-by: Dario Faggioli > Acked-by: Juergen Gross > --- > 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 > @@ -2961,14 +2961,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; > +} Just checking -- is the print_bitmap() thing pure code motion? If so, would you mind saying that explicitly in the commit message, just to save people time when reading this patch? Other than that, looks OK to me -- I haven't done a detailed review of the output layout however. -George > + > +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; > @@ -3002,14 +3083,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) > @@ -3890,12 +3980,14 @@ int main_list(int argc, char **argv) > int opt, verbose = 0; > int context = 0; > int details = 0; > + int numa = 0; > int option_index = 0; > static struct option long_options[] = { > {"long", 0, 0, 'l'}, > {"help", 0, 0, 'h'}, > {"verbose", 0, 0, 'v'}, > {"context", 0, 0, 'Z'}, > + {"numa", 0, 0, 'n'}, > {0, 0, 0, 0} > }; > > @@ -3904,7 +3996,7 @@ int main_list(int argc, char **argv) > int nb_domain, rc; > > while (1) { > - opt = getopt_long(argc, argv, "lvhZ", long_options, &option_index); > + opt = getopt_long(argc, argv, "lvhZn", long_options, &option_index); > if (opt == -1) > break; > > @@ -3921,6 +4013,9 @@ int main_list(int argc, char **argv) > case 'Z': > context = 1; > break; > + case 'n': > + numa = 1; > + break; > default: > fprintf(stderr, "option `%c' not supported.\n", optopt); > break; > @@ -3956,7 +4051,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); > @@ -4228,56 +4323,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) > @@ -4301,7 +4346,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,