[Qemu-devel] Re: [PATCH 1/3] NUMA: add -numa command line option

[Anthony Liguori <anthony@codemonkey.ws>]

Andre Przywara wrote:
> This adds and parses a -numa command line option to QEMU.
>
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>
> # HG changeset patch
> # User Andre Przywara <andre.przywara@amd.com>
> # Date 1228991781 -3600
> # Node ID 394d02758aa4358be3bcd14f9d59efaf42e89328
> # Parent  b3a4224604d267a0e406a6d6809947f342a6b2ed
> introduce -numa command line option
>
> diff -r b3a4224604d2 -r 394d02758aa4 sysemu.h
> --- a/sysemu.h	Thu Dec 11 11:22:27 2008 +0100
> +++ b/sysemu.h	Thu Dec 11 11:36:21 2008 +0100
> @@ -92,6 +92,16 @@ extern int alt_grab;
>  extern int alt_grab;
>  extern int usb_enabled;
>  extern int smp_cpus;
> +
> +#define MAX_NODES 64
> +extern int numnumanodes;
> +extern uint64_t hostnodes[MAX_NODES];
>   

This should not be in this patch.  This patch should just contain the 
bits needed to create a virtual guest NUMA topology.  The hostnode stuff 
should be a separate patch.

> +extern uint64_t node_mem[MAX_NODES];
> +extern uint64_t node_to_cpus[MAX_NODES];
> +
> +int parse_numa_args (const char *opt, uint64_t *hostnodes, uint64_t *mems,
> +                     uint64_t *cpus, int maxentries, int expect_numnodes);
> +
>  extern int cursor_hide;
>  extern int graphic_rotate;
>  extern int no_quit;
> diff -r b3a4224604d2 -r 394d02758aa4 vl.c
> --- a/vl.c	Thu Dec 11 11:22:27 2008 +0100
> +++ b/vl.c	Thu Dec 11 11:36:21 2008 +0100
> @@ -222,6 +222,10 @@ int win2k_install_hack = 0;
>  #endif
>  int usb_enabled = 0;
>  int smp_cpus = 1;
> +int numnumanodes = 0;
> +uint64_t hostnodes[MAX_NODES];
> +uint64_t node_mem[MAX_NODES];
> +uint64_t node_to_cpus[MAX_NODES];
>  const char *vnc_display;
>  int acpi_enabled = 1;
>  int fd_bootchk = 1;
> @@ -3968,6 +3972,10 @@ static void help(int exitcode)
>  	   "-daemonize      daemonize QEMU after initializing\n"
>  #endif
>  	   "-option-rom rom load a file, rom, into the option ROM space\n"
> +           "-numa nrnodes[,mem:size1[;size2..]][,cpu:cpu1[;cpu2..]][,pin:node1[;node2]]\n"
> +           "                create a multi NUMA node guest and optionally pin it to\n"
> +           "                to the given host nodes. If mem and cpu are omitted,\n"
> +           "                resources are split equally\n"
>   

You're whitespace damaged.

> +
> +#define PARSE_FLAG_BITMASK   1
> +#define PARSE_FLAG_SUFFIX    2
> +
> +static int parse_to_array (const char *arg, uint64_t *array,
> +    char delim, int maxentries, int flags)
> +{
> +const char *s;
> +char *end;
> +int num = 0;
> +unsigned long long int val,endval;
>   

You're really whitespace damaged.

> +    for (s = arg; s != NULL && *s != 0; s++) {
> +        val = strtoull (s, &end, 10);
> +        if (end == s && *s != '*') {num++; continue;}
> +        if (num >= maxentries) break;
> +        switch (*end) {
> +            case 'g':
> +            case 'G':
> +                if (flags & PARSE_FLAG_SUFFIX) val *= 1024;
> +           	/* fall through */
> +            case 'm':
> +            case 'M':
> +                if (flags & PARSE_FLAG_SUFFIX) val *= 1024;
> +           	/* fall through */
> +            case 'k':
> +            case 'K':
> +           	    if (flags & PARSE_FLAG_SUFFIX) val *= 1024;
> +           	    break;
> +            case '*':
> +                val = (unsigned long long int)-1;
> +                break;
> +            case '-':
> +           	    if (!(flags & PARSE_FLAG_BITMASK)) break;
> +                s = end + 1;
> +                endval = strtoull (s, &end, 10);
> +           	    val = (1 << (endval + 1)) - (1 << val);
> +                break;
> +            case 0:
> +           	    if (flags & PARSE_FLAG_SUFFIX) val *= 1024 * 1024;
> +           	/* fall through */
> +            default:
> +           	    if (flags & PARSE_FLAG_BITMASK) val = 1 << val;
> +           	    break;
> +        }
> +        array[num++] = val;
> +        if ((s = strchr (end, delim)) == NULL) break;
> +    }
> +    return num;
> +}
>
>   

