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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2015-10-08 at 16:16 +0100, George Dunlap wrote:
> On 08/10/15 13:52, Dario Faggioli wrote:

> > 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?
> 
Scheduling can't happen on the cpu, until later than the end of this
function, when, in cpupool_assign_cpu_locked(), we set to 1 its
corresponding bit in the target cpupool's cpu_valid mask.

In fact, scheduling events happening before that, would basically mean
that a cpu outside of any cpupool is somehow being considered for
scheduling, which, as said, would be a bug. In fact, I sent patches
back in July to cure occurrences of that behavior.

We've been discussing, basically about the same issue, with Jan in
here:
https://www.choon.net/forum/read.php?22,3817262,3817489

And I'll add the promised ASSERT() and comment, when sending v2 of that
patch. :-)

Make sense?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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