[PATCH] slightly consolidate code in free_domheap_pages()

[PATCH] slightly consolidate code in free_domheap_pages()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2823 bytes --]

... 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;
+        }
+        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);




[-- Attachment #2: free-domheap-pages-consolidate.patch --]
[-- Type: text/plain, Size: 2870 bytes --]

slightly consolidate code in free_domheap_pages()

... 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;
+        }
+        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);

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

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

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

[Andrew Cooper]

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

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

[Jan Beulich]

>>> On 20.06.14 at 15:16, <andrew.cooper3@citrix.com> 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@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.

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

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

[Andrew Cooper]

On 20/06/14 15:23, Jan Beulich wrote:
>> 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
>

But it means that we will scrub all pages for all domains no matter how
it died, which is contrary to the statement in the comment which says
that we don't scrub a cleanly shut down domain.

>From what I can see, the only pages Xen won't scrub are pages handed
back via decrease reservation, but I don't see how that is relevant in
this context.

~Andrew

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

[Jan Beulich]

>>> On 20.06.14 at 16:35, <andrew.cooper3@citrix.com> wrote:
> On 20/06/14 15:23, Jan Beulich wrote:
>>> 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.
> 
> But it means that we will scrub all pages for all domains no matter how
> it died, which is contrary to the statement in the comment which says
> that we don't scrub a cleanly shut down domain.
> 
> From what I can see, the only pages Xen won't scrub are pages handed
> back via decrease reservation, but I don't see how that is relevant in
> this context.

Nothing like this is being said in that comment:

            /*
             * 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.
             */

("has died" doesn't [to me at least] exclude the clean shutdown case).

Jan

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

[Ian Campbell]

On Fri, 2014-06-20 at 13:40 +0100, Jan Beulich wrote:

> +        if ( likely(d) && likely(d != dom_cow) )

OOI is this more or less efficient than a single likely around the
entire thing?

> +        else
> +        {
> +            ASSERT(!d || !order);

Is this effectively replacing the ASSERT(order == 0) In the previous d
== dom_cow case?

If so, can d at this point ever be anything other than dom_cow or NULL?
I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
0)) would more clearly capture the intent of the test (with the spelling
out of the conditions being more important than the de morganing of the
expression).

Ian.

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

[Jan Beulich]

>>> On 24.06.14 at 12:04, <Ian.Campbell@eu.citrix.com> wrote:
> On Fri, 2014-06-20 at 13:40 +0100, Jan Beulich wrote:
> 
>> +        if ( likely(d) && likely(d != dom_cow) )
> 
> OOI is this more or less efficient than a single likely around the
> entire thing?

likely()/unlikely() around && or || expressions is never having the
intended effect unless these expressions can be guaranteed to
result in only a single branch instruction in the compiled code.
That's because branch likelihood needs to be determined for each
branch instruction individually (and e.g. likely(x && y) doesn't
necessarily mean likely(x) && likely(y), i.e. it may only be the
particular combination of the two that is likely).

>> +        else
>> +        {
>> +            ASSERT(!d || !order);
> 
> Is this effectively replacing the ASSERT(order == 0) In the previous d
> == dom_cow case?

Yes.

> If so, can d at this point ever be anything other than dom_cow or NULL?
> I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
> 0)) would more clearly capture the intent of the test (with the spelling
> out of the conditions being more important than the de morganing of the
> expression).

Indeed, d can only be NULL or dom_cow here (being in the else part
of the if() you quoted at the top). So an alternative might indeed be
ASSERT(d != dom_cow || !order), but that seems less desirable to
me as it opens up ways to pass the ASSERT() with d != NULL should
the if() condition ever get modified. I.e. I'd prefer the assertion to be
as restrictive as possible, getting relaxed only when in fact necessary.

Jan

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

[Ian Campbell]

