Re: [PATCH] libxl: add memory attach support

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Wed, Aug 31, 2016 at 03:56:02PM +0800, Bob Liu wrote:
> On 08/30/2016 11:20 PM, Joao Martins wrote:
> > Hey!
> >
> > On 08/30/2016 11:00 AM, Bob Liu wrote:
> > > Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl
> > > driver, using libxl_set_memory_target in xen libxl.
> > >
> > > With "virsh attach-device" command we can then hotplug memory to a domain:
> > > <memory model='dimm'>
> > >     <target>
> > >         <size unit='MiB'>128</size>
> > >         <node>0</node>
> > >     </target>
> > > </memory>
> > >
> > > $ virsh attach-device domain_name this.xml --live

Actually, attaching a memory device should do something else than
calling libxl_set_memory_target().  It should add a guest-visible memory
device into the DIMM slot (especially when it is model='dimm').  I'm no
libxl expert, but the function you are using is just setting the memory
IMHO and it is the same as if you would remove the check for:
 newmem > virDomainDefGetMemoryTotal
in libxlDomainSetMemoryFlags()

> > > Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx>
> > See few very minor comments below, but overall LGTM.
> >
> > > ---
> > >  src/libxl/libxl_domain.c |  1 +
> > >  src/libxl/libxl_driver.c | 29 +++++++++++++++++++++++++++++
> > >  2 files changed, 30 insertions(+)
> > >
> > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > > index f529a2e..3924ba0 100644
> > > --- a/src/libxl/libxl_domain.c
> > > +++ b/src/libxl/libxl_domain.c
> > > @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
> > >      .macPrefix = { 0x00, 0x16, 0x3e },
> > >      .devicesPostParseCallback = libxlDomainDeviceDefPostParse,
> > >      .domainPostParseCallback = libxlDomainDefPostParse,
> > > +    .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG
> > >  };
> > >
> > >
> > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > > index a34eb02..541ea3b 100644
> > > --- a/src/libxl/libxl_driver.c
> > > +++ b/src/libxl/libxl_driver.c
> > > @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver,
> > >  #endif
> > >
> > >  static int
> > > +libxlDomainAttachMemory(libxlDriverPrivatePtr driver,
> > > +                        virDomainObjPtr vm,
> > > +                        virDomainMemoryDefPtr mem)
> > > +{
> > > +    int res = 0;
> > I think you don't need to initialize the variable.
> >
> > > +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> > > +
> > > +    if (mem->targetNode != 0) {
> > > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > +                       _("unsupport non-zero target node for memory device"));
> > Probably changing the message to "non-zero target node not supported" instead? The
> > messages sounds like you are removing support for it. But english is not my
> > mothertongue so may be it's just my wrong reading :)
> >
> > Should we fail with other error as this patch, or VIR_WARN the user and ignore
>
> Agree to use VIR_WARN(), will be updated.

I think definitely fail.  Otherwise you succeed with something that the
user clearly did not want.

> > targetNode. AFAIK libxl doesn't yet support ballooning for a specific node. What do
> > others think?

And it's said here as well, you are changing ballooning, which memory
hot-plug should not do.

> > > +        return -1;
> > > +    }
> > > +
> > > +    res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1);
> > Should we unlock virDomainObj while ballooning, as similarly done in
> > libxlDomainSetMemory* ?
>
> Yes, that's necessary.
>
> > > +    if (res < 0) {
> > > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > > +                       _("Failed to attach %lluKB memory for domain %d"),
> > > +                       mem->size, vm->def->id);
> > > +        return -1;
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > > +static int
> > >  libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver,
> > >                              virDomainObjPtr vm,
> > >                              virDomainHostdevDefPtr hostdev)
> > > @@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
> > >                  dev->data.hostdev = NULL;
> > >              break;
> > >
> > > +        case VIR_DOMAIN_DEVICE_MEMORY:
> > > +            ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
> > > +            dev->data.memory = NULL;
> > This should probably be:
> >
> > ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
> > if (!ret)
> >     dev->data.memory = NULL;
> >
> > Right?
>
> This code was from qemu:
> 7408         /* note that qemuDomainAttachMemory always consumes dev->data.memory
> 7409          * and dispatches DeviceAdded event on success */
> 7410         ret = qemuDomainAttachMemory(driver, vm,
> 7411                                      dev->data.memory);
> 7412         dev->data.memory = NULL;
>
> But I also forgot to free mem in libxlDomainAttachMemory(), thank you for the reminding.

qemu uses it because it calls virDomainMemoryInsert() which steals the
pointer data.  You are leaking the memory here.  Yes, you can free the
data in libxlDomainAttachMemory(), but in case it is not needed (like in
qemu), it is just creating ugly code.  Also looking at qemu's
implementation, there are bunch of safety checks and domain definition
updates which are definitely not done in this patch.

> > > +            break;
> > > +
> > >          default:
> > >              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > >                             _("device type '%s' cannot be attached"),
>
> --
> Regards,
> -Bob
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list