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

[Tim Deegan <tim@xxxxxxx>]

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?
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.

How about outstanding_claimed_pages, or just outstanding_claims?

>  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.

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.

> +/*
> + * 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?

Cheers,

Tim.

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