On Tue, 2014-06-24 at 11:25 +0100, Jan Beulich wrote:
> >>> On 24.06.14 at 12:04, <Ian.Campbell@eu.citrix.com> wrote:
> > On Fri, 2014-06-20 at 13:40 +0100, Jan Beulich wrote:
> > 
> >> +        if ( likely(d) && likely(d != dom_cow) )
> > 
> > OOI is this more or less efficient than a single likely around the
> > entire thing?
> 
> likely()/unlikely() around && or || expressions is never having the
> intended effect unless these expressions can be guaranteed to
> result in only a single branch instruction in the compiled code.
> That's because branch likelihood needs to be determined for each
> branch instruction individually (and e.g. likely(x && y) doesn't
> necessarily mean likely(x) && likely(y), i.e. it may only be the
> particular combination of the two that is likely).

Make sense, thanks.

> >> +        else
> >> +        {
> >> +            ASSERT(!d || !order);
> > 
> > Is this effectively replacing the ASSERT(order == 0) In the previous d
> > == dom_cow case?
> 
> Yes.
> 
> > If so, can d at this point ever be anything other than dom_cow or NULL?
> > I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
> > 0)) would more clearly capture the intent of the test (with the spelling
> > out of the conditions being more important than the de morganing of the
> > expression).
> 
> Indeed, d can only be NULL or dom_cow here (being in the else part
> of the if() you quoted at the top). So an alternative might indeed be
> ASSERT(d != dom_cow || !order), but that seems less desirable to
> me as it opens up ways to pass the ASSERT() with d != NULL should
> the if() condition ever get modified. I.e. I'd prefer the assertion to be
> as restrictive as possible, getting relaxed only when in fact necessary.

Since the original if involves d == dom_cow but nothing to do with order
it seemed that the check was somehow specific to dom_cow's relationship
to higher order allocations.

I suppose the question is what relationship would a non-NULL d have to
the order of the allocation. i.e. if the if were changed to also
consider dom_foo why would we expect now that dom_foo had any order
requirements?

Ian.

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

[Jan Beulich]

>>> On 24.06.14 at 13:27, <Ian.Campbell@eu.citrix.com> wrote:
> On Tue, 2014-06-24 at 11:25 +0100, Jan Beulich wrote:
>> >>> On 24.06.14 at 12:04, <Ian.Campbell@eu.citrix.com> wrote:
>> > If so, can d at this point ever be anything other than dom_cow or NULL?
>> > I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
>> > 0)) would more clearly capture the intent of the test (with the spelling
>> > out of the conditions being more important than the de morganing of the
>> > expression).
>> 
>> Indeed, d can only be NULL or dom_cow here (being in the else part
>> of the if() you quoted at the top). So an alternative might indeed be
>> ASSERT(d != dom_cow || !order), but that seems less desirable to
>> me as it opens up ways to pass the ASSERT() with d != NULL should
>> the if() condition ever get modified. I.e. I'd prefer the assertion to be
>> as restrictive as possible, getting relaxed only when in fact necessary.
> 
> Since the original if involves d == dom_cow but nothing to do with order
> it seemed that the check was somehow specific to dom_cow's relationship
> to higher order allocations.
> 
> I suppose the question is what relationship would a non-NULL d have to
> the order of the allocation. i.e. if the if were changed to also
> consider dom_foo why would we expect now that dom_foo had any order
> requirements?

We won't know, but by having it the way it is now in the patch we're
on the safe side (nothing unintended will slip through), whereas if we
change to comparing against dom_cow a not sufficiently careful future
change may introduce an issue.

Jan

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

[Ian Campbell]

