Re: [PATCH 1/6] mmu: Introduce XENMEM_claim_pages (subop of memory ops).

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

On Tue, Mar 05, 2013 at 12:01:56PM +0000, Tim Deegan wrote:
> Hi,
> 
> At 12:47 -0500 on 04 Mar (1362401229), Konrad Rzeszutek Wilk wrote:
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -252,11 +252,124 @@ static long midsize_alloc_zone_pages;
> >  #define MIDSIZE_ALLOC_FRAC 128
> >  
> >  static DEFINE_SPINLOCK(heap_lock);
> > +static long total_unclaimed_pages; /* total outstanding claims by all domains */
> 
> Could this field (& associated per-domain stuff) have a better name?

Yes. d->outstanding_pages ?

> AFAICT from the code, this is a count of _claimed_ pages (specifically,
> claimed but not allocated).  It caused me to double-take almost every
> time I saw it used in this patch.

The word 'unclaimed' does not help either.

> 
> How about outstanding_claimed_pages, or just outstanding_claims?

outstanding_claims is a great name.

> 
> >  unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
> >  {
> > +    long dom_before, dom_after, dom_claimed, sys_before, sys_after;
> > +
> >      ASSERT(spin_is_locked(&d->page_alloc_lock));
> > -    return d->tot_pages += pages;
> > +    d->tot_pages += pages;
> > +
> > +    /*
> > +     * can test d->claimed_pages race-free because it can only change
> > +     * if d->page_alloc_lock and heap_lock are both held, see also
> > +     * domain_set_unclaimed_pages below
> > +     */
> > +    if ( !d->unclaimed_pages )
> > +        goto out;
> > +
> > +    spin_lock(&heap_lock);
> > +    /* adjust domain unclaimed_pages; may not go negative */
> > +    dom_before = d->unclaimed_pages;
> > +    dom_after = dom_before - pages;
> > +    if ( (dom_before > 0) && (dom_after < 0) )
> > +        dom_claimed = 0;
> 
> Why the test for dom_before > 0 ?  I think it might be better to
> BUG_ON() any of these counts ever being negative, rather than to carry
> on regardless.

I put in a BUG_ON( dom_before <= 0 ) and simplified that logic to be

    BUG_ON(dom_before < 0);
    dom_claimed = dom_after < 0 ? 0 : dom_after;


> 
> Or is this meant to be a cunning way of handling the case where 
> sizeof (long) == 4 and unclaimed_pages > 2^31 ?  I suspect that
> will fall foul of the undefinedness of signed overflow.
> 
> > +    else
> > +        dom_claimed = dom_after;
> > +    d->unclaimed_pages = dom_claimed;
> > +    /* flag accounting bug if system unclaimed_pages would go negative */
> > +    sys_before = total_unclaimed_pages;
> > +    sys_after = sys_before - (dom_before - dom_claimed);
> > +    BUG_ON((sys_before > 0) && (sys_after < 0));
> 
> Same question here.

That can certainly be simplified to be:
	+    BUG_ON(sys_after < 0);

> 
> > +/*
> > + * XENMEM_claim_pages flags:
> > + *  normal: claim is successful only if sufficient free pages
> > + *    are available.
> > + *  tmem: claim is successful only if sufficient free pages
> > + *    are available and (if tmem is enabled) hypervisor
> > + *    may also consider tmem "freeable" pages to satisfy the claim.
> 
> The 'normal' restriction isn't actually enforced except at claim time.
> Since the gate in alloc_heap_pages is effectively:
> 
>   (request > free + freeable - total_reserved)
> 
> later allocations (from any source) can take up all the free memory as
> long as freeable >= total_reserved).
> 
> Is there a use for it?  Presumably by turning on tmem you're happy with
> the idea that allocations might have to come from freeable memory?


The big thing is *might*. I put this in the code path to explain better:

  /* note; The usage of tmem claim means that allocation from a guest *might*
+     * have to come from freeable memory. Using free memory is always better, if
+     * it is available, than using freeable memory. This flag is for the use
+     * case where the toolstack cannot be constantly aware of the exact current
+     * value of free and/or freeable on each machine and is multi-machine
+     * capable. It can try/fail a "normal" claim on all machines first then,
+     * and if the normal claim on all machines fail, then "fallback" to a
+     * tmem-flag type claim.
+     *
+     * The memory claimed by a normal claim isn't enforced against "freeable
+     * pages" once the claim is successful. That's by design. Once the claim
+     * has been made, it still can take minutes before the claim is fully
+     * satisfied. Tmem can make use of the unclaimed pages during this time
+     * (to store ephemeral/freeable pages only, not persistent pages).
+     */

Here is the updated patch (with the long git commit description removed).

diff --git a/xen/common/domain.c b/xen/common/domain.c
index b360de1..90ce40b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -507,6 +507,7 @@ int domain_kill(struct domain *d)
         evtchn_destroy(d);
         gnttab_release_mappings(d);
         tmem_destroy(d->tmem);
+        domain_set_unclaimed_pages(d, 0, 0);
         d->tmem = NULL;
         /* fallthrough */
     case DOMDYING_dying:
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index b7f6619..c98e99c 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -154,6 +154,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
 
     info->tot_pages         = d->tot_pages;
     info->max_pages         = d->max_pages;
+    info->outstanding_pages = d->outstanding_pages;
     info->shr_pages         = atomic_read(&d->shr_pages);
     info->paged_pages       = atomic_read(&d->paged_pages);
     info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 08550ef..aa698b1 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -712,6 +712,37 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    case XENMEM_claim_pages:
+        if ( !IS_PRIV(current->domain) )
+            return -EPERM;
+
+        if ( copy_from_guest(&reservation, arg, 1) )
+            return -EFAULT;
+
+        if ( !guest_handle_is_null(reservation.extent_start) )
+            return -EINVAL;
+
+        if ( reservation.extent_order != 0 )
+            return -EINVAL;
+
+        d = rcu_lock_domain_by_id(reservation.domid);
+        if ( d == NULL )
+            return -EINVAL;
+
+        rc = domain_set_unclaimed_pages(d, reservation.nr_extents,
+                                        reservation.mem_flags);
+
+        rcu_unlock_domain(d);
+
+        break;
+
+    case XENMEM_get_unclaimed_pages:
+        if ( !IS_PRIV(current->domain) )
+            return -EPERM;
+
+        rc = get_outstanding_claims();
+        break;
+
     default:
         rc = arch_memory_op(op, arg);
         break;
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9e9fb15..73e2392 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -252,11 +252,137 @@ static long midsize_alloc_zone_pages;
 #define MIDSIZE_ALLOC_FRAC 128
 
 static DEFINE_SPINLOCK(heap_lock);
+static long outstanding_claims; /* total outstanding claims by all domains */
 
 unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
 {
+    long dom_before, dom_after, dom_claimed, sys_before, sys_after;
+
     ASSERT(spin_is_locked(&d->page_alloc_lock));
-    return d->tot_pages += pages;
+    d->tot_pages += pages;
+
+    /*
+     * can test d->claimed_pages race-free because it can only change
+     * if d->page_alloc_lock and heap_lock are both held, see also
+     * domain_set_unclaimed_pages below
+     */
+    if ( !d->outstanding_pages )
+        goto out;
+
+    spin_lock(&heap_lock);
+    /* adjust domain outstanding pages; may not go negative */
+    dom_before = d->outstanding_pages;
+    dom_after = dom_before - pages;
+    BUG_ON(dom_before < 0);
+    dom_claimed = dom_after < 0 ? 0 : dom_after;
+    d->outstanding_pages = dom_claimed;
+    /* flag accounting bug if system outstanding_claims would go negative */
+    sys_before = outstanding_claims;
+    sys_after = sys_before - (dom_before - dom_claimed);
+    BUG_ON(sys_after < 0);
+    outstanding_claims = sys_after;
+    spin_unlock(&heap_lock);
+
+out:
+    return d->tot_pages;
+}
+
+int domain_set_unclaimed_pages(struct domain *d, unsigned long pages,
+                                unsigned long flags)
+{
+    int ret = -ENOMEM;
+    unsigned long claim, avail_pages;
+
+    /*
+     * Sanity check.
+     */
+    switch ( flags ) {
+    case XENMEM_CLAIMF_normal:
+    case XENMEM_CLAIMF_tmem:
+        if ( pages == 0 )
+            return -EINVAL;
+        break;
+    case XENMEM_CLAIMF_cancel:
+        pages = 0;
+        break;
+    default:
+    return -ENOSYS;
+    }
+
+    /*
+     * take the domain's page_alloc_lock, else all d->tot_page adjustments
+     * must always take the global heap_lock rather than only in the much
+     * rarer case that d->outstanding_pages is non-zero
+     */
+    spin_lock(&d->page_alloc_lock);
+    spin_lock(&heap_lock);
+
+    /* pages==0 means "unset" the claim. */
+    if ( pages == 0 )
+    {
+        outstanding_claims -= d->outstanding_pages;
+        d->outstanding_pages = 0;
+        ret = 0;
+        goto out;
+    }
+
+    /* only one active claim per domain please */
+    if ( d->outstanding_pages )
+    {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* disallow a claim not exceeding current tot_pages or above max_pages */
+    if ( (pages <= d->tot_pages) || (pages > d->max_pages) )
+    {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* how much memory is available? */
+    avail_pages = total_avail_pages;
+    /* note; The usage of tmem claim means that allocation from a guest *might*
+     * have to come from freeable memory. Using free memory is always better, if
+     * it is available, than using freeable memory. This flag is for the use
+     * case where the toolstack cannot be constantly aware of the exact current
+     * value of free and/or freeable on each machine and is multi-machine
+     * capable. It can try/fail a "normal" claim on all machines first then,
+     * and if the normal claim on all machines fail, then "fallback" to a
+     * tmem-flag type claim.
+     *
+     * The memory claimed by a normal claim isn't enforced against "freeable
+     * pages" once the claim is successful. That's by design. Once the claim
+     * has been made, it still can take minutes before the claim is fully
+     * satisfied. Tmem can make use of the unclaimed pages during this time
+     * (to store ephemeral/freeable pages only, not persistent pages).
+     */
+    if ( flags & XENMEM_CLAIMF_tmem )
+        avail_pages += tmem_freeable_pages();
+    avail_pages -= outstanding_claims;
+
+    /*
+     * note, if domain has already allocated memory before making a claim
+     * then the claim must take tot_pages into account
+     */
+    claim = pages - d->tot_pages;
+    if ( claim > avail_pages )
+        goto out;
+
+    /* yay, claim fits in available memory, stake the claim, success! */
+    d->outstanding_pages = claim;
+    outstanding_claims += d->outstanding_pages;
+    ret = 0;
+
+out:
+    spin_unlock(&heap_lock);
+    spin_unlock(&d->page_alloc_lock);
+    return ret;
+}
+
+long get_outstanding_claims(void)
+{
+    return outstanding_claims;
 }
 
 static unsigned long init_node_heap(int node, unsigned long mfn,
@@ -397,7 +523,7 @@ static void __init setup_low_mem_virq(void)
 static void check_low_mem_virq(void)
 {
     unsigned long avail_pages = total_avail_pages +
-        (opt_tmem ? tmem_freeable_pages() : 0);
+        (opt_tmem ? tmem_freeable_pages() : 0) - outstanding_claims;
 
     if ( unlikely(avail_pages <= low_mem_virq_th) )
     {
@@ -466,6 +592,15 @@ static struct page_info *alloc_heap_pages(
     spin_lock(&heap_lock);
 
     /*
+     * Claimed memory is considered unavailable unless the request
+     * is made by a domain with sufficient unclaimed pages.
+     */
+    if ( (outstanding_claims + request >
+          total_avail_pages + tmem_freeable_pages()) &&
+          (d == NULL || d->outstanding_pages < request) )
+        goto not_found;
+
+    /*
      * TMEM: When available memory is scarce due to tmem absorbing it, allow
      * only mid-size allocations to avoid worst of fragmentation issues.
      * Others try tmem pools then fail.  This is a workaround until all
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index deb19db..113b8dc 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -36,7 +36,7 @@
 #include "grant_table.h"
 #include "hvm/save.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000008
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000009
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -95,6 +95,7 @@ struct xen_domctl_getdomaininfo {
     uint32_t flags;              /* XEN_DOMINF_* */
     uint64_aligned_t tot_pages;
     uint64_aligned_t max_pages;
+    uint64_aligned_t outstanding_pages;
     uint64_aligned_t shr_pages;
     uint64_aligned_t paged_pages;
     uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 1c5ca19..5c264ca 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -68,6 +68,8 @@ struct xen_memory_reservation {
      *   IN:  GPFN bases of extents to populate with memory
      *   OUT: GMFN bases of extents that were allocated
      *   (NB. This command also updates the mach_to_phys translation table)
+     * XENMEM_claim_pages:
+     *   IN: must be zero
      */
     XEN_GUEST_HANDLE(xen_pfn_t) extent_start;
 
@@ -430,10 +432,51 @@ typedef struct xen_mem_sharing_op xen_mem_sharing_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
 
 /*
- * Reserve ops for future/out-of-tree "claim" patches (Oracle)
+ * Attempt to stake a claim for a domain on a quantity of pages
+ * of system RAM, but _not_ assign specific pageframes.  Only
+ * arithmetic is performed so the hypercall is very fast and need
+ * not be preemptible, thus sidestepping time-of-check-time-of-use
+ * races for memory allocation.  Returns 0 if the hypervisor page
+ * allocator has atomically and successfully claimed the requested
+ * number of pages, else non-zero.
+ *
+ * Any domain may have only one active claim.  When sufficient memory
+ * has been allocated to resolve the claim, the claim silently expires.
+ * Claiming zero pages effectively resets any outstanding claim and
+ * is always successful.
+ *
+ * Note that a valid claim may be staked even after memory has been
+ * allocated for a domain.  In this case, the claim is not incremental,
+ * i.e. if the domain's tot_pages is 3, and a claim is staked for 10,
+ * only 7 additional pages are claimed.
+ *
+ * Caller must be privileged or the hypercall fails.
  */
 #define XENMEM_claim_pages                  24
-#define XENMEM_get_unclaimed_pages          25
+
+/*
+ * XENMEM_claim_pages flags:
+ *  normal: claim is successful only if sufficient free pages
+ *    are available.
+ *  tmem: claim is successful only if sufficient free pages
+ *    are available and (if tmem is enabled) hypervisor
+ *    may also consider tmem "freeable" pages to satisfy the claim.
+ *  cancel: cancel the outstanding claim for the domain.
+ */
+#define XENMEM_CLAIMF_ignored                0
+
+#define _XENMEM_CLAIMF_normal                1
+#define XENMEM_CLAIMF_normal                 (1U<<_XENMEM_CLAIMF_normal)
+#define _XENMEM_CLAIMF_tmem                  2
+#define XENMEM_CLAIMF_tmem                   (1U<<_XENMEM_CLAIMF_tmem)
+#define _XENMEM_CLAIMF_cancel                3
+#define XENMEM_CLAIMF_cancel                 (1U<<_XENMEM_CLAIMF_cancel)
+/*
+ * Get the number of pages currently claimed (but not yet "possessed")
+ * across all domains.  The caller must be privileged but otherwise
+ * the call never fails.
+ */
+#define XENMEM_get_unclaimed_pages            25
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 2f701f5..8a355b6 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -49,7 +49,11 @@ void free_xenheap_pages(void *v, unsigned int order);
 #define alloc_xenheap_page() (alloc_xenheap_pages(0,0))
 #define free_xenheap_page(v) (free_xenheap_pages(v,0))
 
+/* Claim handling */
 unsigned long domain_adjust_tot_pages(struct domain *d, long pages);
+int domain_set_unclaimed_pages(
+    struct domain *d, unsigned long pages, unsigned long flags);
+long get_outstanding_claims(void);
 
 /* Domain suballocator. These functions are *not* interrupt-safe.*/
 void init_domheap_pages(paddr_t ps, paddr_t pe);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index e108436..569e76e 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -242,6 +242,7 @@ struct domain
     struct page_list_head page_list;  /* linked list */
     struct page_list_head xenpage_list; /* linked list (size xenheap_pages) */
     unsigned int     tot_pages;       /* number of pages currently possesed */
+    unsigned int     outstanding_pages; /* pages claimed but not possessed  */
     unsigned int     max_pages;       /* maximum value for tot_pages        */
     atomic_t         shr_pages;       /* number of shared pages             */
     atomic_t         paged_pages;     /* number of paged-out pages          */