Re: [Xen-devel] [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

On Mon, Jul 11, 2016 at 12:31:43PM +0100, Wei Liu wrote:
[...]
> 
> > 
> > > +/* If the user did not explicitly specify a device model
> > > + * version, we need to use the same logic used during domain
> > > + * creation to derive the device model version.
> > > + */
> > > +version = d_config->b_info.device_model_version;
> > > +if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
> > > +version = libxl__get_device_model_version(gc, 
> > > _config->b_info);
> > 
> > I think this is rather unfortunate.  Won't changing the default device
> > model while the domain is running will cause this function to give
> > wrong answers ?
> 
> I think saving the device model into the template should work. No need
> to derive it during runtime.
> 

Actually this information is already available via
libxl__device_model_version_running.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

On Fri, Jul 08, 2016 at 06:44:28PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest 
> config"):
> > ... because the available vcpu bitmap can change during domain life time
> > due to cpu hotplug and unplug.
> > 
> > For QEMU upstream, we interrogate QEMU for the number of vcpus. For
> > others, we look directly into xenstore for information.
> ...
> > +static int libxl__update_avail_vcpus_qmp(libxl__gc *gc, uint32_t domid,
> > + unsigned int max_vcpus,
> > + libxl_bitmap *map)
> > +{
> > +int rc;
> > +
> > +/* For QEMU upstream we always need to return the number
> > + * of cpus present to QEMU whether they are online or not;
> > + * otherwise QEMU won't accept the saved state.
> 
> This comment is confusing to me.  Perhaps `return' should read
> `provide' ?  But the connection between the body of this function and
> the comment is still obscure.
> 

"provide" is ok.

To avoid confusion the comment can be moved a few line above to describe
the function, not the code snippet. Does that work better?

> > +static int libxl__update_avail_vcpus_xenstore(libxl__gc *gc, uint32_t 
> > domid,
> > +  unsigned int max_vcpus,
> > +  libxl_bitmap *map)
> > +{
> > +int rc;
> > +unsigned int i;
> > +const char *dompath;
> ...
> > +for (i = 0; i < max_vcpus; i++) {
> > +const char *path = GCSPRINTF("%s/cpu/%u/availability", dompath, i);
> > +const char *content = libxl__xs_read(gc, XBT_NULL, path);
> > +if (!strcmp(content, "online"))
> > +libxl_bitmap_set(map, i);
> 
> Firstly, this should be libxl__xs_read_checked.  Secondly if "content"
> is NULL, this will crash.
> 

Will fix.

> > @@ -7294,6 +7341,55 @@ int libxl_retrieve_domain_configuration(libxl_ctx 
> > *ctx, uint32_t domid,
> >  libxl_dominfo_dispose();
> >  }
> >  
> > +/* VCPUs */
> > +{
> > +libxl_bitmap *map = _config->b_info.avail_vcpus;
> > +unsigned int max_vcpus = d_config->b_info.max_vcpus;
> > +libxl_device_model_version version;
> > +
> > +libxl_bitmap_dispose(map);
> > +libxl_bitmap_init(map);
> > +libxl_bitmap_alloc(CTX, map, max_vcpus);
> > +libxl_bitmap_set_none(map);
> 
> I see that this function already trashes the domain config when it
> fails (leaving it up to the caller to free the partial config) but
> that this is not documented :-/.

d_config is an out parameter. User can't expect the returned d_config to
be valid if this API returns error state. And user is still responsible
for calling _init before calling this API and _dispose afterwards. So
this still fits into how we expect libxl types to be used. Nothing new
needs to be documented.

All other fields are already handled in this way.

> 
> > +/* If the user did not explicitly specify a device model
> > + * version, we need to use the same logic used during domain
> > + * creation to derive the device model version.
> > + */
> > +version = d_config->b_info.device_model_version;
> > +if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
> > +version = libxl__get_device_model_version(gc, 
> > _config->b_info);
> 
> I think this is rather unfortunate.  Won't changing the default device
> model while the domain is running will cause this function to give
> wrong answers ?

I think saving the device model into the template should work. No need
to derive it during runtime.

Wei.

> 
> Thanks,
> Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Ian Jackson]

Wei Liu writes ("[PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest 
config"):
> ... because the available vcpu bitmap can change during domain life time
> due to cpu hotplug and unplug.
> 
> For QEMU upstream, we interrogate QEMU for the number of vcpus. For
> others, we look directly into xenstore for information.
...
> +static int libxl__update_avail_vcpus_qmp(libxl__gc *gc, uint32_t domid,
> + unsigned int max_vcpus,
> + libxl_bitmap *map)
> +{
> +int rc;
> +
> +/* For QEMU upstream we always need to return the number
> + * of cpus present to QEMU whether they are online or not;
> + * otherwise QEMU won't accept the saved state.

This comment is confusing to me.  Perhaps `return' should read
`provide' ?  But the connection between the body of this function and
the comment is still obscure.

> +static int libxl__update_avail_vcpus_xenstore(libxl__gc *gc, uint32_t domid,
> +  unsigned int max_vcpus,
> +  libxl_bitmap *map)
> +{
> +int rc;
> +unsigned int i;
> +const char *dompath;
...
> +for (i = 0; i < max_vcpus; i++) {
> +const char *path = GCSPRINTF("%s/cpu/%u/availability", dompath, i);
> +const char *content = libxl__xs_read(gc, XBT_NULL, path);
> +if (!strcmp(content, "online"))
> +libxl_bitmap_set(map, i);

Firstly, this should be libxl__xs_read_checked.  Secondly if "content"
is NULL, this will crash.

> @@ -7294,6 +7341,55 @@ int libxl_retrieve_domain_configuration(libxl_ctx 
> *ctx, uint32_t domid,
>  libxl_dominfo_dispose();
>  }
>  
> +/* VCPUs */
> +{
> +libxl_bitmap *map = _config->b_info.avail_vcpus;
> +unsigned int max_vcpus = d_config->b_info.max_vcpus;
> +libxl_device_model_version version;
> +
> +libxl_bitmap_dispose(map);
> +libxl_bitmap_init(map);
> +libxl_bitmap_alloc(CTX, map, max_vcpus);
> +libxl_bitmap_set_none(map);

I see that this function already trashes the domain config when it
fails (leaving it up to the caller to free the partial config) but
that this is not documented :-/.

> +/* If the user did not explicitly specify a device model
> + * version, we need to use the same logic used during domain
> + * creation to derive the device model version.
> + */
> +version = d_config->b_info.device_model_version;
> +if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
> +version = libxl__get_device_model_version(gc, _config->b_info);

I think this is rather unfortunate.  Won't changing the default device
model while the domain is running will cause this function to give
wrong answers ?

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Anthony PERARD]

On Wed, Jun 15, 2016 at 10:31:41AM +0100, Wei Liu wrote:
> ... because the available vcpu bitmap can change during domain life time
> due to cpu hotplug and unplug.
> 
> For QEMU upstream, we interrogate QEMU for the number of vcpus. For
> others, we look directly into xenstore for information.
> 
> Reported-by: Jan Beulich 
> Signed-off-by: Wei Liu 

Reviewed-by: Anthony PERARD 

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

... because the available vcpu bitmap can change during domain life time
due to cpu hotplug and unplug.

For QEMU upstream, we interrogate QEMU for the number of vcpus. For
others, we look directly into xenstore for information.

Reported-by: Jan Beulich 
Signed-off-by: Wei Liu 
---
Cc: Ian Jackson 
Cc: Anthony PERARD 

v3:
1. Fix indentation of abort.
2. Use strcmp instead of strncmp.
---
 tools/libxl/libxl.c | 96 +
 1 file changed, 96 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 65af9ee..b748bf1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7246,6 +7246,53 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src)
 (*dst)[i] = (*src)[i];
 }
 
+static int libxl__update_avail_vcpus_qmp(libxl__gc *gc, uint32_t domid,
+ unsigned int max_vcpus,
+ libxl_bitmap *map)
+{
+int rc;
+
+/* For QEMU upstream we always need to return the number
+ * of cpus present to QEMU whether they are online or not;
+ * otherwise QEMU won't accept the saved state.
+ */
+rc = libxl__qmp_query_cpus(gc, domid, map);
+if (rc) {
+LOG(ERROR, "fail to get number of cpus for domain %d", domid);
+goto out;
+}
+
+rc = 0;
+out:
+return rc;
+}
+
+static int libxl__update_avail_vcpus_xenstore(libxl__gc *gc, uint32_t domid,
+  unsigned int max_vcpus,
+  libxl_bitmap *map)
+{
+int rc;
+unsigned int i;
+const char *dompath;
+
+dompath = libxl__xs_get_dompath(gc, domid);
+if (!dompath) {
+rc = ERROR_FAIL;
+goto out;
+}
+
+for (i = 0; i < max_vcpus; i++) {
+const char *path = GCSPRINTF("%s/cpu/%u/availability", dompath, i);
+const char *content = libxl__xs_read(gc, XBT_NULL, path);
+if (!strcmp(content, "online"))
+libxl_bitmap_set(map, i);
+}
+
+rc = 0;
+out:
+return rc;
+}
+
 int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
 libxl_domain_config *d_config)
 {
@@ -7294,6 +7341,55 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, 
uint32_t domid,
 libxl_dominfo_dispose();
 }
 
+/* VCPUs */
+{
+libxl_bitmap *map = _config->b_info.avail_vcpus;
+unsigned int max_vcpus = d_config->b_info.max_vcpus;
+libxl_device_model_version version;
+
+libxl_bitmap_dispose(map);
+libxl_bitmap_init(map);
+libxl_bitmap_alloc(CTX, map, max_vcpus);
+libxl_bitmap_set_none(map);
+
+/* If the user did not explicitly specify a device model
+ * version, we need to use the same logic used during domain
+ * creation to derive the device model version.
+ */
+version = d_config->b_info.device_model_version;
+if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
+version = libxl__get_device_model_version(gc, _config->b_info);
+
+switch (d_config->b_info.type) {
+case LIBXL_DOMAIN_TYPE_HVM:
+switch (version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+rc = libxl__update_avail_vcpus_qmp(gc, domid,
+   max_vcpus, map);
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+case LIBXL_DEVICE_MODEL_VERSION_NONE:
+rc = libxl__update_avail_vcpus_xenstore(gc, domid,
+max_vcpus, map);
+break;
+default:
+abort();
+}
+break;
+case LIBXL_DOMAIN_TYPE_PV:
+rc = libxl__update_avail_vcpus_xenstore(gc, domid,
+max_vcpus, map);
+break;
+default:
+abort();
+}
+
+if (rc) {
+LOG(ERROR, "fail to update available cpu map for domain %d", 
domid);
+goto out;
+}
+}
+
 /* Memory limits:
  *
  * Currently there are three memory limits:
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel