Re: [PATCH v4 1/6] xen: sched: fix locking of remove_vcpu() in credit1

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

On 04/11/15 17:17, Dario Faggioli wrote:
> In fact, csched_vcpu_remove() (i.e., the credit1
> implementation of remove_vcpu()) manipulates runqueues,
> so holding the runqueue lock is necessary.
> 
> However, the vCPU just can't be on the runqueue, when
> the function is called. We can therefore ASSERT() that,
> and avoid doing any runqueue manipulations (rather than
> adding the runqueue locking around it).
> 
> Also, while there, *_lock_irq() (for the private lock) is
> enough, there is no need to *_lock_irqsave().
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> ---
> Changes from v3:
>  * instead of adding locking, get rid of __runq_remove(),
>    and add an ASSERT() about vCPU not being in runq already,
>    as suggested during review.
> 
> Changes from the other series:
>  * split the patch (wrt the original patch, in the original
>    series), and take care, in this one, only of remove_vcpu();
>  * removed pointless parentheses.
> ---
> The fact that vCPU can't be in runqueue when calling remove_vcpu() is true for
> other schedulers as well. In them, though, there isn't any race condition to
> fix. Therefore, taking care of the other schedulers will happen in a followup
> series.
> ---
>  xen/common/sched_credit.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 1b30e67..6dfcff6 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -934,28 +934,25 @@ csched_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
>      struct csched_private *prv = CSCHED_PRIV(ops);
>      struct csched_vcpu * const svc = CSCHED_VCPU(vc);
>      struct csched_dom * const sdom = svc->sdom;
> -    unsigned long flags;
>  
>      SCHED_STAT_CRANK(vcpu_remove);
>  
> +    ASSERT(!__vcpu_on_runq(svc));
> +
>      if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
>      {
>          SCHED_STAT_CRANK(vcpu_unpark);
>          vcpu_unpause(svc->vcpu);
>      }
>  
> -    if ( __vcpu_on_runq(svc) )
> -        __runq_remove(svc);
> -
> -    spin_lock_irqsave(&(prv->lock), flags);
> +    spin_lock_irq(&prv->lock);
>  
>      if ( !list_empty(&svc->active_vcpu_elem) )
>          __csched_vcpu_acct_stop_locked(prv, svc);
>  
> -    spin_unlock_irqrestore(&(prv->lock), flags);
> +    spin_unlock_irq(&prv->lock);
>  
>      BUG_ON( sdom == NULL );
> -    BUG_ON( !list_empty(&svc->runq_elem) );
>  }
>  
>  static void
>