Re: [Xen-devel] [PATCH v2 1/3] xen: delay page scrubbing to allocation path

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.06.14 at 15:39, <lliubbo@xxxxxxxxx> wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -711,6 +711,12 @@ static struct page_info *alloc_heap_pages(
>  
>      for ( i = 0; i < (1 << order); i++ )
>      {
> +        if ( test_bit(_PGC_need_scrub, &pg[i].count_info) )
> +        {
> +            scrub_one_page(&pg[i]);
> +            pg[i].count_info &= ~PGC_need_scrub;
> +        }
> +

heap_lock is still being held here - scrubbing should be done after it
was dropped (or else you re-introduce the same latency problem to
other paths now needing to wait for the scrubbing to complete).

> @@ -876,6 +882,15 @@ static void free_heap_pages(
>          midsize_alloc_zone_pages = max(
>              midsize_alloc_zone_pages, total_avail_pages / 
> MIDSIZE_ALLOC_FRAC);
>  
> +    if ( need_scrub )
> +    {
> +        if ( !tainted )
> +        {
> +            for ( i = 0; i < (1 << order); i++ )
> +                pg[i].count_info |= PGC_need_scrub;
> +        }
> +    }

Two if()s like these should be folded into one.

> @@ -889,6 +904,17 @@ static void free_heap_pages(
>                   (PFN_ORDER(pg-mask) != order) ||
>                   (phys_to_nid(page_to_maddr(pg-mask)) != node) )
>                  break;
> +            /* If we need scrub, only merge with PGC_need_scrub pages */
> +            if ( need_scrub )
> +            {
> +                if ( !test_bit(_PGC_need_scrub, &(pg-mask)->count_info) )
> +                    break;
> +            }
> +            else
> +            {
> +                if ( test_bit(_PGC_need_scrub, &(pg-mask)->count_info) )
> +                    break;
> +            }

You're setting PGC_need_scrub on each 4k page anyway (which
is debatable), hence there's no need to look at the passed in
need_scrub flag here: Just check whether both chunks have the
flag set the same. Same below.

> @@ -1535,7 +1571,7 @@ void free_xenheap_pages(void *v, unsigned int order)
>  
>      memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
>  
> -    free_heap_pages(virt_to_page(v), order);
> +    free_heap_pages(virt_to_page(v), order, 1);

Why?

> @@ -1588,11 +1624,10 @@ void free_xenheap_pages(void *v, unsigned int order)
>  
>      for ( i = 0; i < (1u << order); i++ )
>      {
> -        scrub_one_page(&pg[i]);
>          pg[i].count_info &= ~PGC_xen_heap;
>      }
>  
> -    free_heap_pages(pg, order);
> +    free_heap_pages(pg, order, 1);

The flags needs to be 1 here, but I don't see why you also pass 1 in
the other free_xenheap_pages() incarnation above.

> @@ -1745,24 +1780,20 @@ void free_domheap_pages(struct page_info *pg, 
> unsigned int order)
>           * domain has died we assume responsibility for erasure.
>           */
>          if ( unlikely(d->is_dying) )
> -            for ( i = 0; i < (1 << order); i++ )
> -                scrub_one_page(&pg[i]);
> -
> -        free_heap_pages(pg, order);
> +            free_heap_pages(pg, order, 1);
> +        else
> +            free_heap_pages(pg, order, 0);
>      }
>      else if ( unlikely(d == dom_cow) )
>      {
>          ASSERT(order == 0); 
> -        scrub_one_page(pg);
> -        free_heap_pages(pg, 0);
> +        free_heap_pages(pg, 0, 1);
>          drop_dom_ref = 0;
>      }
>      else
>      {
>          /* Freeing anonymous domain-heap pages. */
> -        for ( i = 0; i < (1 << order); i++ )
> -            scrub_one_page(&pg[i]);
> -        free_heap_pages(pg, order);
> +        free_heap_pages(pg, order, 1);
>          drop_dom_ref = 0;
>      }
>  

This hunk is patching no longer existing code (see commit daa4b800
"slightly consolidate code in free_domheap_pages()").

Jan


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