Re: [Xen-devel] [PATCH 3/9] xen: sched: make locking for {insert, remove}_vcpu consistent

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.09.15 at 18:55, <dario.faggioli@xxxxxxxxxx> wrote:
> In case of credit1, remove_vcpu() handling needs some
> fixing remove_vcpu() too, i.e.:
>  - it manipulates runqueues, so the runqueue lock must
>    be acquired;

Is this really a fix needed only as a result of the insert side
changes? If not, since the insert side looks more like an
improvement than a bug fix, perhaps splitting the two (and
requesting backport of the remove side change) would be
better?

> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -905,8 +905,19 @@ csched_vcpu_insert(const struct scheduler *ops, struct 
> vcpu *vc)
>  {
>      struct csched_vcpu *svc = vc->sched_priv;
>  
> -    if ( !__vcpu_on_runq(svc) && vcpu_runnable(vc) && !vc->is_running )
> -        __runq_insert(vc->processor, svc);
> +    /*
> +     * For the idle domain, this is called, during boot, before
> +     * than alloc_pdata() has been called for the pCPU.
> +     */
> +    if ( !is_idle_vcpu(vc) )

Looking through the patch I can't spot why this would all of the
sudden need special casing. Can you explain that please? In
particular (in case it's just the acquiring of the lock that now
becomes an issue) - where would the runqueue insertion be
happening now? Or if one of the latter two conditions makes it
so that insertion turns out not to be needed, perhaps they could
be moved up to replace the is-idle check?

Also, not the least since the comment gets repeated below, if
it is to stay then I think the "than" should be dropped (or
re-worded more extensively).

Jan


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