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, <jgr...@suse.com> 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)
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
