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

[Juergen Gross <jgross@suse.com>]

On 02/02/16 10:53, Dario Faggioli wrote:
> 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@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.

Without setting prv->credit the lock isn't necessary. In case of a
race domain weights wouldn't be honored correctly for just one
timeslice and I doubt this would be noticeable at all.

OTOH I don't mind splitting the patch into two, I have to respin
anyway.


Juergen