| Message ID | 20191224130416.3570-6-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xl/libxl: allow creation of domains with a specified domid | expand |
Paul Durrant writes ("[PATCH 5/6] libxl: allow creation of domains with specified or random domid"):
> This patch modifies do_domain_create() to use the value of domid that is
> passed in. A new 'special value' - RANDOM_DOMID - is added into the API
> and this, INVALID_DOMID or any valid domid is passed unmodified to
> libxl__domain_make(). Any other invalid domid value will cause an error.
>
> If RANDOM_DOMID is passed in then libxl__domain_make() will use
> libxl__random_bytes() to choose a domid. If the chosen value is not a
> valid domid then this step will be repeated. Once a valid value is chosen
> it will be passed to xc_domain_create() but if this fails with an errno
> value of EEXIST, a new random value will be chosen and the operation will
> be retried.
What is the use case for this ?
I think using this is hazardous if you ever destroy domains, because
it will lead to reuse of domids in circumstances[1] where right now
that can't happen.
[1] Fewer than ~2^16 creations per Xen boot.
Ian.
> -----Original Message----- > From: Ian Jackson <ian.jackson@citrix.com> > Sent: 02 January 2020 17:27 > To: Durrant, Paul <pdurrant@amazon.com> > Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard > <anthony.perard@citrix.com> > Subject: Re: [PATCH 5/6] libxl: allow creation of domains with specified > or random domid > > Paul Durrant writes ("[PATCH 5/6] libxl: allow creation of domains with > specified or random domid"): > > This patch modifies do_domain_create() to use the value of domid that is > > passed in. A new 'special value' - RANDOM_DOMID - is added into the API > > and this, INVALID_DOMID or any valid domid is passed unmodified to > > libxl__domain_make(). Any other invalid domid value will cause an error. > > > > If RANDOM_DOMID is passed in then libxl__domain_make() will use > > libxl__random_bytes() to choose a domid. If the chosen value is not a > > valid domid then this step will be repeated. Once a valid value is > chosen > > it will be passed to xc_domain_create() but if this fails with an errno > > value of EEXIST, a new random value will be chosen and the operation > will > > be retried. > > What is the use case for this ? > The use-case is trying to make sure that, across a pool of hosts, the likelihood of creating two domains with the same domid is small. Because a transparent migration requires that domid is persisted, migrations are more likely to fail (due to a domid already being in use) if a sequential scheme is used. > I think using this is hazardous if you ever destroy domains, because > it will lead to reuse of domids in circumstances[1] where right now > that can't happen. > > [1] Fewer than ~2^16 creations per Xen boot. Agreed, but is that actually a problem? A domain must be fully destroyed (i.e. all page refs dropped) before its id will become available for re-use. What are we actually trying to avoid by not immediately recycling? I'd certainly be open to adding some sort of retirement list for domids that would prevent re-use within a specified time, if that would help. Paul > > Ian.
On Tue, Dec 24, 2019 at 8:06 AM Paul Durrant <pdurrant@amazon.com> wrote: > > This patch modifies do_domain_create() to use the value of domid that is > passed in. A new 'special value' - RANDOM_DOMID - is added into the API > and this, INVALID_DOMID or any valid domid is passed unmodified to > libxl__domain_make(). Any other invalid domid value will cause an error. > > If RANDOM_DOMID is passed in then libxl__domain_make() will use > libxl__random_bytes() to choose a domid. If the chosen value is not a > valid domid then this step will be repeated. Once a valid value is chosen > it will be passed to xc_domain_create() but if this fails with an errno > value of EEXIST, a new random value will be chosen and the operation will > be retried. > > If a valid domid is passed in and xc_domain_create() fails with errno > set to EEXIST then this will result in a new error value of > ERROR_DEVICE_EXISTS being returned from libxl__domain_make(). This is > done so that domcreate_complete() can be adjusted so as not to tear down > the existing domain that the attempted creation happened to collide with. > > NOTE: libxl__logv() is also modified to only log valid domid values in > messages rather than any domid, valid or otherwise, that is not > INVALID_DOMID. > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: Anthony PERARD <anthony.perard@citrix.com> > --- <snip> > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c <snip> > @@ -571,6 +569,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > .max_grant_frames = b_info->max_grant_frames, > .max_maptrack_frames = b_info->max_maptrack_frames, > }; > + uint32_t in_domid = *domid; > > if (info->type != LIBXL_DOMAIN_TYPE_PV) { > create.flags |= XEN_DOMCTL_CDF_hvm; > @@ -600,10 +599,24 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, > goto out; > } > > - ret = xc_domain_create(ctx->xch, domid, &create); > + for (;;) { > + if (in_domid == RANDOM_DOMID) { > + ret = libxl__random_bytes(gc, (void *)domid, sizeof(*domid)); Since valid domids are ~0-2^15, you may want to used a temporary uint16_t instead of the uint32_t domid to tighten up the range. Regards, Jason > + if (ret < 0) > + break; > + > + if (!libxl_domid_valid_guest(*domid)) > + continue; > + } > + > + ret = xc_domain_create(ctx->xch, domid, &create); > + if (ret == 0 || errno != EEXIST || in_domid != RANDOM_DOMID) > + break; > + } > +
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 18c1a2d6bf..6e7f5a0241 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -1268,6 +1268,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src); */ #define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG +/* + * LIBXL_HAVE_SPECIFIED_DOMID + * + * libxl_domain_create_new() and libxl_domain_create_restore() will use + * a caller specified domid value. + */ +#define LIBXL_HAVE_SPECIFIED_DOMID + typedef char **libxl_string_list; void libxl_string_list_dispose(libxl_string_list *sl); int libxl_string_list_length(const libxl_string_list *sl); @@ -1528,7 +1536,11 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */); /* domain related functions */ #define INVALID_DOMID ~0 +#define RANDOM_DOMID (INVALID_DOMID - 1) +/* On entry a domid == RANDOM_DOMID, a valid random domain id will be + * chosen, otherwise if domid is a valid value then that will be used as + * the domain id */ /* If the result is ERROR_ABORTED, the domain may or may not exist * (in a half-created state). *domid will be valid and will be the * domain id, or INVALID_DOMID, as appropriate */ diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 1835a5502c..1d98567f59 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -555,8 +555,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, libxl_domain_create_info *info = &d_config->c_info; libxl_domain_build_info *b_info = &d_config->b_info; - assert(soft_reset || *domid == INVALID_DOMID); - uuid_string = libxl__uuid2string(gc, info->uuid); if (!uuid_string) { rc = ERROR_NOMEM; @@ -571,6 +569,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, .max_grant_frames = b_info->max_grant_frames, .max_maptrack_frames = b_info->max_maptrack_frames, }; + uint32_t in_domid = *domid; if (info->type != LIBXL_DOMAIN_TYPE_PV) { create.flags |= XEN_DOMCTL_CDF_hvm; @@ -600,10 +599,24 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, goto out; } - ret = xc_domain_create(ctx->xch, domid, &create); + for (;;) { + if (in_domid == RANDOM_DOMID) { + ret = libxl__random_bytes(gc, (void *)domid, sizeof(*domid)); + if (ret < 0) + break; + + if (!libxl_domid_valid_guest(*domid)) + continue; + } + + ret = xc_domain_create(ctx->xch, domid, &create); + if (ret == 0 || errno != EEXIST || in_domid != RANDOM_DOMID) + break; + } + if (ret < 0) { - LOGED(ERROR, *domid, "domain creation fail"); - rc = ERROR_FAIL; + LOGED(ERROR, in_domid, "domain creation fail"); + rc = (errno == EEXIST) ? ERROR_DEVICE_EXISTS : ERROR_FAIL; goto out; } @@ -1111,7 +1124,6 @@ static void initiate_domain_create(libxl__egc *egc, if (ret) { LOGD(ERROR, domid, "cannot make domain: %d", ret); dcs->guest_domid = domid; - ret = ERROR_FAIL; goto error_out; } @@ -1752,7 +1764,8 @@ static void domcreate_complete(libxl__egc *egc, if (!rc && d_config->b_info.exec_ssidref) rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, d_config->b_info.exec_ssidref); - bool retain_domain = !rc || rc == ERROR_ABORTED; + bool retain_domain = !rc || rc == ERROR_ABORTED || + rc == ERROR_DEVICE_EXISTS; if (retain_domain) { libxl__domain_userdata_lock *lock; @@ -1845,7 +1858,21 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, if (rc < 0) goto out_err; } cdcs->dcs.callback = domain_create_cb; - cdcs->dcs.domid = INVALID_DOMID; + + /* Allow valid and special values */ + switch (*domid) { + case INVALID_DOMID: + case RANDOM_DOMID: + break; + default: + if (libxl_domid_valid_guest(*domid)) + break; + + rc = ERROR_FAIL; + goto out_err; + } + + cdcs->dcs.domid = *domid; cdcs->dcs.soft_reset = false; if (cdcs->dcs.restore_params.checkpointed_stream == diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c index ba5637358e..dc6aaa9c9f 100644 --- a/tools/libxl/libxl_internal.c +++ b/tools/libxl/libxl_internal.c @@ -234,7 +234,7 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval, fileline[sizeof(fileline)-1] = 0; domain[0] = 0; - if (domid != INVALID_DOMID) + if (libxl_domid_valid_guest(domid)) snprintf(domain, sizeof(domain), "Domain %"PRIu32":", domid); x: xtl_log(ctx->lg, msglevel, errnoval, "libxl",
This patch modifies do_domain_create() to use the value of domid that is passed in. A new 'special value' - RANDOM_DOMID - is added into the API and this, INVALID_DOMID or any valid domid is passed unmodified to libxl__domain_make(). Any other invalid domid value will cause an error. If RANDOM_DOMID is passed in then libxl__domain_make() will use libxl__random_bytes() to choose a domid. If the chosen value is not a valid domid then this step will be repeated. Once a valid value is chosen it will be passed to xc_domain_create() but if this fails with an errno value of EEXIST, a new random value will be chosen and the operation will be retried. If a valid domid is passed in and xc_domain_create() fails with errno set to EEXIST then this will result in a new error value of ERROR_DEVICE_EXISTS being returned from libxl__domain_make(). This is done so that domcreate_complete() can be adjusted so as not to tear down the existing domain that the attempted creation happened to collide with. NOTE: libxl__logv() is also modified to only log valid domid values in messages rather than any domid, valid or otherwise, that is not INVALID_DOMID. Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wl@xen.org> Cc: Anthony PERARD <anthony.perard@citrix.com> --- tools/libxl/libxl.h | 12 ++++++++++ tools/libxl/libxl_create.c | 43 +++++++++++++++++++++++++++++------- tools/libxl/libxl_internal.c | 2 +- 3 files changed, 48 insertions(+), 9 deletions(-)