From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v4 1/6] xen: sched: fix locking of remove_vcpu() in credit1 Date: Wed, 4 Nov 2015 17:22:11 +0000 Message-ID: <563A3EC3.1090304@citrix.com> References: <20151104170839.20002.15551.stgit@Solace.station> <20151104171733.20002.86229.stgit@Solace.station> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Zu1lI-0003Lu-Or for xen-devel@lists.xenproject.org; Wed, 04 Nov 2015 17:22:16 +0000 In-Reply-To: <20151104171733.20002.86229.stgit@Solace.station> 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 , xen-devel@lists.xenproject.org Cc: George Dunlap , Andrew Cooper , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 04/11/15 17:17, Dario Faggioli wrote: > In fact, csched_vcpu_remove() (i.e., the credit1 > implementation of remove_vcpu()) manipulates runqueues, > so holding the runqueue lock is necessary. > > However, the vCPU just can't be on the runqueue, when > the function is called. We can therefore ASSERT() that, > and avoid doing any runqueue manipulations (rather than > adding the runqueue locking around it). > > Also, while there, *_lock_irq() (for the private lock) is > enough, there is no need to *_lock_irqsave(). > > Signed-off-by: Dario Faggioli Reviewed-by: George Dunlap > --- > Cc: George Dunlap > Cc: Andrew Cooper > Cc: Jan Beulich > --- > Changes from v3: > * instead of adding locking, get rid of __runq_remove(), > and add an ASSERT() about vCPU not being in runq already, > as suggested during review. > > Changes from the other series: > * split the patch (wrt the original patch, in the original > series), and take care, in this one, only of remove_vcpu(); > * removed pointless parentheses. > --- > The fact that vCPU can't be in runqueue when calling remove_vcpu() is true for > other schedulers as well. In them, though, there isn't any race condition to > fix. Therefore, taking care of the other schedulers will happen in a followup > series. > --- > xen/common/sched_credit.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > index 1b30e67..6dfcff6 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -934,28 +934,25 @@ csched_vcpu_remove(const struct scheduler *ops, struct vcpu *vc) > struct csched_private *prv = CSCHED_PRIV(ops); > struct csched_vcpu * const svc = CSCHED_VCPU(vc); > struct csched_dom * const sdom = svc->sdom; > - unsigned long flags; > > SCHED_STAT_CRANK(vcpu_remove); > > + ASSERT(!__vcpu_on_runq(svc)); > + > if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) ) > { > SCHED_STAT_CRANK(vcpu_unpark); > vcpu_unpause(svc->vcpu); > } > > - if ( __vcpu_on_runq(svc) ) > - __runq_remove(svc); > - > - spin_lock_irqsave(&(prv->lock), flags); > + spin_lock_irq(&prv->lock); > > if ( !list_empty(&svc->active_vcpu_elem) ) > __csched_vcpu_acct_stop_locked(prv, svc); > > - spin_unlock_irqrestore(&(prv->lock), flags); > + spin_unlock_irq(&prv->lock); > > BUG_ON( sdom == NULL ); > - BUG_ON( !list_empty(&svc->runq_elem) ); > } > > static void >