WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [Patch 2/2] support of cpupools in xl: commands and libr

To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Subject: Re: [Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes
From: Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>
Date: Tue, 05 Oct 2010 15:49:25 +0200
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Tue, 05 Oct 2010 06:54:16 -0700
Dkim-signature: v=1; a=rsa-sha256; c=simple/simple; d=ts.fujitsu.com; i=juergen.gross@xxxxxxxxxxxxxx; q=dns/txt; s=s1536b; t=1286286585; x=1317822585; h=message-id:date:from:mime-version:to:cc:subject: references:in-reply-to:content-transfer-encoding; z=Message-ID:=20<4CAB2CE5.1070208@xxxxxxxxxxxxxx>|Date:=20 Tue,=2005=20Oct=202010=2015:49:25=20+0200|From:=20Juergen =20Gross=20<juergen.gross@xxxxxxxxxxxxxx>|MIME-Version: =201.0|To:=20Ian=20Campbell=20<Ian.Campbell@xxxxxxxxxx> |CC:=20"xen-devel@xxxxxxxxxxxxxxxxxxx"=20<xen-devel@lists .xensource.com>|Subject:=20Re:=20[Xen-devel]=20[Patch=202 /2]=20support=20of=20cpupools=20in=20xl:=20commands=09and =0D=0A=20library=20changes|References:=20<4CA58BC1.903040 7@xxxxxxxxxxxxxx>=20<1285926115.16095.46889.camel@xxxxxxx k.xensource.com>|In-Reply-To:=20<1285926115.16095.46889.c amel@xxxxxxxxxxxxxxxxxxxxxx>|Content-Transfer-Encoding: =207bit; bh=HJ6MlHr7/Tv2mLrGVbbCn2/Ur2iyExK4MiEK54PshV0=; b=oVSWf0ZXXe95/iAl6nxxxUP+99k5AceXVK/8nce4zbYs1buwd8Ynu0Fi VDtXAM6zbGMIBMUytmhEB0yQKPiOt5WEKeznXmGI3lENtFGljZ7Xok1np NOvPUZD2J0poIxoUZkTz70PDHRZZgIzazWsrLf0PQzaccy/Agi0FP/4+b lWh3coIe8z2JnL8GwaOdG98xxLU29QWFcdv7bpcifUG4UDME9lDQfLgTc H98YdzXwbRYgw3rjm/2zrnOz3bIR0;
Domainkey-signature: s=s1536a; d=ts.fujitsu.com; c=nofws; q=dns; h=X-SBRSScore:X-IronPort-AV:Received:X-IronPort-AV: Received:Received:Message-ID:Date:From:Organization: User-Agent:MIME-Version:To:CC:Subject:References: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=iTcciLQLOKtTwUp9e6KgMFh5rdf7TOKWZivZ+1qt3jgkDvOAspUDi4cF BzK6KR4XV5rIUn42QmtagB+XYRYtB6Hq1f5/qyLZrf3rpnzZvnlkM1EbJ fKct9/iEsBEj7KvS4RZs09shtnh0/LvroY6zHXfGmWg0HlokWLOKgWYUi f08USOBlLJf6JHZqAE0YRWIFakidqOF94PGnrQ+0nFHyi20567wcQLGqo yrcCtiVgjWbQYFQd8dTAXxVH+F2eZ;
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1285926115.16095.46889.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Fujitsu Technology Solutions
References: <4CA58BC1.9030407@xxxxxxxxxxxxxx> <1285926115.16095.46889.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.12) Gecko/20100913 Iceowl/1.0b1 Icedove/3.0.7
On 10/01/10 11:41, Ian Campbell wrote:
Not a new issue with this series but is there documentation for the pool
configuration file format or an example somewhere? If not could you
maybe add an example at some point (e.g. under tools/examples)?

On Fri, 2010-10-01 at 08:20 +0100, Juergen Gross wrote:
Signed-off-by: juergen.gross@xxxxxxxxxxxxxx

