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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 18/08/16 11:00, 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). Two scenarios are possible:
>  1) we are on cpu X, and IS_RUNQ_IDLE() peeks at cpu's
>     X own runq;
>  2) we are on cpu X, but IS_RUNQ_IDLE() peeks at some
>     other cpu's runq.
> 
> Scenario 2) absolutely requies that the appropriate
> runq lock is taken. Scenario 1) works even without
> taking the cpu's own runq lock, and this is important
> for the case when _csched_pick_cpu() is called from
> csched_vcpu_acct() which in turn is called by
> csched_tick().


> 
> Races have been observed and reported (by both XenServer
> own testing and OSSTest [1]), in the form of
> IS_RUNQ_IDLE() falling over LIST_POISON, because we're
> not currently holding the proper lock, in
> csched_vcpu_insert(), when scenario 1) occurs.
> 
> Since this is all very tricky, in addition to fix things,
> add both an ASSERT() and a comment in IS_RUNQ_IDLE() (which
> is also becoming static inline instead of macro).
> 
> [1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg02144.html
> 
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> ---
> Changes from v1:
>  - macro IS_RUNQ_IDLE() to static inline is_runq_idle(), as suggested
>    during review;
>  - add an ASSERT() and a comment, as suggested during review;
>  - take into account what's described in the changelog as "scenario 1)",
>    which wasn't being considered in v1.
> ---
>  xen/common/sched_credit.c |   38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 220ff0d..daace81 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -84,9 +84,6 @@
>  #define CSCHED_VCPU(_vcpu)  ((struct csched_vcpu *) (_vcpu)->sched_priv)
>  #define CSCHED_DOM(_dom)    ((struct csched_dom *) (_dom)->sched_priv)
>  #define RUNQ(_cpu)          (&(CSCHED_PCPU(_cpu)->runq))
> -/* Is the first element of _cpu's runq its idle vcpu? */
> -#define IS_RUNQ_IDLE(_cpu)  (list_empty(RUNQ(_cpu)) || \
> -                             
> is_idle_vcpu(__runq_elem(RUNQ(_cpu)->next)->vcpu))
>  
>  
>  /*
> @@ -248,6 +245,33 @@ __runq_elem(struct list_head *elem)
>      return list_entry(elem, struct csched_vcpu, runq_elem);
>  }
>  
> +/* Is the first element of cpu's runq (if any) cpu's idle vcpu? */
> +static inline bool_t is_runq_idle(unsigned int cpu)
> +{
> +    /*
> +     * If we are on cpu, and we are peeking at our own runq while cpu itself
> +     * is not idle, that's fine even if we don't hold the runq lock. In fact,
> +     * the fact that there is a (non idle!) vcpu running means that at least
> +     * the idle vcpu is in the runq. And since only cpu itself (via work
> +     * stealing) can add stuff to the runq, and no other cpu will ever steal
> +     * our idle vcpu, that maks the runq manipulations done below safe, even
> +     * without locks.

Thanks for investigating this and figuring out why the lockless access
hasn't caused a problem before.  But relying on this behavior going
forward doesn't really seem like a great idea if we can avoid it.

We can't grab the pcpu scheduler lock in csched_tick(), or in the whole
of csched_vcpu_acct() because we grab the private lock in
__csched_vcpu_acct_start() (and that violates the locking order).  But
is there a reason we can't grab the pcpu lock just around the call to
_csched_cpu_pick?

If not, we would then need to put a comment in the runq struct listing
the restrictions on access: namely, that nothing can be inserted from
other pcpus; and ideally a wrapper for all list modification operations
to ASSERT() that we're on the right pcpu.

 -George


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