From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH] xen: recalculate per-cpupool credits when updating timeslice Date: Tue, 2 Feb 2016 11:35:37 +0100 Message-ID: <56B08679.3060807@suse.com> References: <1454062908-32013-1-git-send-email-jgross@suse.com> <56AB511402000078000CC59C@suse.com> <56AB45FE.5010500@suse.com> <1454406782.9227.27.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1454406782.9227.27.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli , Jan Beulich Cc: george.dunlap@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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, 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