[libvirt] [PATCH 1/2] conf: Add support for preallocated fd memory

[Safka, JaroslavX]

Hi, thanks all for review
Comments inside

Best regards
Jarek

> -----Original Message-----
> From: Daniel P. Berrange [mailto:berrange at redhat.com]
> Sent: Tuesday, October 04, 2016 5:15 PM
> To: Michal Privoznik <mprivozn at redhat.com>
> Cc: Safka, JaroslavX <jaroslavx.safka at intel.com>; libvir-list at redhat.com
> Subject: Re: [libvirt] [PATCH 1/2] conf: Add support for preallocated fd memory
> 
> On Tue, Oct 04, 2016 at 05:07:16PM +0200, Michal Privoznik wrote:
> > On 29.09.2016 10:56, Jaroslav Safka wrote:
> > > This first change introduces xml parsing support for preallocated
> > > shared file descriptor based memory backing.
> > > It allows vhost-user to be used without hugepages.
> > >
> > > New xml elements:
> > > <memoryBacking>
> > >   <source type='file|anonymous' path='/path/to/qemu/' />
> 
> I'm pretty sure I said previously that path should *not* be present in the XML,
> as that is a linux-ism / internal impl detail not appropriate to expose.
> 

What way you suggest Daniel? There was two proposals in previous review from Sean to use xml or compile time parameter.
Btw there is something other than Linux? ;-)) (just joke)

> > >   <access Mode='shared|private'/>
> > >   <allocation mode='immediate|ondemand'/> </memoryBacking>
> >
> > Okay, this is definitely interesting approach (not only because while
> > reviewing this, I've found an old branch in my git where I've started
> > to work on this).
> >
> > Frankly, I don't know if this is a good API or not. Historically, we
> > required Dan's ACK on XML schema :-)
> 
> It is mostly ok, but what I think is missing though is integration with the existing
> logic in this area.
> 
> eg we have a access mode attribute on the NUMA cell:
> 
>       <cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/>
> 
> if this attribute is *not* specified on the NUMA cell, then the parser should be
> auto-filling it based on the top level <access mode> element. And of course test
> files to demonstrate that is working.
> 

I'm not sure if I get it. Actual code take these actions.
Find (switch) for VIR_NUMA_MEM_ACCESS_SHARED in cell element. If yes then add "share"
If it is VIR_NUMA_MEM_ACCESS_DEFAULT then look at the default from <access mode=.. />
And use it (add "share").

Also in reverse mode (generating xml) it will create the same xml as input xml.

> 
> 
> > > ---
> > >  docs/schemas/domaincommon.rng                      |  37 +++++
> > >  src/conf/domain_conf.c                             | 149 ++++++++++++++++-----
> > >  src/conf/domain_conf.h                             |  34 +++++
> > >  .../qemuxml2argv-memorybacking-set.xml             |  32 +++++
> > >  .../qemuxml2argv-memorybacking-unset.xml           |  32 +++++
> > >  .../qemuxml2xmlout-memorybacking-set.xml           |  40 ++++++
> > >  .../qemuxml2xmlout-memorybacking-unset.xml         |  40 ++++++
> > >  tests/qemuxml2xmltest.c                            |   3 +
> > >  8 files changed, 334 insertions(+), 33 deletions(-)  create mode
> > > 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml
> > >  create mode 100644
> > > tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml
> > >  create mode 100644
> > > tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml
> > >  create mode 100644
> > > tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml
> >
> > You need to update the docs too. formatdomain.html.in to be more precise.
> 


Yeah thanks, I must find it and look at it :)


> 
> Regards,
> Daniel
> --
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.