Re: [Xen-devel] [PATCH] xen: credit1: fix a race when picking initial pCPU for a vCPU

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 12/08/16 05:07, Dario Faggioli wrote:
> In the Credit1 hunk of 9f358ddd69463 ("xen: Have
> schedulers revise initial placement") csched_cpu_pick()
> is called without taking the runqueue lock of the
> (temporary) pCPU that the vCPU has been assigned to
> (e.g., in XEN_DOMCTL_max_vcpus).
> 
> However, although 'hidden' in the IS_RUNQ_IDLE() macro,
> that function does access the runq (for doing load
> balancing calculations), and hence the appropriate lock
> must be taken.
> 
> Races have been observed, in the form of IS_RUNQ_IDLE()
> falling over LIST_POISON.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

It might be nice if we could add an ASSERT() that the appropriate
runqueue was locked, to make sure we don't get caught out again like
this in the future, but I think that would probably require turning it
into a static inline (which probably wouldn't be so bad anyway).

But in any case:

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Let me know if you want me to check this in as-is or if you think you
might send a follow-up patch adding an ASSERT.

 -George

> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> ---
>  xen/common/sched_credit.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 220ff0d..b6f82e8 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -998,9 +998,13 @@ csched_vcpu_insert(const struct scheduler *ops, struct 
> vcpu *vc)
>  
>      BUG_ON( is_idle_vcpu(vc) );
>  
> -    /* This is safe because vc isn't yet being scheduled */
> +    /* csched_cpu_pick() looks in vc->processor's runq, so we need the lock. 
> */
> +    lock = vcpu_schedule_lock_irq(vc);
> +
>      vc->processor = csched_cpu_pick(ops, vc);
>  
> +    spin_unlock_irq(lock);
> +
>      lock = vcpu_schedule_lock_irq(vc);
>  
>      if ( !__vcpu_on_runq(svc) && vcpu_runnable(vc) && !vc->is_running )
> 


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