[Qemu-devel] Re: [PATCH 1/8] v2: introduce -numa command line option

[Anthony Liguori <anthony@codemonkey.ws>]

Andre Przywara wrote:
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>
> # HG changeset patch
> # User Andre Przywara <andre.przywara@amd.com>
> # Date 1229425990 -3600
> # Node ID 6690ab0a34fdedaebbfe1068dfe7351734e8a1d7
> # Parent  4271a7547668e4b02cae88e66c936d9cacab624e
> add -numa command line option
>
> diff -r 4271a7547668 -r 6690ab0a34fd sysemu.h
>
> @@ -4065,6 +4072,7 @@ enum {
>      QEMU_OPTION_usb,
>      QEMU_OPTION_usbdevice,
>      QEMU_OPTION_smp,
> +    QEMU_OPTION_numa,
>      QEMU_OPTION_vnc,
>      QEMU_OPTION_no_acpi,
>      QEMU_OPTION_curses,
> @@ -4171,6 +4179,7 @@ static const QEMUOption qemu_options[] =
>      { "win2k-hack", 0, QEMU_OPTION_win2k_hack },
>      { "usbdevice", HAS_ARG, QEMU_OPTION_usbdevice },
>      { "smp", HAS_ARG, QEMU_OPTION_smp },
> +    { "numa", HAS_ARG, QEMU_OPTION_numa},
>      { "vnc", HAS_ARG, QEMU_OPTION_vnc },
>  #ifdef CONFIG_CURSES
>      { "curses", 0, QEMU_OPTION_curses },
> @@ -4456,6 +4465,94 @@ static void termsig_setup(void)
>  }
>  
>  #endif
> +
> +#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;
> +
> +    for (s = arg; s != NULL && *s != 0; s++) {
> +        val = strtoull(s, &end, 10);
> +        if (end == s && *s != '*') {
> +            num++; continue;
> +        }
> +        if (num >= maxentries) break;
> +        if (*end == delim && (flags & PARSE_FLAG_SUFFIX))
> +            val *= 1024 * 1024;
> +        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;
>   

The fall throughs here are very confusion.  No suffix means G or bitmask 
depending on the context?  The indenting is really messed up in this 
function too.

> +int parse_numa_args(const char *opt, uint64_t *mems,
> +                     uint64_t *cpus, int maxentries, int expect_numnodes)
> +{
> +const char *s;
> +char *arg, *val, *end, *token;
> +int num = 0;
>   

This indenting is messed up.

> +    arg = strdup(opt); token = arg;
> +    if (expect_numnodes) {
> +        s = strsep(&token, ",");
> +        if (s == NULL) {
> +            free(arg);
> +            return -1;
> +        }
> +        num = strtol(s, &end, 10);
> +        if (s == end) {
> +            free(arg);
> +            return -1;
> +        }
> +    }
> +    while ((s=strsep(&token, ","))!=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);
> +            }
> +        }
> +    }
> +    free(arg);
> +    return num;
> +}
>  
>  int main(int argc, char **argv, char **envp)
>  {
> @@ -4556,6 +4653,11 @@ 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++) {
> +        node_to_cpus[i] = 0;
> +        node_mem[i] = 0;
> +    }
>  
>      usb_devices_index = 0;
>  
> @@ -5011,6 +5113,20 @@ int main(int argc, char **argv, char **e
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_numa:
> +                if (numnumanodes > 0)
> +                    parse_numa_args(optarg, node_mem,
> +                        node_to_cpus, MAX_NODES, 0);
> +                else
> +                    numnumanodes = parse_numa_args(optarg,
> +                        node_mem, node_to_cpus, MAX_NODES, 1);
> +                numnumanodes = parse_numa_args(optarg,
> +                    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 +5267,24 @@ int main(int argc, char **argv, char **e
>             monitor_device = "stdio";
>      }
>  
> +    if (numnumanodes > 0) {
> +        int i;
> +
> +        if (numnumanodes > smp_cpus)
> +            numnumanodes = smp_cpus;
> +
> +        for (i = 0; i < numnumanodes; i++) if (node_mem[i] != 0) break;
>   

Please split to multiple lines.

> +        if (i == numnumanodes) {
> +            for (i = 0; i < numnumanodes; i++)
> +                node_mem[i] = (ram_size / numnumanodes) & ~((1 << 20UL) - 1);
> +        }
> +        for (i = 0; i < numnumanodes; i++) if (node_to_cpus[i] != 0) break;
> +        if (i == numnumanodes) {
> +            for (i = 0; i < smp_cpus; i++)
> +                node_to_cpus[i % numnumanodes] |= 1<<i;
>   

The way CPUs are allocate here seems strange?  Each CPU is assigned 
round robin?  Should you have node 0 contain 1..X, node 1 contain X..Y, 
node 2 contain Y..smp_cpus?

Regards,

Anthony Liguori