Re: [PATCH v2 04/11] xen: sched: close potential races when switching scheduler to CPUs

[Dario Faggioli <dario.faggioli@citrix.com>]

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

On Thu, 2016-04-07 at 15:54 +0100, George Dunlap wrote:
> On 06/04/16 18:23, Dario Faggioli wrote:
> > 
> > This patch, therefore, introduces a new hook in the scheduler
> > interface, called switch_sched, meant at being used when
> > switching scheduler on a CPU, and implements it for the
> > various schedulers (that needs it: i.e., all except ARINC653),
> > so that things are done in the proper order and under the
> > protection of the best suited (set of) lock(s). It is
> > necessary to add the hook (as compared to keep doing things
> > in generic code), because different schedulers may have
> >  different locking schemes.
> > 
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Hey Dario! Everything here looks good, except for one thing: the
> scheduler lock for arinc653 scheduler. :-)  
>
Bah! :-/

> What happens now if you
> assign a cpu to credit2, and then assign it to arinc653?  Since arinc
> doesn't implement the switch_sched() functionality, the per-cpu
> scheduler lock will still point to the credit2 lock, won't it?
> 
So, to assign a cpu to a pool you have to first remove it from the pool
it's currently in. And removing a cpu from its pool, means re-assigning 
it to the default pool (pool0), which uses as scheduler whatever we
chose at boot. (Then, of course, vcpus of domains in the default pool
still won't run there, because its corresponding bit in
pool0->cpu_valid mask is 0.)

So, if you move a cpu from pool0 to an ARINC pool, indeed the lock
would keep pointing to pool0's runqueue lock (which will be one of the
Credit2's runqueue locks, if the default scheduler is Credit2).

And that is the case even if you move a cpu from some non-default pool
to an ARINC pool, because you always have to remove the cpu from where
it is first, which automatically puts it back in the default pool.

This is relevant because...

> Which will *work*, although it will add unnecessary contention to the
> credit2 lock;  until the lock goes away, at which point
> vcpu_schedule_lock*() will essentially be using a wild pointer.
> 
...since we're talking about the lock of the default scheduler, it'll
never go away.

But indeed I agree that this just adds unnecessary contention which, in
case of Credit2 or RTDS, where the lock is shared between pcpus, is
something we certainly don't want (good catch!).

I'll add a very simple implementation of switch_sched to ARINC, which
basically puts back sd->schedule_lock to sd->_lock, and also does the
basic operations for actually switching scheduler, which before this
patch were done in schedule_cpu_switch(), and hence are also needed in
there.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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