This syntax is sufficiently complex that it needs thorough documentation 
(in qemu-doc.texi).

> +int parse_numa_args (const char *opt, uint64_t *hostnodes, uint64_t *mems,
> +                     uint64_t *cpus, int maxentries, int expect_numnodes)
> +{
> +const char *s;
> +char *arg, *val, *end, *strtokarg;
> +int num = 0;
> +
> +    arg = strdup(opt); strtokarg = arg;
> +    if (expect_numnodes) {
> +        s = strtok(strtokarg, ",");
>   

strtok is generally evil.  strsep() is a better choice.

> +        if (s == NULL) {free (arg); return -1;}
>   

Don't do this one on line.

> +        num = strtol (s, &end, 10);
> +        if (s == end) {free (arg); return -1;}
> +        strtokarg = NULL;
> +    }
> +    while ((s=strtok(strtokarg, ","))!=NULL) {
> +        strtokarg = NULL;
> +        if ((val = strchr (s, ':'))) {
> +            *val++ = 0;
> +            if (!strcmp (s, "mem") && mems != NULL) {
> +                parse_to_array (val, mems, ';', maxentries, PARSE_FLAG_SUFFIX);
> +            } else if (!strcmp (s, "cpu") && cpus != NULL) {
> +                parse_to_array (val, cpus, ';', maxentries, PARSE_FLAG_BITMASK);
> +            } else if (!strcmp (s, "pin") && hostnodes != NULL) {
> +                parse_to_array (val, hostnodes, ';', maxentries, 0);
> +            }
> +        }
> +    }
> +    free (arg);
> +    return num;
> +
>   

Please try to make things make the rest of qemu better.  You have a lot 
of weird space after function names, etc.  Consistency in large projects 
is important.

>  int main(int argc, char **argv, char **envp)
>  {
> @@ -4556,6 +4648,12 @@ int main(int argc, char **argv, char **e
>      for(i = 1; i < MAX_PARALLEL_PORTS; i++)
>          parallel_devices[i] = NULL;
>      parallel_device_index = 0;
> +
> +    for(i = 0; i < MAX_NODES; i++) {
> +        hostnodes[i] = (uint64_t)-1;
> +        node_to_cpus[i] = 0;
> +        node_mem[i] = 0;
> +    }
>  
>      usb_devices_index = 0;
>  
> @@ -5011,6 +5109,14 @@ int main(int argc, char **argv, char **e
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_numa:
> +                numnumanodes = parse_numa_args (optarg,
> +                    hostnodes, node_mem, node_to_cpus, MAX_NODES, 1);
> +                if (numnumanodes < 0) {
> +                    fprintf(stderr, "Invalid number of NUMA nodes\n");
> +                    exit(1);
> +                }
> +                break;
>  	    case QEMU_OPTION_vnc:
>  		vnc_display = optarg;
>  		break;
> @@ -5151,6 +5257,24 @@ int main(int argc, char **argv, char **e
>             monitor_device = "stdio";
>      }
>  
> +    if (numnumanodes > 0) {
> +        int i;
> +
> +        if (numnumanodes > smp_cpus)
> +            numnumanodes = smp_cpus;
>   

Why have this limitation?  We would like to see CPU-less nodes supported 
either as memory-only nodes or as IO nodes.

Which leads me to the question of how to plan on describing which nodes 
have what hardware?

Regards,

Anthony Liguori