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

[Juergen Gross <jgross@suse.com>]

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?


Juergen