Re: [PATCH] fix locking in cpu_disable_scheduler()

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3387 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 4277 bytes --]

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

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