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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.14 at 15:16, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxx>
>>
>> --- 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.

Right, but this isn't the subject of the patch - the check got moved
from further down (i.e. even farther outside the locked region) up
here. Moving it into the locked region ought to probably be a
separate (potentially backportable) patch. Otoh this is a read, and
we don't really care for races here as is_dying would never get
cleared once it was set.

> 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?

Yes, just like the comment says.

Jan


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