Re: [PATCH] slightly consolidate code in free_domheap_pages()

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3466 bytes --]

On 20/06/14 13:40, Jan Beulich wrote:
> ... to combine the three scrubbing paths into a single one.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1724,47 +1724,45 @@ void free_domheap_pages(struct page_info
>  
>          spin_unlock_recursive(&d->page_alloc_lock);
>      }
> -    else if ( likely(d != NULL) && likely(d != dom_cow) )
> +    else
>      {
> -        /* NB. May recursively lock from relinquish_memory(). */
> -        spin_lock_recursive(&d->page_alloc_lock);
> +        bool_t scrub;
>  
> -        for ( i = 0; i < (1 << order); i++ )
> +        if ( likely(d) && likely(d != dom_cow) )
>          {
> -            BUG_ON((pg[i].u.inuse.type_info & PGT_count_mask) != 0);
> -            page_list_del2(&pg[i], &d->page_list, &d->arch.relmem_list);
> -        }
> +            /* NB. May recursively lock from relinquish_memory(). */
> +            spin_lock_recursive(&d->page_alloc_lock);
>  
> -        drop_dom_ref = !domain_adjust_tot_pages(d, -(1 << order));
> -
> -        spin_unlock_recursive(&d->page_alloc_lock);
> +            for ( i = 0; i < (1 << order); i++ )
> +            {
> +                BUG_ON((pg[i].u.inuse.type_info & PGT_count_mask) != 0);
> +                page_list_del2(&pg[i], &d->page_list, &d->arch.relmem_list);
> +            }
> +
> +            drop_dom_ref = !domain_adjust_tot_pages(d, -(1 << order));
> +
> +            spin_unlock_recursive(&d->page_alloc_lock);
> +
> +            /*
> +             * Normally we expect a domain to clear pages before freeing them,
> +             * if it cares about the secrecy of their contents. However, after
> +             * a domain has died we assume responsibility for erasure.
> +             */
> +            scrub = !!d->is_dying;

d->is_dying is technically protected by d->page_alloc_lock, and one
extra boolean read isn't going to extend the critical region too much.

Unrelated to the content of the patch, I can't see a codepath where we
would relinquish domain memory from a clean shutdown without setting
d->is_dying.  Does this mean that we are even scrubbing pages from
cleanly shut down domains?

~Andrew

> +        }
> +        else
> +        {
> +            ASSERT(!d || !order);
> +            drop_dom_ref = 0;
> +            scrub = 1;
> +        }
>  
> -        /*
> -         * Normally we expect a domain to clear pages before freeing them, if 
> -         * it cares about the secrecy of their contents. However, after a 
> -         * domain has died we assume responsibility for erasure.
> -         */
> -        if ( unlikely(d->is_dying) )
> +        if ( unlikely(scrub) )
>              for ( i = 0; i < (1 << order); i++ )
>                  scrub_one_page(&pg[i]);
>  
>          free_heap_pages(pg, order);
>      }
> -    else if ( unlikely(d == dom_cow) )
> -    {
> -        ASSERT(order == 0); 
> -        scrub_one_page(pg);
> -        free_heap_pages(pg, 0);
> -        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);
> -        drop_dom_ref = 0;
> -    }
>  
>      if ( drop_dom_ref )
>          put_domain(d);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 4364 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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