Re: [Xen-devel] [PATCH v2 2/2] xen/balloon: Enforce various limits on target

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Apr 29, 2013 at 03:44:09PM +0100, Ian Campbell wrote:
> On Mon, 2013-04-29 at 12:37 +0100, Daniel Kiper wrote:
>
> > This patch enforces on target limit statically defined in Linux Kernel
> > source and limit defined by hypervisor or host. This way the balloon
> > driver should not attempt to populate pages above given limits
> > because they may fail.
> >
> > Particularly this patch fixes bug which led to flood
> > of dom0 kernel log with messages similar to:
> >
> > System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added
> > xen_balloon: reserve_additional_memory: add_memory() failed: -17
>
> I think it would be OK to simply tone down this message (and perhaps add
> the failed pages to the balloon, if that makes sense). This isn't
> dissimilar to increase_reservation failing.

If add_memory() fails it is hard error. It means that we do not
know where new or ballooned pages should be placed.

> > +/*
> > + * Extra internal memory reserved by libxl.
> > + * Check tools/libxl/libxl_memory.txt file in Xen source for more details.
> > + */
> > +#define LIBXL_MAXMEM_CONSTANT_PAGES        (1024 * 1024 / PAGE_SIZE)
>
> I think we need to find a way to achieve your aims which doesn't require
> leaking internal implementation details of libxl into the guest kernels.
> What happens if libxl decides to double this?

I agree that this is not elegant solution. However, if we would like to
be in line with docs/misc/libxl_memory.txt (this is correct path) this
is a must. Once I thought that this value could be passed via xenstore
but I think it is rather small chance it would be changed in near
future. As I know this slack is reserved now just in case (correct me
if I am wrong). If this value will be changed we could pass new value
via xenstore (or other convenient mechanism).

> > +
> >  #ifdef CONFIG_HIGHMEM
> >  #define inc_totalhigh_pages() (totalhigh_pages++)
> >  #define dec_totalhigh_pages() (totalhigh_pages--)
> > @@ -491,11 +496,42 @@ static void balloon_process(struct work_struct *work)
> >     mutex_unlock(&balloon_mutex);
> >  }
> >
> > -/* Resets the Xen limit, sets new target, and kicks off processing. */
> > +/* Enforce limits, set new target and kick off processing. */
> >  void balloon_set_new_target(unsigned long target)
> >  {
> > +   domid_t domid = DOMID_SELF;
> > +   int rc;
> > +
> > +   /* Enforce statically defined limit. */
> > +   target = min(target, MAX_DOMAIN_PAGES);
> > +
> > +   rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
> > +
> > +   if (xen_initial_domain()) {
> > +           if (rc <= 0) {
> > +                   pr_debug("xen_balloon: %s: Initial domain target limit "
> > +                                   "could not be established: %i\n",
> > +                                   __func__, rc);
> > +                   goto no_host_limit;
> > +           }
> > +   } else {
> > +           if (rc <= 0) {
> > +                   pr_info("xen_balloon: %s: Guest domain target limit "
> > +                           "could not be established: %i\n", __func__, rc);
> > +                   goto no_host_limit;
> > +           }
> > +
> > +           /* Do not take into account memory reserved for internal stuff. 
> > */
> > +           rc -= LIBXL_MAXMEM_CONSTANT_PAGES;
> > +   }
>
> Why is this needed? Wouldn't it be a toolstack bug to set the target
> greater than this limit? But if it did ask then it would no doubt be
> expecting the guest to try and reach that limit (perhaps it intends to
> raise the maximum later?).

For domU XENMEM_maximum_reservation is always equal
<user_requested_maximum> + LIBXL_MAXMEM_CONSTANT_PAGES.
Acording to docs/misc/libxl_memory.txt LIBXL_MAXMEM_CONSTANT_PAGES
is reserved for extra internal. It means that we should not allow
balloon driver to reserve more than user_requested_maximum.

Daniel

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