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

Re: [libvirt] [PATCH] Implement CPU topology support for QEMU driver



On Tue, Jan 12, 2010 at 03:25:42PM +0100, Jiri Denemark wrote:
> QEMU's command line equivalent for the following domain XML fragment
>     <vcpus>2</vcpus>
>     <cpu ...>
>         ...
>         <topology sockets='1' cores='2', threads='1'/>
>     </cpu>
> 
> is
> 
>     -smp 2,sockets=1,cores=2,threads=1
> 
> This syntax was introduced in QEMU-0.12.
> 
> Signed-off-by: Jiri Denemark <jdenemar redhat com>
> ---
>  src/qemu/qemu_conf.c |  137 ++++++++++++++++++++++++++++++++++++++++++++------
>  src/qemu/qemu_conf.h |    3 +-
>  2 files changed, 123 insertions(+), 17 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index d3da776..bc4736a 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_conf.c: QEMU configuration management
>   *
> - * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc.
> + * Copyright (C) 2006, 2007, 2008, 2009, 2010 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -1115,6 +1115,10 @@ static unsigned int qemudComputeCmdFlags(const char *help,
>          flags |= QEMUD_CMD_FLAG_CHARDEV;
>      if (strstr(help, "-balloon"))
>          flags |= QEMUD_CMD_FLAG_BALLOON;
> +    if (strstr(help, "cores=") &&
> +        strstr(help, "threads=") &&
> +        strstr(help, "sockets="))
> +        flags |= QEMUD_CMD_FLAG_SMP_TOPOLOGY;
>  
>      if (version >= 9000)
>          flags |= QEMUD_CMD_FLAG_VNC_COLON;
> @@ -1900,7 +1904,6 @@ int qemudBuildCommandLine(virConnectPtr conn,
>                            const char *migrateFrom) {
>      int i;
>      char memory[50];
> -    char vcpus[50];
>      char boot[VIR_DOMAIN_BOOT_LAST];
>      struct utsname ut;
>      int disableKQEMU = 0;
> @@ -2049,7 +2052,6 @@ int qemudBuildCommandLine(virConnectPtr conn,
>       * is not supported, then they're out of luck anyway
>       */
>      snprintf(memory, sizeof(memory), "%lu", def->maxmem/1024);
> -    snprintf(vcpus, sizeof(vcpus), "%lu", def->vcpus);
>      snprintf(domid, sizeof(domid), "%d", def->id);
>  
>      ADD_ENV_LIT("LC_ALL=C");
> @@ -2112,8 +2114,34 @@ int qemudBuildCommandLine(virConnectPtr conn,
>          ADD_ARG_LIT("-mem-path");
>          ADD_ARG_LIT(driver->hugepage_path);
>      }
> +
>      ADD_ARG_LIT("-smp");
> -    ADD_ARG_LIT(vcpus);
> +    if ((qemuCmdFlags & QEMUD_CMD_FLAG_SMP_TOPOLOGY) && def->cpu) {
> +        virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +        virBufferVSprintf(&buf, "%lu", def->vcpus);
> +
> +        if (def->cpu->sockets > 0)
> +            virBufferVSprintf(&buf, ",sockets=%u", def->cpu->sockets);
> +
> +        if (def->cpu->cores > 0)
> +            virBufferVSprintf(&buf, ",cores=%u", def->cpu->cores);
> +
> +        if (def->cpu->threads > 0)
> +            virBufferVSprintf(&buf, ",threads=%u", def->cpu->threads);
> +
> +        if (virBufferError(&buf)) {
> +            virBufferFreeAndReset(&buf);
> +            goto no_memory;
> +        }
> +
> +        ADD_ARG(virBufferContentAndReset(&buf));
> +    }
> +    else {
> +        char vcpus[50];
> +        snprintf(vcpus, sizeof(vcpus), "%lu", def->vcpus);
> +        ADD_ARG_LIT(vcpus);
> +    }


Can you split this piece of code out into a separate method with a
signature like

  static char *qemuBuildCPUStr(virDomainDefPtr def, int qemuCmdFlags);

The main qemuBuildCommandLine method is getting out of control :-)

Also, in the case where 'def->cpu' is NULL, but QEMUD_CMD_FLAG_SMP_TOPOLOGY
is available, I think we should explicitly set an arg based on

   sockets=def->vcpus
   cores=1
   threads=1

so that we avoid relying on any changable QEMU default values.


>  
>      if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME) {
>          ADD_ARG_LIT("-name");
> @@ -3729,6 +3757,27 @@ error:
>  }
>  
>  
> +static virCPUDefPtr
> +qemuInitGuestCPU(virConnectPtr conn,
> +                 virDomainDefPtr dom)
> +{
> +    if (!dom->cpu) {
> +        virCPUDefPtr cpu;
> +
> +        if (VIR_ALLOC(cpu) < 0) {
> +            virReportOOMError(conn);
> +            return NULL;
> +        }
> +
> +        cpu->type = VIR_CPU_TYPE_GUEST;
> +        cpu->match = VIR_CPU_MATCH_EXACT;
> +        dom->cpu = cpu;
> +    }
> +
> +    return dom->cpu;
> +}
> +
> +
>  static int
>  qemuParseCommandLineCPU(virConnectPtr conn,
>                          virDomainDefPtr dom,
> @@ -3738,10 +3787,8 @@ qemuParseCommandLineCPU(virConnectPtr conn,
>      const char *p = val;
>      const char *next;
>  
> -    if (VIR_ALLOC(cpu) < 0)
> -        goto no_memory;
> -
> -    cpu->type = VIR_CPU_TYPE_GUEST;
> +    if (!(cpu = qemuInitGuestCPU(conn, dom)))
> +        goto error;
>  
>      do {
>          if (*p == '\0' || *p == ',')
> @@ -3785,7 +3832,6 @@ qemuParseCommandLineCPU(virConnectPtr conn,
>          }
>      } while ((p = next));
>  
> -    dom->cpu = cpu;
>      return 0;
>  
>  syntax:
> @@ -3796,7 +3842,71 @@ syntax:
>  no_memory:
>      virReportOOMError(conn);
>  error:
> -    virCPUDefFree(cpu);
> +    return -1;
> +}
> +
> +
> +static int
> +qemuParseCommandLineSmp(virConnectPtr conn,
> +                        virDomainDefPtr dom,
> +                        const char *val)
> +{
> +    const char *p = val;
> +    const char *next;
> +    const char *const options[] = { "sockets=", "cores=", "threads=" };
> +    unsigned int topology[] = { 0, 0, 0 };
> +    char *end;
> +
> +    do {
> +        if (*p == '\0' || *p == ',')
> +            goto syntax;
> +
> +        if ((next = strchr(p, ',')))
> +            next++;
> +
> +        if (c_isdigit(*p)) {
> +            int n;
> +            if (virStrToLong_i(p, &end, 10, &n) < 0 ||
> +                !end || (*end != ',' && *end != '\0'))
> +                goto syntax;
> +            dom->vcpus = n;
> +        } else {
> +            int i;
> +            int n = -1;
> +            for (i = 0; i < ARRAY_CARDINALITY(options); i++) {
> +                if (STRPREFIX(p, options[i])) {
> +                    p += strlen(options[i]);
> +                    if (virStrToLong_i(p, &end, 10, &n) < 0 ||
> +                        !end || (*end != ',' && *end != '\0'))
> +                        goto syntax;
> +                    topology[i] = n;
> +                    break;
> +                }
> +            }
> +
> +            if (n < 0)
> +                goto syntax;
> +        }
> +    } while ((p = next));

If you strip off the initial  CPU count value, then you can use

   qemuParseCommandLineKeywords

to parse the reset of the  name=value  args.


> +
> +    if (topology[0] && topology[1] && topology[2]) {
> +        virCPUDefPtr cpu;
> +
> +        if (!(cpu = qemuInitGuestCPU(conn, dom)))
> +            goto error;
> +
> +        cpu->sockets = topology[0];
> +        cpu->cores = topology[1];
> +        cpu->threads = topology[2];
> +    } else if (topology[0] || topology[1] || topology[2])
> +        goto syntax;
> +
> +    return 0;
> +
> +syntax:
> +    qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                     _("cannot parse CPU topology '%s'"), val);
> +error:
>      return -1;
>  }
>  
> @@ -3948,14 +4058,9 @@ virDomainDefPtr qemuParseCommandLine(virConnectPtr conn,
>              }
>              def->memory = def->maxmem = mem * 1024;
>          } else if (STREQ(arg, "-smp")) {
> -            int vcpus;
>              WANT_VALUE();
> -            if (virStrToLong_i(val, NULL, 10, &vcpus) < 0) {
> -                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, \
> -                                 _("cannot parse CPU count '%s'"), val);
> +            if (qemuParseCommandLineSmp(conn, def, val) < 0)
>                  goto error;
> -            }
> -            def->vcpus = vcpus;
>          } else if (STREQ(arg, "-uuid")) {
>              WANT_VALUE();
>              if (virUUIDParse(val, def->uuid) < 0) {
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 82254ca..3f74cc9 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_conf.h: QEMU configuration management
>   *
> - * Copyright (C) 2006, 2007, 2009 Red Hat, Inc.
> + * Copyright (C) 2006, 2007, 2009, 2010 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -78,6 +78,7 @@ enum qemud_cmd_flags {
>      QEMUD_CMD_FLAG_ENABLE_KVM    = (1 << 23), /* Is the -enable-kvm flag available to "enable KVM full virtualization support" */
>      QEMUD_CMD_FLAG_MONITOR_JSON  = (1 << 24), /* JSON mode for monitor */
>      QEMUD_CMD_FLAG_BALLOON       = (1 << 25), /* -balloon available */
> +    QEMUD_CMD_FLAG_SMP_TOPOLOGY  = (1 << 26), /* Is sockets=s,cores=c,threads=t available for -smp? */
>  };
>  
>  /* Main driver state */
> -- 

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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