Re: [Xen-devel] [PATCH v8 1/2] hypervisor: XENMEM_claim_pages (subop of existing) hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.11.12 at 16:50, Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3841,7 +3841,7 @@ int donate_page(
>      {
>          if ( d->tot_pages >= d->max_pages )
>              goto fail;
> -        d->tot_pages++;
> +        domain_adjust_tot_pages(d, 1);

I would generally expect there to always be paired increments
and decrements, yet I fail to spot this one's counterpart. There
is one a few lines down in steal_page().

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1656,7 +1656,7 @@ gnttab_transfer(
>          }
>  
>          /* Okay, add the page to 'e'. */
> -        if ( unlikely(e->tot_pages++ == 0) )
> +        if ( unlikely(domain_adjust_tot_pages(e, 1) == 1) )

This one's counterpart ought to also be the one in steal_page(),
but did you really check? As that isn't the first one you missed,
what did you do to find them all (apparently you didn't simply
grep for "tot_pages")?

> +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));
> +    /*
> +     * 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 )
> +        return d->tot_pages += pages;
> +    spin_lock(&heap_lock);
> +    d->tot_pages += pages;

Why don't you do this first thing, right after the comment above?
Would remove redundancy. Perhaps then even invert the if()
condition and have just a single return point.

Plus, if you went the route Keir suggested and split out the
conversion to domain_adjust_tot_pages() without anything
else, that would be what could probably go in
uncontroversially, making the remaining amount of changes
quite a bit smaller (and touching a more limited set of files).

Jan

> +    /* 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;
> +    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));
> +    total_unclaimed_pages = sys_after;
> +    spin_unlock(&heap_lock);
> +    return d->tot_pages;
> +}



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