Should go after the comment...

Support of cpu pools in xl:
   library functions
   xl pool-create
   xl pool-list
   xl pool-destroy
   xl pool-cpu-add
   xl pool-cpu-remove
   xl pool-migrate
Renamed all cpu pool related names to *cpupool*

But not the actual xl command names -- I presume this is for xm
compatibility, which is shame but unavoidable I suppose.

Hmm. What about adding aliases for xm (xm cpupool-*) and using only
the cpupool-* commands for xl?


diff -r f501dd7581e2 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Fri Oct 01 09:03:51 2010 +0200
+++ b/tools/libxl/libxl.c       Fri Oct 01 09:12:27 2010 +0200
@@ -607,10 +607,10 @@ int libxl_domain_info(libxl_ctx *ctx, li
      return 0;
  }

-libxl_poolinfo * libxl_list_pool(libxl_ctx *ctx, int *nb_pool)
+libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool)
  {
-    libxl_poolinfo *ptr;
-    int i;
+    libxl_cpupoolinfo *ptr;
+    int i, m;
      xc_cpupoolinfo_t *info;
      uint32_t poolid;
      libxl_physinfo physinfo;
@@ -627,12 +627,19 @@ libxl_poolinfo * libxl_list_pool(libxl_c
          info = xc_cpupool_getinfo(ctx->xch, poolid);
          if (info == NULL)
              break;
-        ptr = realloc(ptr, (i + 1) * sizeof(libxl_poolinfo));
+        ptr = realloc(ptr, (i + 1) * sizeof(libxl_cpupoolinfo));
          if (!ptr) {
              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool 
info");
              return NULL;
          }
          ptr[i].poolid = info->cpupool_id;
+        ptr[i].sched_id = info->sched_id;
+        ptr[i].n_dom = info->n_dom;
+        if (libxl_cpumap_alloc(&ptr[i].cpumap, physinfo.max_cpu_id + 1))
+            break;

Ah, here's the user of physinfo I couldn't find in the previous patch!

Why isn't this just using "info->cpumap_size * sizeof(*info->cpumap)"
though? (I have a feeling I asked this before but I can't find the
question or answer in the thread anywhere so maybe I just thought about
it).

Changing to xc_get_max_cpus().


I have a feeling that most users of these interfaces are more interested
in the number of CPUs represented by the cpumap rather than the number
of bytes which happen to be needed to represent them. Perhaps this is
something you could look at while changing the map to uint8_t?

I think this would make sense.


@@ -3659,3 +3664,180 @@ void libxl_file_reference_destroy(libxl_
      libxl__file_reference_unmap(f);
      free(f->path);
  }
+
+int libxl_get_freecpus(libxl_ctx *ctx, libxl_cpumap *cpumap)
+{
+    libxl_physinfo info;
+    int ret;
+
+    if (libxl_get_physinfo(ctx,&info) != 0)
+        return ERROR_FAIL;
+
+    ret = libxl_cpumap_alloc(cpumap, info.max_cpu_id + 1);
+    if (ret)
+        return ret;
+
+    if (xc_cpupool_freeinfo(ctx->xch, cpumap->map, cpumap->size)) {
+        free(cpumap->map);

This should be libxl_cpumap_destroy(&cpumap).

With the change of xc_cpupool_freeinfo() no need for free() any more.


+        cpumap->map = NULL;
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl_create_cpupool(libxl_ctx *ctx, char *name, int schedid,
+                         libxl_cpumap cpumap, libxl_uuid *uuid,
+                         uint32_t *poolid)
+{
+    libxl__gc gc = LIBXL_INIT_GC(ctx);
+    int rc;
+    int i;
+    xs_transaction_t t;
+    char *uuid_string;
+
+    uuid_string = libxl__uuid2string(&gc, *uuid);
+    if (!uuid_string)
+        return ERROR_NOMEM;
+
+    rc = xc_cpupool_create(ctx->xch, poolid, schedid);
+    if (rc) {
+        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+           "Could not create cpupool");
+        return ERROR_FAIL;
+    }
+
+    for (i = 0; i<  cpumap.size * 8; i++)
+        if (cpumap.map[i / 64]&  (1L<<  (i % 64))) {

I see a lot of this sort of thing in various places, perhaps it is worth
(later, not now) having an iterator in the style of the Linux foreach_*
macros? e.g.
        xl_cpumap_foreach_present(cpumap, i) {
                rc = xc_cpupool_addcpu(ctx->xch, *poolid, i);
                ...
        }


I'll look into this when I change the cpumap type.

+            rc = xc_cpupool_addcpu(ctx->xch, *poolid, i);
+            if (rc) {
+                LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+                    "Error moving cpu to cpupool");
+                return ERROR_FAIL;

I guess it's a toss up whether this should destroy the partially created
pool or not.

A call of libxl_destroy_cpupool() should do the job.


+            }
+        }
+
+    for (;;) {
+        t = xs_transaction_start(ctx->xsh);
+
+        xs_mkdir(ctx->xsh, t, libxl__sprintf(&gc, "/local/pool/%d", *poolid));
+        libxl__xs_write(&gc, t, libxl__sprintf(&gc, "/local/pool/%d/uuid", 
*poolid),
+                 uuid_string);
+        libxl__xs_write(&gc, t, libxl__sprintf(&gc, "/local/pool/%d/name", 
*poolid),
+                 name);
+
+        if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))
+            return 0;

Does this return success on failure with errno != EAGAIN?

Like lots of other libxl functions...
Missing xenstore entries seem not to be regarded as failures.


+    }
+}
+
+int libxl_destroy_cpupool(libxl_ctx *ctx, uint32_t poolid)
+{
+    libxl__gc gc = LIBXL_INIT_GC(ctx);
+    int rc, i;
+    xc_cpupoolinfo_t *info;
+    xs_transaction_t t;
+
+    info = xc_cpupool_getinfo(ctx->xch, poolid);
+    if (info == NULL)
+        return ERROR_NOMEM;
+
+    rc = ERROR_INVAL;
+    if ((info->cpupool_id != poolid) || (info->n_dom))
+        goto out;
+
+    for (i = 0; i<  info->cpumap_size; i++)
+        if (info->cpumap[i / 64]&  (1L<<  (i % 64))) {
+            rc = xc_cpupool_removecpu(ctx->xch, poolid, i);

I take it this is necessary and that destroying a pool doesn't
implicitly remove all CPUs from the pool?

Not in the hypervisor. This is subject to the tools.


+            if (rc) {
+                LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+                    "Error removing cpu from cpupool");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+        }
+
+    rc = xc_cpupool_destroy(ctx->xch, poolid);
+    if (rc) {
+        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, "Could not destroy 
cpupool");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    for (;;) {
+        t = xs_transaction_start(ctx->xsh);
+
+        xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "/local/pool/%d", 
poolid));
+
+        if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))
+            break;

