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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2015-10-01 at 02:03 -0600, Jan Beulich wrote:
> > > > 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?
> 
Yes, I like the idea, I'll do this.

> > --- 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? 
>
So, this is an issue in Credit2 (from where the comment was taken) and
RTDS. I've checked further and, for Credit, it is an issue only without
patch 4 of this series, and even in that case, it can be cured in
another (better) way, I think.

So, I'll not only split, but also rework this patch a bit, and
resubmit.

> 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?
> 
Yes, it was the !vc->is_running part which prevents the insertion of
idle vcpus in runqueues already. I wouldn't like having it guarding the
whole thing, for various reasons, but as I said, I think I can rework
the patch to make things more clear.

> 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).
> 
Yep, as a part of the rework, I'll reword the comment(s).

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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