Re: Claim mode and HVM PoD interact badly

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jan 10, 2014 at 03:56:13PM +0000, Ian Campbell wrote:
> On Fri, 2014-01-10 at 10:28 -0500, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jan 10, 2014 at 03:16:25PM +0000, Ian Campbell wrote:
> > > On Fri, 2014-01-10 at 09:58 -0500, Konrad Rzeszutek Wilk wrote:
> > > > Which implies to me that we _need_ the 'maxmem' amount of memory at boot time.
> > > > And then it is the responsibility of the balloon driver to give the memory
> > > > back (and this is where the 'static-max' et al come in play to tell the
> > > > balloon driver to balloon out).
> > > 
> > > PoD exists purely so that we don't need the 'maxmem' amount of memory at
> > > boot time. It is basically there in order to let the guest get booted
> > > far enough to load the balloon driver to give the memory back.
> > > 
> > > It's basically a boot time zero-page sharing mechanism AIUI.
> > 
> > But it does look to gulp up hypervisor memory and return it during
> > allocation of memory for the guest.
> 
> It should be less than the maxmem-memory amount though. Perhaps because
> Wei is using relatively small sizes the pod cache ends up being a
> similar size to the saving?
> 
> Or maybe I just don't understand PoD, since the code you quote does seem
> to contradict that.
> 
> Or maybe libxl's calculation of pod_target is wrong?
> 
> > From reading the code the patch seems correct - we will _need_ that
> > extra 128MB 'claim' to allocate/free the 128MB extra pages. They
> > are temporary as we do free them.
> 
> It does makes sense that the PoD cache should be included in the claim,
> I just don't get why the cache is so big...

I think it expands and shrinks to make sure that the memory is present
in the hypervisor. If there is not enough memory it would -ENOMEM and
the toolstack would know immediately.

But that seems silly - as that memory might be in the future used
by other guests and then you won't be able to use said cache. But since
it is a "cache" I guess that is OK.

> 
> > > > diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
> > > > index 77bd365..65e9577 100644
> > > > --- a/tools/libxc/xc_hvm_build_x86.c
> > > > +++ b/tools/libxc/xc_hvm_build_x86.c
> > > > @@ -335,7 +335,12 @@ static int setup_guest(xc_interface *xch,
> > > >  
> > > >      /* try to claim pages for early warning of insufficient memory available */
> > > >      if ( claim_enabled ) {
> > > > -        rc = xc_domain_claim_pages(xch, dom, nr_pages - cur_pages);
> > > > +        unsigned long nr = nr_pages - cur_pages;
> > > > +
> > > > +        if ( pod_mode )
> > > > +            nr = target_pages - 0x20;
> > > 
> > > 0x20?
> > 
> > Yup. From earlier:
> > 
> > 305     if ( pod_mode )
> > 306     {
> > 307         /*
> > 308          * Subtract 0x20 from target_pages for the VGA "hole".  Xen will
> > 309          * adjust the PoD cache size so that domain tot_pages will be
> > 310          * target_pages - 0x20 after this call.
> > 311          */
> > 312         rc = xc_domain_set_pod_target(xch, dom, target_pages - 0x20,
> > 313                                       NULL, NULL, NULL);
> > 314         if ( rc != 0 )
> > 315         {
> > 316             PERROR("Could not set PoD target for HVM guest.\n");
> > 317             goto error_out;
> > 318         }
> > 319     }
> > 
> > Maybe a nice little 'pod_delta' or 'pod_pages' should be used instead of copying
> > this around.
> 
> A #define or something might be nice, yes.
> 
> > 
> > > 
> > > > +
> > > > +        rc = xc_domain_claim_pages(xch, dom, nr);
> > > >          if ( rc != 0 )
> > > >          {
> > > >              PERROR("Could not allocate memory for HVM guest as we cannot claim memory!");
> > > 
> > > 
> 
>