Re: [Xen-devel] [RFC/PATCH v2] XENMEM_claim_pages (subop of existing) hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.11.12 at 00:55, Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote:
> @@ -685,6 +685,35 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case XENMEM_claim_pages:
> +    {
> +        unsigned long request;
> +
> +        start_extent = cmd >> MEMOP_EXTENT_SHIFT;

What's this? You don't do continuations (you explicitly do that
change in order to be fast), and hence this ought to be ignored
just like e.g. XENMEM_remove_from_physmap ignores it.

> +
> +        if ( copy_from_guest(&reservation, arg, 1) )
> +            return start_extent;

-EFAULT

> +
> +        if ( !(guest_handle_is_null(reservation.extent_start)) )
> +            return start_extent;

-EINVAL

> +
> +        rc = rcu_lock_target_domain_by_id(reservation.domid, &d);
> +        if ( rc )
> +            return rc;
> +
> +        /*
> +         * extent_order may be non-zero, but is only a multiplier and
> +         * does not currently claim any order>0 slabs, though this is
> +         * a possible future feature
> +         */

If extent_order doesn't do what it promises to do, just don't
allow non-zero values.

> +        request = reservation.nr_extents << reservation.extent_order;
> +        rc = domain_set_unclaimed_pages(d, request, reservation.mem_flags);
> +
> +        rcu_unlock_domain(d);
> +
> +        break;
> +    }
> +
>...
> +unsigned long domain_decrease_tot_pages(struct domain *d, unsigned long 
> pages)
> +{
> +    ASSERT(spin_is_locked(&d->page_alloc_lock));
> +    ASSERT(!spin_is_locked(&heap_lock));

There are numerous of these throughout the code, but while
asserting a certain lock to be held is valid, asserting that one
isn't being held (by _anyone_) clearly isn't. You're presumably
mixing this up with something like spin_is_owned()...

> +    spin_lock(&heap_lock);
> +    d->tot_pages -= pages;

Shouldn't you do the same lockless approach for the common case
here as you did on Keir's advice in domain_increase_tot_pages()?

> +    if ( d->unclaimed_pages )
> +    {
> +        d->unclaimed_pages += pages;
> +        total_unclaimed_pages += pages;
> +    }
> +    spin_unlock(&heap_lock);
> +    return d->tot_pages;
> +}
> +
> +int domain_set_unclaimed_pages(struct domain *d, unsigned long pages,
> +                                unsigned long flags)
> +{
> +    int ret = -ENOMEM;
> +    unsigned long claim = 0, avail_pages = 0;

Pointless initializers.

> +
> +    /*
> +     * take the domain's page_alloc_lock, else all increases/decreases
> +     * must always take the global heap_lock rather than only in the much
> +     * rarer case that d->unclaimed_pages is non-zero
> +     */
> +    ASSERT(!spin_is_locked(&d->page_alloc_lock));
> +    spin_lock(&d->page_alloc_lock);
> +    ASSERT(!spin_is_locked(&heap_lock));
> +    spin_lock(&heap_lock);
> +
> +    /* pages==0 means "unset" the claim (and flags is ignored) */
> +    if (pages == 0)
> +    {
> +        total_unclaimed_pages -= d->unclaimed_pages;
> +        d->unclaimed_pages = 0;
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    /* only one active claim per domain please */
> +    if ( d->unclaimed_pages)
> +        goto out;

-ENOMEM for this case seems wrong.

> +
> +    /* how much memory is available? */
> +    avail_pages = total_avail_pages;
> +    if ( !(flags & XENMEM_CLAIMF_free_only) )
> +        avail_pages += tmem_freeable_pages();
> +    avail_pages -= total_unclaimed_pages;
> +
> +    /*
> +     * 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;

What if pages < d->tot_pages?

> +
> +    /* yay, claim fits in available memory, stake the claim, success! */
> +    d->unclaimed_pages = claim;
> +    total_unclaimed_pages += d->unclaimed_pages;
> +    ret = 0;
> +
> +out:
> +    spin_unlock(&heap_lock);
> +    spin_unlock(&d->page_alloc_lock);
> +    return ret;
> +}
>  
>  static unsigned long init_node_heap(int node, unsigned long mfn,
>                                      unsigned long nr, bool_t *use_tail)
> @@ -443,6 +537,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 ( (total_unclaimed_pages + request >
> +           total_avail_pages + tmem_freeable_pages()) &&
> +          (d == NULL || d->unclaimed_pages < request) )
> +        goto not_found;

The treatment of d being NULL certainly needs further thought:
Is it really better to fulfill the claim and fail some (perhaps
important) _xmalloc()?

Also, I'm missing a mechanism by which the tools could find out
how much unclaimed memory is available, in order to determine
(if in use) how much memory needs to be ballooned out of Dom0.

Similarly, but perhaps of lower priority, there is no integration
with the low-mem handling.

Finally, there still are a number of formatting issues.

Jan

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