Same comment wrt failure with errno != EAGAIN.

Same answer as above :-)


+int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid)
+{
+    libxl__gc gc = LIBXL_INIT_GC(ctx);
+    int rc;
+    char *dom_path;
+    char *vm_path;
+    char *poolname;
+    xs_transaction_t t;
+
+    dom_path = libxl__xs_get_dompath(&gc, domid);
+    if (!dom_path) {
+        return ERROR_FAIL;
+    }
+
+    rc = xc_cpupool_movedomain(ctx->xch, poolid, domid);
+    if (rc) {
+        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+            "Error moving domain to cpupool");
+        return ERROR_FAIL;
+    }
+
+    poolname = libxl__cpupoolid_to_name(&gc, poolid);
+    vm_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/vm", 
dom_path));
+    for (; vm_path;) {
+        t = xs_transaction_start(ctx->xsh);
+
+        libxl__xs_write(&gc, t, libxl__sprintf(&gc, "%s/pool_name", vm_path), 
poolname);
+
+        if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))

errno != EGAIN handling again.

Have you thought about possible races here? Is it possible that the
reads of vm_path and poolname need to be under the transaction to avoid
races with other operations renaming the pool or migrating the guest
etc?

Theoretically possible, yes.


@@ -5168,3 +5174,444 @@ int main_tmem_freeable(int argc, char **
      printf("%d\n", mb);
      return 0;
  }