On Tue, 2014-06-24 at 12:53 +0100, Jan Beulich wrote:
> >>> On 24.06.14 at 13:27, <Ian.Campbell@eu.citrix.com> wrote:
> > On Tue, 2014-06-24 at 11:25 +0100, Jan Beulich wrote:
> >> >>> On 24.06.14 at 12:04, <Ian.Campbell@eu.citrix.com> wrote:
> >> > If so, can d at this point ever be anything other than dom_cow or NULL?
> >> > I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
> >> > 0)) would more clearly capture the intent of the test (with the spelling
> >> > out of the conditions being more important than the de morganing of the
> >> > expression).
> >> 
> >> Indeed, d can only be NULL or dom_cow here (being in the else part
> >> of the if() you quoted at the top). So an alternative might indeed be
> >> ASSERT(d != dom_cow || !order), but that seems less desirable to
> >> me as it opens up ways to pass the ASSERT() with d != NULL should
> >> the if() condition ever get modified. I.e. I'd prefer the assertion to be
> >> as restrictive as possible, getting relaxed only when in fact necessary.
> > 
> > Since the original if involves d == dom_cow but nothing to do with order
> > it seemed that the check was somehow specific to dom_cow's relationship
> > to higher order allocations.
> > 
> > I suppose the question is what relationship would a non-NULL d have to
> > the order of the allocation. i.e. if the if were changed to also
> > consider dom_foo why would we expect now that dom_foo had any order
> > requirements?
> 
> We won't know, but by having it the way it is now in the patch we're
> on the safe side (nothing unintended will slip through), whereas if we
> change to comparing against dom_cow a not sufficiently careful future
> change may introduce an issue.

That's true I . Could you add a comment though, since as it is the
current relationship to dom_cow and order is somewhat obscured.

ASSERT(d == NULL || ( d == dom_cow && !order ) ) is too much?

Ian.

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

[Jan Beulich]

>>> On 24.06.14 at 14:10, <Ian.Campbell@eu.citrix.com> wrote:
> On Tue, 2014-06-24 at 12:53 +0100, Jan Beulich wrote:
>> >>> On 24.06.14 at 13:27, <Ian.Campbell@eu.citrix.com> wrote:
>> > On Tue, 2014-06-24 at 11:25 +0100, Jan Beulich wrote:
>> >> >>> On 24.06.14 at 12:04, <Ian.Campbell@eu.citrix.com> wrote:
>> >> > If so, can d at this point ever be anything other than dom_cow or NULL?
>> >> > I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
>> >> > 0)) would more clearly capture the intent of the test (with the spelling
>> >> > out of the conditions being more important than the de morganing of the
>> >> > expression).
>> >> 
>> >> Indeed, d can only be NULL or dom_cow here (being in the else part
>> >> of the if() you quoted at the top). So an alternative might indeed be
>> >> ASSERT(d != dom_cow || !order), but that seems less desirable to
>> >> me as it opens up ways to pass the ASSERT() with d != NULL should
>> >> the if() condition ever get modified. I.e. I'd prefer the assertion to be
>> >> as restrictive as possible, getting relaxed only when in fact necessary.
>> > 
>> > Since the original if involves d == dom_cow but nothing to do with order
>> > it seemed that the check was somehow specific to dom_cow's relationship
>> > to higher order allocations.
>> > 
>> > I suppose the question is what relationship would a non-NULL d have to
>> > the order of the allocation. i.e. if the if were changed to also
>> > consider dom_foo why would we expect now that dom_foo had any order
>> > requirements?
>> 
>> We won't know, but by having it the way it is now in the patch we're
>> on the safe side (nothing unintended will slip through), whereas if we
>> change to comparing against dom_cow a not sufficiently careful future
>> change may introduce an issue.
> 
> That's true I . Could you add a comment though, since as it is the
> current relationship to dom_cow and order is somewhat obscured.

That I can do, ...

> ASSERT(d == NULL || ( d == dom_cow && !order ) ) is too much?

... but this looks too ugly to me (namely in the else path to an if
checking d against exactly these two values).

Jan

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

[Ian Campbell]

