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

[Meng Xu <xumengpanda@xxxxxxxxx>]

Hi Dario,

2015-10-29 19:04 GMT-04:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
>
> The insert_vcpu() hook is handled with inconsistent locking.
> In fact, schedule_cpu_switch() calls the hook with runqueue
> lock held, while sched_move_domain() relies on the hook
> implementations to take the lock themselves (and, since that
> is not done in Credit1 and RTDS, such operation is not safe
> in those cases).
>
> This is fixed as follows:
>  - take the lock in the hook implementations, in specific
>    schedulers' code;
>  - avoid calling insert_vcpu(), for the idle vCPU, in
>    schedule_cpu_switch(). In fact, idle vCPUs are set to run
>    immediately, and the various schedulers won't insert them
>    in their runqueues anyway, even when explicitly asked to.
>
> While there, still in schedule_cpu_switch(), locking with
> _irq() is enough (there's no need to do *_irqsave()).
>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> ---
> Cahnges from v2:
>  * locking in schedule_cpu_switch() is left in place (but
>    turned to just _irq(), instead than *_irqsave());
>  * call to insert_vcpu() in schedule_cpu_switch() is
>    removed in this patch (rather than later in the series).
>
> Changes from v1 (of this series):
>  * in Credit1, the lock wants to be an _irqsave() one. If
>    not, the ASSERT() in _spin_lock_irq() will trigger when
>    the hook is called, during boot, from sched_init_vcpu();
>  * reprhased the changelog (to be less verbose);
>  * add a few words, in the changelog, about why it is safe
>    to get rid of the locking in schedule_cpu_switch(). Proper
>    commentary and ASSERT()-s about that will come in another
>    patch.
>
> Changes from the other series:
>  * split the patch (wrt the original patch, in the original
>    series), and take care, in this one, only of insert_vcpu();
> ---
>  xen/common/sched_credit.c |    6 ++++++
>  xen/common/sched_rt.c     |    3 +++
>  xen/common/schedule.c     |    6 ++----
>  3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 6f71e0d..e16bd3a 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -903,10 +903,16 @@ static void
>  csched_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
>  {
>      struct csched_vcpu *svc = vc->sched_priv;
> +    spinlock_t *lock;
> +    unsigned long flags;
> +
> +    lock = vcpu_schedule_lock_irqsave(vc, &flags);


This is inconsistent with the commit log.
I also checked the

branch rel/sched/fix-vcpu-ins-rem-v2 in your repo., it is the following code:

lock = vcpu_schedule_lock_irq(vc, &flags);

I guess maybe you forgot to change it in this commit but change it the
following commit?


Besides this mistake, this patch looks good to me.

Thanks,

Meng

-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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