Re: [Xen-devel] [PATCH] xen: recalculate per-cpupool credits when updating timeslice

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-01-29 at 11:59 +0100, Juergen Gross wrote:
> On 29/01/16 11:46, Jan Beulich wrote:
> > > > > On 29.01.16 at 11:21, <JGross@xxxxxxxx> wrote:
> > > --- a/xen/common/sched_credit.c
> > > +++ b/xen/common/sched_credit.c
> > > @@ -1086,12 +1086,19 @@ csched_dom_cntl(
> > > Âstatic inline void
> > > Â__csched_set_tslice(struct csched_private *prv, unsigned
> > > timeslice)
> > > Â{
> > > +ÂÂÂÂunsigned long flags;
> > > +
> > > +ÂÂÂÂspin_lock_irqsave(&prv->lock, flags);
> > > +
> > > ÂÂÂÂÂprv->tslice_ms = timeslice;
> > > ÂÂÂÂÂprv->ticks_per_tslice = CSCHED_TICKS_PER_TSLICE;
> > > ÂÂÂÂÂif ( prv->tslice_ms < prv->ticks_per_tslice )
> > > ÂÂÂÂÂÂÂÂÂprv->ticks_per_tslice = 1;
> > > ÂÂÂÂÂprv->tick_period_us = prv->tslice_ms * 1000 / prv-
> > > >ticks_per_tslice;
> > > ÂÂÂÂÂprv->credits_per_tslice = CSCHED_CREDITS_PER_MSEC * prv-
> > > >tslice_ms;
> > > +ÂÂÂÂprv->credit = prv->credits_per_tslice * prv->ncpus;
> > > +
> > > +ÂÂÂÂspin_unlock_irqrestore(&prv->lock, flags);
> > > Â}
> > 
> > The added locking, which has no reason give for in the description
> > at all, puzzles me: I can see it being needed (and having been
> > missing) when called from csched_sys_cntl(), but it's not clear to
> > me why it would be needed when called from csched_init(). Yet
> > csched_sys_cntl() subsequently als updates prv->ratelimit_us,
> > and hence the lock would perhaps better be taken there?
> 
> The locking is needed to protect against csched_alloc_pdata() and
> csched_free_pdata(). prv->credit could be permananently wrong
> without the lock, while prv->ratelimit_us can't be modified
> concurrently in a wrong way (it could be modified by two concurrent
> calls of csched_sys_cntl(), but even with locking one of both
> calls would be the winner, same applies to the case with no lock).
> 
> OTOH I don't mind moving the lock to csched_sys_cntl(). Dario,
> George, any preferences?
> 
Yes, I think having the lock in csched_sys_cntl() would be preferable.

In any case, since the lack of locking and lack of recalculation look
like two pretty independent existing bugs to me, can we have either:
Âa. two patches;
Âb. one patch but with both the issues described in the changelog.

My preference going to a.

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