From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] fix locking in cpu_disable_scheduler() Date: Mon, 28 Oct 2013 16:24:14 +0000 Message-ID: <526E8FAE.9030205@citrix.com> References: <526E994E02000078000FD571@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6534558184177519786==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vapc2-00039u-AP for xen-devel@lists.xenproject.org; Mon, 28 Oct 2013 16:24:18 +0000 In-Reply-To: <526E994E02000078000FD571@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: George Dunlap , xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============6534558184177519786== Content-Type: multipart/alternative; boundary="------------050209060709050407030600" --------------050209060709050407030600 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 28/10/13 16:05, Jan Beulich wrote: > So commit eedd6039 ("scheduler: adjust internal locking interface") > uncovered - by now using proper spin lock constructs - a bug after all: > When bringing down a CPU, cpu_disable_scheduler() gets called with > interrupts disabled, and hence the use of vcpu_schedule_lock_irq() was > never really correct (i.e. the caller ended up with interrupts enabled > despite having disabled them explicitly). > > Fixing this however surfaced another problem: The call path > vcpu_migrate() -> evtchn_move_pirqs() wants to acquire the event lock, > which however is a non-IRQ-safe once, and hence check_lock() doesn't > like this lock to be acquired when interrupts are already off. As we're > in stop-machine context here, getting things wrong wrt interrupt state > management during lock acquire/release is out of question though, so > the simple solution to this appears to be to just suppress spin lock > debugging for the period of time while the stop machine callback gets > run. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper One comment however. With the unlock_irqrestore functions, having the cpu and flags parameters reversed wrt the lock_irqsave looks funny. As this new interface is still very new, would it be worth moving the flags parameter to the end of unlock_irqrestore() for consistency ? ~Andrew > > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -601,7 +601,8 @@ int cpu_disable_scheduler(unsigned int c > { > for_each_vcpu ( d, v ) > { > - spinlock_t *lock = vcpu_schedule_lock_irq(v); > + unsigned long flags; > + spinlock_t *lock = vcpu_schedule_lock_irqsave(v, &flags); > > cpumask_and(&online_affinity, v->cpu_affinity, c->cpu_valid); > if ( cpumask_empty(&online_affinity) && > @@ -622,14 +623,12 @@ int cpu_disable_scheduler(unsigned int c > if ( v->processor == cpu ) > { > set_bit(_VPF_migrating, &v->pause_flags); > - vcpu_schedule_unlock_irq(lock, v); > + vcpu_schedule_unlock_irqrestore(lock, flags, v); > vcpu_sleep_nosync(v); > vcpu_migrate(v); > } > else > - { > - vcpu_schedule_unlock_irq(lock, v); > - } > + vcpu_schedule_unlock_irqrestore(lock, flags, v); > > /* > * A vcpu active in the hypervisor will not be migratable. > --- a/xen/common/stop_machine.c > +++ b/xen/common/stop_machine.c > @@ -110,6 +110,7 @@ int stop_machine_run(int (*fn)(void *), > local_irq_disable(); > stopmachine_set_state(STOPMACHINE_DISABLE_IRQ); > stopmachine_wait_state(); > + spin_debug_disable(); > > stopmachine_set_state(STOPMACHINE_INVOKE); > if ( (cpu == smp_processor_id()) || (cpu == NR_CPUS) ) > @@ -117,6 +118,7 @@ int stop_machine_run(int (*fn)(void *), > stopmachine_wait_state(); > ret = stopmachine_data.fn_result; > > + spin_debug_enable(); > stopmachine_set_state(STOPMACHINE_EXIT); > stopmachine_wait_state(); > local_irq_enable(); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------050209060709050407030600 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 28/10/13 16:05, Jan Beulich wrote:
So commit eedd6039 ("scheduler: adjust internal locking interface")
uncovered - by now using proper spin lock constructs - a bug after all:
When bringing down a CPU, cpu_disable_scheduler() gets called with
interrupts disabled, and hence the use of vcpu_schedule_lock_irq() was
never really correct (i.e. the caller ended up with interrupts enabled
despite having disabled them explicitly).

Fixing this however surfaced another problem: The call path
vcpu_migrate() -> evtchn_move_pirqs() wants to acquire the event lock,
which however is a non-IRQ-safe once, and hence check_lock() doesn't
like this lock to be acquired when interrupts are already off. As we're
in stop-machine context here, getting things wrong wrt interrupt state
management during lock acquire/release is out of question though, so
the simple solution to this appears to be to just suppress spin lock
debugging for the period of time while the stop machine callback gets
run.

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

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

One comment however.  With the unlock_irqrestore functions, having the cpu and flags parameters reversed wrt the lock_irqsave looks funny.  As this new interface is still very new, would it be worth moving the flags parameter to the end of unlock_irqrestore() for consistency ?

~Andrew


--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -601,7 +601,8 @@ int cpu_disable_scheduler(unsigned int c
     {
         for_each_vcpu ( d, v )
         {
-            spinlock_t *lock = vcpu_schedule_lock_irq(v);
+            unsigned long flags;
+            spinlock_t *lock = vcpu_schedule_lock_irqsave(v, &flags);
 
             cpumask_and(&online_affinity, v->cpu_affinity, c->cpu_valid);
             if ( cpumask_empty(&online_affinity) &&
@@ -622,14 +623,12 @@ int cpu_disable_scheduler(unsigned int c
             if ( v->processor == cpu )
             {
                 set_bit(_VPF_migrating, &v->pause_flags);
-                vcpu_schedule_unlock_irq(lock, v);
+                vcpu_schedule_unlock_irqrestore(lock, flags, v);
                 vcpu_sleep_nosync(v);
                 vcpu_migrate(v);
             }
             else
-            {
-                vcpu_schedule_unlock_irq(lock, v);
-            }
+                vcpu_schedule_unlock_irqrestore(lock, flags, v);
 
             /*
              * A vcpu active in the hypervisor will not be migratable.
--- a/xen/common/stop_machine.c
+++ b/xen/common/stop_machine.c
@@ -110,6 +110,7 @@ int stop_machine_run(int (*fn)(void *), 
     local_irq_disable();
     stopmachine_set_state(STOPMACHINE_DISABLE_IRQ);
     stopmachine_wait_state();
+    spin_debug_disable();
 
     stopmachine_set_state(STOPMACHINE_INVOKE);
     if ( (cpu == smp_processor_id()) || (cpu == NR_CPUS) )
@@ -117,6 +118,7 @@ int stop_machine_run(int (*fn)(void *), 
     stopmachine_wait_state();
     ret = stopmachine_data.fn_result;
 
+    spin_debug_enable();
     stopmachine_set_state(STOPMACHINE_EXIT);
     stopmachine_wait_state();
     local_irq_enable();






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

--------------050209060709050407030600-- --===============6534558184177519786== 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 --===============6534558184177519786==--