Re: [Xen-devel] [PATCH v5 1/4] xen: move steal_page and donate_page to common code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.09.13 at 18:06, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 
>>> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -230,6 +230,91 @@ int guest_remove_page(struct domain *d, unsigned long 
> gmfn)
>      return 1;
>  }
>  
> +int donate_page(
> +    struct domain *d, struct page_info *page, unsigned int memflags)
> +{
> +    spin_lock(&d->page_alloc_lock);
> +
> +    if ( is_xen_heap_page(page) || (page_get_owner(page) != NULL) )
> +        goto fail;
> +
> +    if ( d->is_dying )
> +        goto fail;
> +
> +    if ( page->count_info & ~(PGC_allocated | 1) )

While the literal 1 was acceptable in arch specific code, I'm not sure
it really is in generic code. Nothing enforces that an arch has the
count starting at bit 0. Even if it reads a little odd, using
(-PGC_count_mask & PGC_count_mask) would probably be an
acceptable replacement here and further down.

> +        goto fail;
> +
> +    if ( !(memflags & MEMF_no_refcount) )
> +    {
> +        if ( d->tot_pages >= d->max_pages )
> +            goto fail;
> +        domain_adjust_tot_pages(d, 1);
> +    }
> +
> +    page->count_info = PGC_allocated | 1;
> +    page_set_owner(page, d);
> +    page_list_add_tail(page,&d->page_list);

Please use the opportunity and insert a blank after the comma,
the more that you're not doing a literal copy of the function
anyway.

> +
> +    spin_unlock(&d->page_alloc_lock);
> +    return 0;
> +
> + fail:
> +    spin_unlock(&d->page_alloc_lock);
> +    gdprintk(XENLOG_WARNING, "Bad donate %p: ed=%p(%u), sd=%p, caf=%08lx, 
> taf=%016lx",
> +            (void *)page_to_mfn(page), d, d->domain_id,
> +            page_get_owner(page), page->count_info, page->u.inuse.type_info);

One of the (unfortunately many) cases where gdprintk() is wrong:
You don't care about the current domain here.

Also casting page_to_mfn() to void * in common code seems fragile.
I wonder why this cast was there in the original code: Using %lx
should be quite fine here.

> +    return -1;
> +}
> +
> +int steal_page(
> +    struct domain *d, struct page_info *page, unsigned int memflags)
> +{
> +    unsigned long x, y;
> +    bool_t drop_dom_ref = 0;
> +
> +    spin_lock(&d->page_alloc_lock);
> +
> +    if ( is_xen_heap_page(page) || (page_get_owner(page) != d) )
> +        goto fail;
> +
> +    /*
> +     * We require there is just one reference (PGC_allocated). We temporarily
> +     * drop this reference now so that we can safely swizzle the owner.
> +     */
> +    y = page->count_info;
> +    do {
> +        x = y;
> +        if ( (x & (PGC_count_mask|PGC_allocated)) != (1 | PGC_allocated) )
> +            goto fail;
> +        y = cmpxchg(&page->count_info, x, x & ~PGC_count_mask);
> +    } while ( y != x );
> +
> +    /* Swizzle the owner then reinstate the PGC_allocated reference. */
> +    page_set_owner(page, NULL);
> +    y = page->count_info;
> +    do {
> +        x = y;
> +        BUG_ON((x & (PGC_count_mask|PGC_allocated)) != PGC_allocated);
> +    } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
> +
> +    /* Unlink from original owner. */
> +    if ( !(memflags & MEMF_no_refcount) && !domain_adjust_tot_pages(d, -1) )
> +        drop_dom_ref = 1;
> +    page_list_del(page, &d->page_list);
> +
> +    spin_unlock(&d->page_alloc_lock);
> +    if ( unlikely(drop_dom_ref) )
> +        put_domain(d);
> +    return 0;
> +
> + fail:
> +    spin_unlock(&d->page_alloc_lock);
> +    printk("Bad page %p: ed=%p(%u), sd=%p, caf=%08lx, taf=%lx\n",
> +            (void *)page_to_mfn(page), d, d->domain_id,
> +            page_get_owner(page), page->count_info, page->u.inuse.type_info);

This clearly lacks a XENLOG_G_<something>. It would also be
desirable to have the word "steal" in the message somewhere, to
make it less generic.

Jan


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