On Tue, 2014-06-24 at 13:25 +0100, Jan Beulich wrote:
> >>> On 24.06.14 at 14:10, <Ian.Campbell@eu.citrix.com> wrote:
> > On Tue, 2014-06-24 at 12:53 +0100, Jan Beulich wrote:
> >> >>> On 24.06.14 at 13:27, <Ian.Campbell@eu.citrix.com> wrote:
> >> > On Tue, 2014-06-24 at 11:25 +0100, Jan Beulich wrote:
> >> >> >>> On 24.06.14 at 12:04, <Ian.Campbell@eu.citrix.com> wrote:
> >> >> > If so, can d at this point ever be anything other than dom_cow or NULL?
> >> >> > I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
> >> >> > 0)) would more clearly capture the intent of the test (with the spelling
> >> >> > out of the conditions being more important than the de morganing of the
> >> >> > expression).
> >> >> 
> >> >> Indeed, d can only be NULL or dom_cow here (being in the else part
> >> >> of the if() you quoted at the top). So an alternative might indeed be
> >> >> ASSERT(d != dom_cow || !order), but that seems less desirable to
> >> >> me as it opens up ways to pass the ASSERT() with d != NULL should
> >> >> the if() condition ever get modified. I.e. I'd prefer the assertion to be
> >> >> as restrictive as possible, getting relaxed only when in fact necessary.
> >> > 
> >> > Since the original if involves d == dom_cow but nothing to do with order
> >> > it seemed that the check was somehow specific to dom_cow's relationship
> >> > to higher order allocations.
> >> > 
> >> > I suppose the question is what relationship would a non-NULL d have to
> >> > the order of the allocation. i.e. if the if were changed to also
> >> > consider dom_foo why would we expect now that dom_foo had any order
> >> > requirements?
> >> 
> >> We won't know, but by having it the way it is now in the patch we're
> >> on the safe side (nothing unintended will slip through), whereas if we
> >> change to comparing against dom_cow a not sufficiently careful future
> >> change may introduce an issue.
> > 
> > That's true I . Could you add a comment though, since as it is the
> > current relationship to dom_cow and order is somewhat obscured.
> 
> That I can do,

Thanks.

>  ...
> 
> > ASSERT(d == NULL || ( d == dom_cow && !order ) ) is too much?
> 
> ... but this looks too ugly to me (namely in the else path to an if
> checking d against exactly these two values).

There's three values in the assert, the third of which is constrained by
the other two, which is rather the point. Anyway, a comment will
suffice.

Ian.

[PATCH v2] slightly consolidate code in free_domheap_pages()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]

... to combine the three scrubbing paths into a single one.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add a comment to an ASSERT() deemed unclear/non-obvious otherwise.

--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1724,47 +1724,52 @@ 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;
+        }
+        else
+        {
+            /*
+             * All we need to check is that on dom_cow only order-0 chunks
+             * make it here. Due to the if() above, the only two possible
+             * cases right now are d == NULL and d == dom_cow. To protect
+             * against relaxation of that if() condition without updating the
+             * check here, don't check d != dom_cow for now.
+             */
+            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);




[-- Attachment #2: free-domheap-pages-consolidate.patch --]
[-- Type: text/plain, Size: 3354 bytes --]

slightly consolidate code in free_domheap_pages()

... to combine the three scrubbing paths into a single one.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add a comment to an ASSERT() deemed unclear/non-obvious otherwise.

--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1724,47 +1724,52 @@ 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;
+        }
+        else
+        {
+            /*
+             * All we need to check is that on dom_cow only order-0 chunks
+             * make it here. Due to the if() above, the only two possible
+             * cases right now are d == NULL and d == dom_cow. To protect
+             * against relaxation of that if() condition without updating the
+             * check here, don't check d != dom_cow for now.
+             */
+            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);

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

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

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

[Ian Campbell]

On Tue, 2014-06-24 at 13:36 +0100, Jan Beulich wrote:
> ... to combine the three scrubbing paths into a single one.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

> ---
> v2: Add a comment to an ASSERT() deemed unclear/non-obvious otherwise.

Thanks.

Acked-by: Ian Campbell <ian.campbell@citrix.com>