From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/memshr: fix preemption in relinquish_shared_pages() Date: Tue, 17 Dec 2013 10:43:39 +0000 Message-ID: <52B02ADB.2050002@citrix.com> References: <52B0224B020000780010E067@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6530096907994924688==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vss7q-0004vo-TV for xen-devel@lists.xenproject.org; Tue, 17 Dec 2013 10:43:43 +0000 In-Reply-To: <52B0224B020000780010E067@nat28.tlf.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 Cc: xen-devel , Tim Deegan , Andres Lagar-Cavilla List-Id: xen-devel@lists.xenproject.org --===============6530096907994924688== Content-Type: multipart/alternative; boundary="------------000609090209000100080401" --------------000609090209000100080401 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 17/12/13 09:07, Jan Beulich wrote: > For one, should hypercall_preempt_check() return false the first time > it gets called, it would never have got called again (because count, > being checked for equality, didn't get reset to zero). > > And then, if there were a huge range of unshared pages, with count not > getting incremented at all in that case there would also not be any > preemption. > > Fix this by using a biased increment (ratio 1:16 for unshared vs shared > pages), and check for certain bits in count to be clear rather than for > a specific value (the alternative would be to do a greater-or-equal > comparison and flush the value to zero in case of a "false" return from > hypercall_preempt_check()). > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -1290,11 +1290,13 @@ int relinquish_shared_pages(struct domai > set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K, > p2m_invalid, p2m_access_rwx); > ASSERT(set_rc != 0); > - count++; > + count += 0x10; > } > + else > + ++count; > > - /* Preempt every 2MiB. Arbitrary */ > - if ( (count == 512) && hypercall_preempt_check() ) > + /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */ > + if ( !(count & 0x1ff0) && hypercall_preempt_check() ) > { > p2m->next_shared_gfn_to_relinquish = gfn + 1; > rc = -EAGAIN; > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------000609090209000100080401 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 17/12/13 09:07, Jan Beulich wrote:
For one, should hypercall_preempt_check() return false the first time
it gets called, it would never have got called again (because count,
being checked for equality, didn't get reset to zero).

And then, if there were a huge range of unshared pages, with count not
getting incremented at all in that case there would also not be any
preemption.

Fix this by using a biased increment (ratio 1:16 for unshared vs shared
pages), and check for certain bits in count to be clear rather than for
a specific value (the alternative would be to do a greater-or-equal
comparison and flush the value to zero in case of a "false" return from
hypercall_preempt_check()).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1290,11 +1290,13 @@ int relinquish_shared_pages(struct domai
             set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
                                     p2m_invalid, p2m_access_rwx);
             ASSERT(set_rc != 0);
-            count++;
+            count += 0x10;
         }
+        else
+            ++count;
 
-        /* Preempt every 2MiB. Arbitrary */
-        if ( (count == 512) && hypercall_preempt_check() )
+        /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
+        if ( !(count & 0x1ff0) && hypercall_preempt_check() )
         {
             p2m->next_shared_gfn_to_relinquish = gfn + 1;
             rc = -EAGAIN;






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

--------------000609090209000100080401-- --===============6530096907994924688== 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 --===============6530096907994924688==--