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: Fri, 29 Jan 2016 11:59:10 +0100 Message-ID: <56AB45FE.5010500@suse.com> References: <1454062908-32013-1-git-send-email-jgross@suse.com> <56AB511402000078000CC59C@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56AB511402000078000CC59C@suse.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: Jan Beulich Cc: george.dunlap@eu.citrix.com, dario.faggioli@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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? Juergen