diff mbox

[v2,2/3] libxl: update vcpus bitmap in retrieved guest config

Message ID 1465396126-27426-3-git-send-email-wei.liu2@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Liu June 8, 2016, 2:28 p.m. UTC
... 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 <jbeulich@suse.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

Comments

Dario Faggioli June 9, 2016, 12:11 a.m. UTC | #1
On Wed, 2016-06-08 at 15:28 +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 <jbeulich@suse.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Dario
Anthony PERARD June 13, 2016, 5:39 p.m. UTC | #2
On Wed, Jun 08, 2016 at 03:28:45PM +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.

I tried to migrate a guest, and libxl abort in
libxl_retrieve_domain_configuration within the switch
(device_model_version).


> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 006b83f..02706ab 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -7222,6 +7222,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;

The value should already be 0 at this point.

> +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 (!strncmp(content, "online", strlen("online")))

I don't think strncmp is usefull here as one of the argument is a plain
string. One could just use strcmp?

> +            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)
>  {
> @@ -7270,6 +7317,46 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>          libxl_dominfo_dispose(&info);
>      }
>  
> +    /* VCPUs */
> +    {
> +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> +
> +        libxl_bitmap_dispose(map);
> +        libxl_bitmap_init(map);
> +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> +        libxl_bitmap_set_none(map);
> +
> +        switch (d_config->b_info.type) {
> +        case LIBXL_DOMAIN_TYPE_HVM:
> +            switch (d_config->b_info.device_model_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();

Missing indentation for abort.

Also, that is where xl abort on migration.

> +            }
> +            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:
Wei Liu June 13, 2016, 6:21 p.m. UTC | #3
On Mon, Jun 13, 2016 at 06:39:36PM +0100, Anthony PERARD wrote:
> On Wed, Jun 08, 2016 at 03:28:45PM +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.
> 
> I tried to migrate a guest, and libxl abort in
> libxl_retrieve_domain_configuration within the switch
> (device_model_version).
> 
> 
> > Reported-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 006b83f..02706ab 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -7222,6 +7222,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;
> 
> The value should already be 0 at this point.
> 

I would like to keep this as-is because this is an idiom that is safer
against further modification of this function.

> > +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 (!strncmp(content, "online", strlen("online")))
> 
> I don't think strncmp is usefull here as one of the argument is a plain
> string. One could just use strcmp?
> 

Fine by me of course.

> > +            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)
> >  {
> > @@ -7270,6 +7317,46 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> >          libxl_dominfo_dispose(&info);
> >      }
> >  
> > +    /* VCPUs */
> > +    {
> > +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> > +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> > +
> > +        libxl_bitmap_dispose(map);
> > +        libxl_bitmap_init(map);
> > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > +        libxl_bitmap_set_none(map);
> > +
> > +        switch (d_config->b_info.type) {
> > +        case LIBXL_DOMAIN_TYPE_HVM:
> > +            switch (d_config->b_info.device_model_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();
> 
> Missing indentation for abort.
> 

Will fix.

> Also, that is where xl abort on migration.
> 

Hmm...  This means the device model version is not valid (unknown?).

Can you paste in your guest config?

Wei.
Anthony PERARD June 14, 2016, 10:47 a.m. UTC | #4
On Mon, Jun 13, 2016 at 07:21:53PM +0100, Wei Liu wrote:
> On Mon, Jun 13, 2016 at 06:39:36PM +0100, Anthony PERARD wrote:
> > On Wed, Jun 08, 2016 at 03:28:45PM +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.
> > 
> > I tried to migrate a guest, and libxl abort in
> > libxl_retrieve_domain_configuration within the switch
> > (device_model_version).
> > 
> > 
> > > Reported-by: Jan Beulich <jbeulich@suse.com>
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 87 insertions(+)
> > > 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 006b83f..02706ab 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -7222,6 +7222,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;
> > 
> > The value should already be 0 at this point.
> > 
> 
> I would like to keep this as-is because this is an idiom that is safer
> against further modification of this function.
> 
> > > +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 (!strncmp(content, "online", strlen("online")))
> > 
> > I don't think strncmp is usefull here as one of the argument is a plain
> > string. One could just use strcmp?
> > 
> 
> Fine by me of course.
> 
> > > +            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)
> > >  {
> > > @@ -7270,6 +7317,46 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > >          libxl_dominfo_dispose(&info);
> > >      }
> > >  
> > > +    /* VCPUs */
> > > +    {
> > > +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> > > +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> > > +
> > > +        libxl_bitmap_dispose(map);
> > > +        libxl_bitmap_init(map);
> > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > +        libxl_bitmap_set_none(map);
> > > +
> > > +        switch (d_config->b_info.type) {
> > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > +            switch (d_config->b_info.device_model_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();
> > 
> > Missing indentation for abort.
> > 
> 
> Will fix.
> 
> > Also, that is where xl abort on migration.
> > 
> 
> Hmm...  This means the device model version is not valid (unknown?).
> 
> Can you paste in your guest config?

With all commented line removed:

builder = 'hvm'
memory = 500
vcpus = 2
maxvcpus = 6
name = "arch"
vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
disk = [
'phy:/dev/vg42/guest_arch64,hda,w',
 'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
]
device_model_args_hvm = [
]
usbdevice='tablet'
boot="cd"
serial='pty'
sdl = 0
vnc = 1
vnclisten = '0.0.0.0'
vncunused = 1
spice=0
uuid = "XXXX"
Wei Liu June 14, 2016, 10:50 a.m. UTC | #5
On Tue, Jun 14, 2016 at 11:47:57AM +0100, Anthony PERARD wrote:
[...]
> > > > +
> > > >  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > >                                          libxl_domain_config *d_config)
> > > >  {
> > > > @@ -7270,6 +7317,46 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > >          libxl_dominfo_dispose(&info);
> > > >      }
> > > >  
> > > > +    /* VCPUs */
> > > > +    {
> > > > +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> > > > +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> > > > +
> > > > +        libxl_bitmap_dispose(map);
> > > > +        libxl_bitmap_init(map);
> > > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > > +        libxl_bitmap_set_none(map);
> > > > +
> > > > +        switch (d_config->b_info.type) {
> > > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > > +            switch (d_config->b_info.device_model_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();
> > > 
> > > Missing indentation for abort.
> > > 
> > 
> > Will fix.
> > 
> > > Also, that is where xl abort on migration.
> > > 
> > 
> > Hmm...  This means the device model version is not valid (unknown?).
> > 
> > Can you paste in your guest config?
> 
> With all commented line removed:
> 
> builder = 'hvm'
> memory = 500
> vcpus = 2
> maxvcpus = 6
> name = "arch"
> vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
> disk = [
> 'phy:/dev/vg42/guest_arch64,hda,w',
>  'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
> ]
> device_model_args_hvm = [
> ]
> usbdevice='tablet'
> boot="cd"
> serial='pty'
> sdl = 0
> vnc = 1
> vnclisten = '0.0.0.0'
> vncunused = 1
> spice=0
> uuid = "XXXX"
> 

This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
so it should be covered by the correct case.

I'm confused.

Wei.

> 
> -- 
> Anthony PERARD
Anthony PERARD June 14, 2016, 10:58 a.m. UTC | #6
On Tue, Jun 14, 2016 at 11:50:12AM +0100, Wei Liu wrote:
> On Tue, Jun 14, 2016 at 11:47:57AM +0100, Anthony PERARD wrote:
> [...]
> > > > > +
> > > > >  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > > >                                          libxl_domain_config *d_config)
> > > > >  {
> > > > > @@ -7270,6 +7317,46 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > > >          libxl_dominfo_dispose(&info);
> > > > >      }
> > > > >  
> > > > > +    /* VCPUs */
> > > > > +    {
> > > > > +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> > > > > +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> > > > > +
> > > > > +        libxl_bitmap_dispose(map);
> > > > > +        libxl_bitmap_init(map);
> > > > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > > > +        libxl_bitmap_set_none(map);
> > > > > +
> > > > > +        switch (d_config->b_info.type) {
> > > > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > > > +            switch (d_config->b_info.device_model_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();
> > > > 
> > > > Missing indentation for abort.
> > > > 
> > > 
> > > Will fix.
> > > 
> > > > Also, that is where xl abort on migration.
> > > > 
> > > 
> > > Hmm...  This means the device model version is not valid (unknown?).
> > > 
> > > Can you paste in your guest config?
> > 
> > With all commented line removed:
> > 
> > builder = 'hvm'
> > memory = 500
> > vcpus = 2
> > maxvcpus = 6
> > name = "arch"
> > vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
> > disk = [
> > 'phy:/dev/vg42/guest_arch64,hda,w',
> >  'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
> > ]
> > device_model_args_hvm = [
> > ]
> > usbdevice='tablet'
> > boot="cd"
> > serial='pty'
> > sdl = 0
> > vnc = 1
> > vnclisten = '0.0.0.0'
> > vncunused = 1
> > spice=0
> > uuid = "XXXX"
> > 
> 
> This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> so it should be covered by the correct case.
> 
> I'm confused.

If it works for you, I'll properly install Xen with your patch in,
I may miss something...
I did:
LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost
Wei Liu June 14, 2016, 11 a.m. UTC | #7
On Tue, Jun 14, 2016 at 11:58:58AM +0100, Anthony PERARD wrote:
[...]
> > 
> > This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> > so it should be covered by the correct case.
> > 
> > I'm confused.
> 
> If it works for you, I'll properly install Xen with your patch in,
> I may miss something...

Yes, it did work for me.

> I did:
> LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost
> 

Never tried this so I'm not sure what might go wrong...

Wei.

> -- 
> Anthony PERARD
Anthony PERARD June 14, 2016, 1:20 p.m. UTC | #8
On Tue, Jun 14, 2016 at 11:58:58AM +0100, Anthony PERARD wrote:
> On Tue, Jun 14, 2016 at 11:50:12AM +0100, Wei Liu wrote:
> > On Tue, Jun 14, 2016 at 11:47:57AM +0100, Anthony PERARD wrote:
> > [...]
> > > > > > +
> > > > > >  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > > > >                                          libxl_domain_config *d_config)
> > > > > >  {
> > > > > > @@ -7270,6 +7317,46 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > > > >          libxl_dominfo_dispose(&info);
> > > > > >      }
> > > > > >  
> > > > > > +    /* VCPUs */
> > > > > > +    {
> > > > > > +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> > > > > > +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> > > > > > +
> > > > > > +        libxl_bitmap_dispose(map);
> > > > > > +        libxl_bitmap_init(map);
> > > > > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > > > > +        libxl_bitmap_set_none(map);
> > > > > > +
> > > > > > +        switch (d_config->b_info.type) {
> > > > > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > > > > +            switch (d_config->b_info.device_model_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();
> > > > > 
> > > > > Missing indentation for abort.
> > > > > 
> > > > 
> > > > Will fix.
> > > > 
> > > > > Also, that is where xl abort on migration.
> > > > > 
> > > > 
> > > > Hmm...  This means the device model version is not valid (unknown?).
> > > > 
> > > > Can you paste in your guest config?
> > > 
> > > With all commented line removed:
> > > 
> > > builder = 'hvm'
> > > memory = 500
> > > vcpus = 2
> > > maxvcpus = 6
> > > name = "arch"
> > > vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
> > > disk = [
> > > 'phy:/dev/vg42/guest_arch64,hda,w',
> > >  'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
> > > ]
> > > device_model_args_hvm = [
> > > ]
> > > usbdevice='tablet'
> > > boot="cd"
> > > serial='pty'
> > > sdl = 0
> > > vnc = 1
> > > vnclisten = '0.0.0.0'
> > > vncunused = 1
> > > spice=0
> > > uuid = "XXXX"
> > > 
> > 
> > This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> > so it should be covered by the correct case.
> > 
> > I'm confused.
> 
> If it works for you, I'll properly install Xen with your patch in,
> I may miss something...
> I did:
> LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost

I tried again, this time Xen properly installed with your patch series.

sh$ xl create arch.hvm
sh$ xl list
Name                                        ID   Mem VCPUs	State	Time(s)
Domain-0                                     0  4000     8     r-----      25.3
arch                                         1   500     2     -b----       5.4
sh$ xl vcpu-set arch 5
sh$ xl list
Name                                        ID   Mem VCPUs	State	Time(s)
Domain-0                                     0  4000     8     r-----      25.8
arch                                         1   500     5     -b----       6.4

# All fine up to here.

sh$ xl migrate arch localhost
Aborted (core dumped)
sh$  xl save arch savefile
Aborted (core dumped)
sh$ xl save arch savefile ~/arch.hvm
Saving to savefile new xl format (info 0x3/0x0/1540)
xc: info: Saving domain 1, type x86 HVM
xc: Frames: 1044482/1044482  100%
xc: End of stream: 0/0    0%

# So there is only one way to migrate or save the guest.

sh$ xl restore savefile
[...]
libxl: error: libxl_dm.c:2187:device_model_spawn_outcome: domain 2 device model: spawn failed (rc=-3)
[...]

# So restore still fail with this in QEMU log:
qemu-system-i386: Unknown savevm section or instance 'cpu_common' 2
qemu-system-i386: load of migration failed: Invalid argument
Ian Jackson June 14, 2016, 4:27 p.m. UTC | #9
Wei Liu writes ("Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config"):
> On Tue, Jun 14, 2016 at 11:58:58AM +0100, Anthony PERARD wrote:
> > I did:
> > LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost
> 
> Never tried this so I'm not sure what might go wrong...

That is how I do most of my ad-hoc testing.

If you have a newer xenstore etc. too you may need something like
  LD_LIBRARY_PATH=../xenstore:.

I find that relative paths work well with xl (because libxl is not
entitled to chdir, and xl doesn't).

Ian.
diff mbox

Patch

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 006b83f..02706ab 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7222,6 +7222,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 (!strncmp(content, "online", strlen("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)
 {
@@ -7270,6 +7317,46 @@  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
         libxl_dominfo_dispose(&info);
     }
 
+    /* VCPUs */
+    {
+        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
+        unsigned int max_vcpus = d_config->b_info.max_vcpus;
+
+        libxl_bitmap_dispose(map);
+        libxl_bitmap_init(map);
+        libxl_bitmap_alloc(CTX, map, max_vcpus);
+        libxl_bitmap_set_none(map);
+
+        switch (d_config->b_info.type) {
+        case LIBXL_DOMAIN_TYPE_HVM:
+            switch (d_config->b_info.device_model_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: