On Fri, Jun 5, 2015 at 7:13 PM, Ian Campbell <ian.campb...@citrix.com>
wrote:
> On Thu, 2015-06-04 at 15:54 +0100, Ian Campbell wrote:
> > (redirecting to xen-devel as I originally intended)
> >
> > On Wed, 2015-06-03 at 13:02 +0800, lwch...@cs.hku.hk wrote:
> > > Hi,
> > >
> > > Wonder if there is any follow-ups from the relevant developers:
> > > (1) are you able to reproduce the "spinning" behavior of "xl vcpu-set"?
> >
> > I am with Xen 4.5, but not with xen-unstable AFAICT.
> >
> > > (2) if yes, can you confirm that it is due to looping with
> > > "retry_transaction"?
> >
> > I don't think so. You are _supposed_ to retry failed transactions in
> > this way, it's how they work.
> >
> > The issue is that the transaction is failing repeatedly in such a
> > manner. This might be due to a lack of error checking within the loop,
> > or it could possibly be a bug in the xenstore daemon.
>
> The real issue is that the loop terminator (info.vcpu_max_id) is -1, so
> we are just looping trying to set 4 billion or so CPUs online.
>
Oh yes, you are completely right. xenstore access log shows it is
indeed setting a huge amount of assumed online CPUs.
> Konrad's change below added an error check for this case, it should
> probably be backported.
>
> Given the operation can be interrupted with Ctrl-C I'm not sure there is
> any security impact.
>
Some third-part management tools might be built directly above xl.
Perhaps they can not rely on "Ctrl-C"..
Best,
Luwei
>
> Ian.
>
>
> commit d83bf9d224eeb5b73b93c2703f7dba4473cfa89c
> Author: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> Date: Fri Apr 3 16:02:29 2015 -0400
>
> libxl: In libxl_set_vcpuonline check for maximum number of VCPUs
> against the cpumap.
>
> There is no sense in trying to online (or offline) CPUs when the size
> of
> cpumap is greater than the maximum number of VCPUs the guest can go to.
>
> As such fail the operation if the count of CPUs to online is greater
> than what the guest started with. For the offline case we do not
> check (as the bits are unset in the cpumap) and let it go through.
>
> We coalesce some of the underlying libxl_set_vcpuonline code
> together which was duplicated in QMP and XenStore codepaths.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> Acked-by: Ian Campbell <ian.campb...@citrix.com>
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e6eebcc..debb20c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5445,27 +5445,19 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx,
> uint32_t domid,
> }
>
> static int libxl__set_vcpuonline_xenstore(libxl__gc *gc, uint32_t domid,
> - libxl_bitmap *cpumap)
> + libxl_bitmap *cpumap,
> + const libxl_dominfo *info)
> {
> - libxl_dominfo info;
> char *dompath;
> xs_transaction_t t;
> - int i, rc;
> -
> - libxl_dominfo_init(&info);
> + int i, rc = ERROR_FAIL;
>
> - rc = libxl_domain_info(CTX, &info, domid);
> - if (rc < 0) {
> - LOGE(ERROR, "getting domain info list");
> - goto out;
> - }
> - rc = ERROR_FAIL;
> if (!(dompath = libxl__xs_get_dompath(gc, domid)))
> goto out;
>
> retry_transaction:
> t = xs_transaction_start(CTX->xsh);
> - for (i = 0; i <= info.vcpu_max_id; i++)
> + for (i = 0; i <= info->vcpu_max_id; i++)
> libxl__xs_write(gc, t,
> libxl__sprintf(gc, "%s/cpu/%u/availability",
> dompath, i),
> "%s", libxl_bitmap_test(cpumap, i) ? "online" :
> "offline");
> @@ -5475,25 +5467,16 @@ retry_transaction:
> } else
> rc = 0;
> out:
> - libxl_dominfo_dispose(&info);
> return rc;
> }
>
> static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
> - libxl_bitmap *cpumap)
> + libxl_bitmap *cpumap,
> + const libxl_dominfo *info)
> {
> - libxl_dominfo info;
> - int i, rc;
> -
> - libxl_dominfo_init(&info);
> + int i;
>
> - rc = libxl_domain_info(CTX, &info, domid);
> - if (rc < 0) {
> - LOGE(ERROR, "getting domain info list");
> - libxl_dominfo_dispose(&info);
> - return rc;
> - }
> - for (i = 0; i <= info.vcpu_max_id; i++) {
> + for (i = 0; i <= info->vcpu_max_id; i++) {
> if (libxl_bitmap_test(cpumap, i)) {
> /* Return value is ignore because it does not tell anything
> useful
> * on the completion of the command.
> @@ -5503,33 +5486,53 @@ static int libxl__set_vcpuonline_qmp(libxl__gc
> *gc, uint32_t domid,
> libxl__qmp_cpu_add(gc, domid, i);
> }
> }
> - libxl_dominfo_dispose(&info);
> return 0;
> }
>
> int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap
> *cpumap)
> {
> GC_INIT(ctx);
> - int rc;
> + int rc, maxcpus;
> + libxl_dominfo info;
> +
> + libxl_dominfo_init(&info);
> +
> + rc = libxl_domain_info(CTX, &info, domid);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info list");
> + goto out;
> + }
> +
> + maxcpus = libxl_bitmap_count_set(cpumap);
> + if (maxcpus > info.vcpu_max_id + 1)
> + {
> + LOGE(ERROR, "Requested %d VCPUs, however maxcpus is %d!",
> + maxcpus, info.vcpu_max_id + 1);
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> switch (libxl__domain_type(gc, domid)) {
> case LIBXL_DOMAIN_TYPE_HVM:
> switch (libxl__device_model_version_running(gc, domid)) {
> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> - rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
> + rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
> break;
> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> - rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap);
> + rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap, &info);
> break;
> default:
> rc = ERROR_INVAL;
> }
> break;
> case LIBXL_DOMAIN_TYPE_PV:
> - rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap);
> + rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
> break;
> default:
> rc = ERROR_INVAL;
> }
> +out:
> + libxl_dominfo_dispose(&info);
> GC_FREE;
> return rc;
> }
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
