Re: [PATCH] Implement faster superpage mapping

[Keir Fraser <keir.fraser@eu.citrix.com>]

On 13/05/2010 20:40, "Dave McCracken" <dcm@mccr.org> wrote:

> Here's my first cut of a faster superpage mapping scheme.  It uses a separate
> superpage table to track mappings of superpages and mappings that conflict
> with using a superpage.
> 
> One new feature of this code is that it requires that every superpage be
> allocated to a domain with a single call.  This ensures that every page in the
> superpage is allocated to the same domain.

Hm, not sure about that restriction but in any case it's not the weakest
aspect of the patch by far anyway...

> +static void enable_superpage(
> +    struct page_info *pg,
> +    unsigned int order)
> +{
> +    struct spage_info *spage;
> +    int i;
> +
> +    spage = page_to_spage(pg);
> +    if (order < SUPERPAGE_ORDER)
> +    {
> +        test_and_clear_bit(_SGT_enabled, &spage->type_info);
> +        return;
> +    }        

Why do you need this case? If this superpage is freshly allocated then it
was not previously SGT_enabled, and the flag should be already clear?

> +    if (order == SUPERPAGE_ORDER)
> +    {
> +        test_and_set_bit(_SGT_enabled, &spage->type_info);
> +        return;
> +    }

This case is completely redundant. The for loop would handle it fine.

> +    order -= SUPERPAGE_ORDER;
> +    for(i = 0; i < (1 << order); i++)
> +        test_and_set_bit(_SGT_enabled, &spage[i].type_info);
> +}

And why test_and_*() everywhere? You don't use the result of the test.

> +static void disable_superpage(
> +    struct page_info *pg,
> +    unsigned int order)
> +{
> +    struct spage_info *spage;
> +    int i;
> +
> +    spage = page_to_spage(pg);
> +    test_and_clear_bit(_SGT_enabled, &spage->type_info);

Here we are at the crux of the matter. If a guest frees a page you just
clear the SGT_enabled bit. So the superpage count_info is a completely
pointless creation which is checked absolutely nowhere. The pages can get
reused while stale superpage mappings hang around. Safety off.

> +    if (order > SUPERPAGE_ORDER)
> +    {
> +        order -= SUPERPAGE_ORDER;
> +        for(i = 1; i < (1 << order); i++)
> +        {
> +            BUG_ON((spage[i].type_info & SGT_count_mask) != 0);

Well, here you check the type count. The general count (count_info) would be
more appropriate. Either way, crashing Xen is very obviously unacceptable.

AFAICS, your attempt to be clever on the handling of type conflicts and
avoid ever needing to loop over all the pages in a superpage is so far a
failure. I don't see how you'll successfully avoid it for count_info (page
lifetime) handling, and in that case it may be simpler just to hold both
count_info *and* type_info counts on every page when super_page's associated
counts become non-zero.

I also noted you added another boot parameter. Just pick one name and stick
with it.

 -- Keir

> +            test_and_clear_bit(_SGT_enabled, &spage[i].type_info);
> +        }
> +    }   
> +}



> Dave McCracken
> Oracle Corp.