+
+int main_poolcreate(int argc, char **argv)
+{
+    char *filename = NULL;
+    char *p, extra_config[1024];
+    int dryrun = 0;
+    int opt;
+    int option_index = 0;
+    static struct option long_options[] = {
+        {"help", 0, 0, 'h'},
+        {"defconfig", 1, 0, 'f'},
+        {"dryrun", 0, 0, 'n'},
+        {0, 0, 0, 0}
+    };
+    int ret;
+    void *config_data = 0;
+    int config_len = 0;
+    XLU_Config *config;
+    const char *buf;
+    char *name, *sched;
+    uint32_t poolid;
+    int schedid = -1;
+    XLU_ConfigList *cpus;
+    int n_cpus, i, n;
+    libxl_cpumap freemap;
+    libxl_cpumap cpumap;
+    libxl_uuid uuid;
+
+    while (1) {
+        opt = getopt_long(argc, argv, "hnf:", long_options,&option_index);
+        if (opt == -1)
+            break;
+
+        switch (opt) {
+        case 'f':
+            filename = optarg;
+            break;
+        case 'h':
+            help("pool-create");
+            return 0;
+        case 'n':
+            dryrun = 1;
+            break;
+        default:
+            fprintf(stderr, "option not supported\n");
+            break;
+        }
+    }
+
+    memset(extra_config, 0, sizeof(extra_config));
+    while (optind<  argc) {
+        if ((p = strchr(argv[optind], '='))) {
+            if (strlen(extra_config) + 1<  sizeof(extra_config)) {
+                if (strlen(extra_config))
+                    strcat(extra_config, "\n");
+                strcat(extra_config, argv[optind]);
+            }
+        } else if (!filename) {
+            filename = argv[optind];
+        } else {
+            help("pool-create");
+            return -ERROR_FAIL;
+        }
+        optind++;
+    }

Move this loop until after libxl_read_file_contents so you can add the
items directly to the end of the data along with the reallocs and
therefore avoid the 1024 character limitation?

The filename may be a result of this loop...


+
+    if (!filename) {
+        help("pool-create");
+        return -ERROR_FAIL;
+    }
+
+    if (libxl_read_file_contents(&ctx, filename,&config_data,&config_len)) {
+        fprintf(stderr, "Failed to read config file: %s: %s\n",
+                filename, strerror(errno));
+        return -ERROR_FAIL;
+    }
+    if (strlen(extra_config)) {
+        if (config_len>  INT_MAX - (strlen(extra_config) + 2)) {
+            fprintf(stderr, "Failed to attach extra configration\n");
+            return -ERROR_FAIL;
+        }
+        config_data = realloc(config_data,
+                              config_len + strlen(extra_config) + 2);
+        if (!config_data) {

Leaks previous config_data on failure, need to use a temporary variable.

Hmm. I had the impression xl isn't always free-ing all it's allocated
memory...
Switching to xrealloc.


+int main_poollist(int argc, char **argv)
+{
+    int opt;
+    int option_index = 0;
+    static struct option long_options[] = {
+        {"help", 0, 0, 'h'},
+        {"long", 0, 0, 'l'},
+        {"cpus", 0, 0, 'c'},
+        {0, 0, 0, 0}
+    };
+    int opt_long = 0;
+    int opt_cpus = 0;
+    char *pool = NULL;
+    libxl_cpupoolinfo *poolinfo;
+    int n_pools, p, c, n;
+    uint32_t poolid;
+    char *name;
+    int ret = 0;
+
+    while (1) {
+        opt = getopt_long(argc, argv, "hlc", long_options,&option_index);
+        if (opt == -1)
+            break;
+
+        switch (opt) {
+        case 'h':
+            help("pool-list");
+            return 0;
+        case 'l':
+            opt_long = 1;
+            break;
+        case 'c':
+            opt_cpus = 1;
+            break;
+        default:
+            fprintf(stderr, "option not supported\n");
+            break;
+        }
+    }
+
+    if ((optind + 1)<  argc) {
+        help("pool-list");
+        return -ERROR_FAIL;
+    }
+    if (optind<  argc) {
+        pool = argv[optind];
+        if (libxl_name_to_cpupoolid(&ctx, pool,&poolid)) {
+            fprintf(stderr, "Pool \'%s\' does not exist\n", pool);
+            return -ERROR_FAIL;
+        }
+    }
+
+    poolinfo = libxl_list_cpupool(&ctx,&n_pools);
+    if (!poolinfo) {
+        fprintf(stderr, "error getting cpupool info\n");
+        return -ERROR_NOMEM;
+    }
+
+    if (!opt_long) {
+        printf("%-19s", "Name");
+        if (opt_cpus)
+            printf("CPU list\n");
+        else
+            printf("CPUs   Sched     Active   Domain count\n");
+    }
+
+    for (p = 0; p<  n_pools; p++) {
+        if (!ret&&  (!pool || (poolinfo[p].poolid != poolid))) {
+            name = libxl_cpupoolid_to_name(&ctx, poolinfo[p].poolid);

Need to free name somewhere.

I can do it, but I think there is much more work ahead for freeing all
allocated memory in xl!


+            if (!name) {
+                fprintf(stderr, "error getting cpupool info\n");
+                ret = -ERROR_NOMEM;
+            }
+            else if (opt_long) {
+                ret = -ERROR_NI;
+            } else {
+                printf("%-19s", name);
+                n = 0;
+                for (c = 0; c<  poolinfo[p].cpumap.size * 8; c++)
+                    if (poolinfo[p].cpumap.map[c / 64]&  (1L<<  (c % 64))) {
+                        if (n&&  opt_cpus) printf(",");
+                        if (opt_cpus) printf("%d", c);
+                        n++;
+                    }
+                if (!opt_cpus) {
+                    printf("%3d %9s       y       %4d", n,
+                           libxl_schedid_to_name(&ctx, poolinfo[p].sched_id),
+                           poolinfo[p].n_dom);
+                }
+                printf("\n");
+            }
+        }
+        libxl_cpupoolinfo_destroy(poolinfo + p);
+    }
+
+    return ret;
+}
+
+int main_pooldestroy(int argc, char **argv)
+{
+    int opt;
+    char *pool;
+    uint32_t poolid;
+
+    while ((opt = getopt(argc, argv, "h")) != -1) {
+        switch (opt) {
+        case 'h':
+            help("pool-destroy");
+            return 0;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    pool = argv[optind];
+    if (!pool) {
+        fprintf(stderr, "no cpupool specified\n");
+        help("pool-destroy");
+        return -ERROR_FAIL;
+    }
+
+    if (cpupool_qualifier_to_cpupoolid(pool,&poolid, NULL) ||
+        !libxl_cpupoolid_to_name(&ctx, poolid)) {

Given that cpupool_qualifier_to_cpupoolid basically ==
libxl_name_to_cpupoolid is there really any need to check the inverse
before attempting to destroy the pool?

Yes.
cpupool_qualifier_to_cpupoolid() will always return 0 for any positive
integer given to it (like domain_qualifier_to_domid()). This may be
still no valid cpupoolid.


Juergen

--
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@xxxxxxxxxxxxxx
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel