Re: [PATCH 2/7] xen: sched: fix locking for insert_vcpu() in credit1 and RTDS

[George Dunlap <george.dunlap@citrix.com>]

On 08/10/15 13:52, Dario Faggioli wrote:
> The insert_vcpu scheduler hook is called with an inconsistent
> locking strategy. In fact, it is sometimes invoked while
> holding the runqueue lock and sometimes when that is not the
> case.
> 
> For instance, in case of schedule_cpu_switch() the lock is
> acquired in generic code. On the other hand, in case of
> sched_move_domain(), locking is left as a responsibility
> of the schedulers implementing the hook.
> 
> This results in Credit1 and RTDS schedulers ending up (in
> case of sched_move_domain()) doing runqueue manipulation
> without holding any runqueue lock, which is a bug. (Credit2
> was doing the locking by itself already.)
> 
> The right thing is to defer locking to the specific schedulers,
> as it's them that know what, how and when it is best to lock
> (as in: runqueue locks, vs. private scheduler locks, vs. both,
> etc.).
> 
> This patch, therefore:
>  - removes any locking around insert_vcpu() from generic
>    code;
>  - add proper locking in the hook implementations, for
>    both Credit1 and RTDS.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Meng Xu <mengxu@cis.upenn.edu>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> ---
> Changes from v1 (within the other series):
>  * split the patch (wrt the original patch, in the original
>    series), and take care, in this one, only of insert_vcpu();
> ---
>  xen/common/sched_credit.c |    5 +++++
>  xen/common/sched_rt.c     |    3 +++
>  xen/common/schedule.c     |    6 ------
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 6f71e0d..fccb368 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -903,10 +903,15 @@ static void
>  csched_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
>  {
>      struct csched_vcpu *svc = vc->sched_priv;
> +    spinlock_t *lock;
> +
> +    lock = vcpu_schedule_lock_irq(vc);
>  
>      if ( !__vcpu_on_runq(svc) && vcpu_runnable(vc) && !vc->is_running )
>          __runq_insert(vc->processor, svc);
>  
> +    vcpu_schedule_unlock_irq(lock, vc);
> +
>      SCHED_STAT_CRANK(vcpu_insert);
>  }
>  
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 6a341b1..1086399 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -622,16 +622,19 @@ rt_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
>  {
>      struct rt_vcpu *svc = rt_vcpu(vc);
>      s_time_t now = NOW();
> +    spinlock_t *lock;
>  
>      /* not addlocate idle vcpu to dom vcpu list */
>      if ( is_idle_vcpu(vc) )
>          return;
>  
> +    lock = vcpu_schedule_lock_irq(vc);
>      if ( now >= svc->cur_deadline )
>          rt_update_deadline(now, svc);
>  
>      if ( !__vcpu_on_q(svc) && vcpu_runnable(vc) && !vc->is_running )
>          __runq_insert(ops, svc);
> +    vcpu_schedule_unlock_irq(lock, vc);
>  
>      /* add rt_vcpu svc to scheduler-specific vcpu list of the dom */
>      list_add_tail(&svc->sdom_elem, &svc->sdom->vcpu);
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index c5f640f..9aa209d 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1488,9 +1488,7 @@ void __init scheduler_init(void)
>  
>  int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>  {
> -    unsigned long flags;
>      struct vcpu *idle;
> -    spinlock_t *lock;
>      void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
>      struct scheduler *old_ops = per_cpu(scheduler, cpu);
>      struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
> @@ -1509,8 +1507,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>          return -ENOMEM;
>      }
>  
> -    lock = pcpu_schedule_lock_irqsave(cpu, &flags);
> -
>      SCHED_OP(old_ops, tick_suspend, cpu);
>      vpriv_old = idle->sched_priv;
>      idle->sched_priv = vpriv;
> @@ -1520,8 +1516,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
>      SCHED_OP(new_ops, tick_resume, cpu);
>      SCHED_OP(new_ops, insert_vcpu, idle);
>  
> -    pcpu_schedule_unlock_irqrestore(lock, flags, cpu);

It seems to me that the locking here wasn't to protect insert_vcpu, but
to prevent any scheduling events from happening on cpu until all the
expected infrastructure (ticks, idle vcpu, &c) were ready.  I can't
immediately convince myself that removing these is safe in that regard.
 Can you address this?

 -George