From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] slightly consolidate code in free_domheap_pages() Date: Fri, 20 Jun 2014 14:16:35 +0100 Message-ID: <53A43433.3070900@citrix.com> References: <53A447C9020000780001BF5F@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5410898764988316794==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WxygK-0002DP-VV for xen-devel@lists.xenproject.org; Fri, 20 Jun 2014 13:16:41 +0000 In-Reply-To: <53A447C9020000780001BF5F@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Ian Campbell , Ian Jackson , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org --===============5410898764988316794== Content-Type: multipart/alternative; boundary="------------040809090309090605060208" --------------040809090309090605060208 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 20/06/14 13:40, Jan Beulich wrote: > ... to combine the three scrubbing paths into a single one. > > Signed-off-by: Jan Beulich > > --- 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 --------------040809090309090605060208 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
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

--------------040809090309090605060208-- --===============5410898764988316794== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5410898764988316794==--