From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v3 1/6] xen: sched: fix locking of remove_vcpu() in credit1 Date: Mon, 2 Nov 2015 18:04:31 +0000 Message-ID: <5637A5AF.4020205@citrix.com> References: <20151029225158.25219.4625.stgit@Solace.station> <20151029230413.25219.20869.stgit@Solace.station> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZtJTF-0004YW-MN for xen-devel@lists.xenproject.org; Mon, 02 Nov 2015 18:04:41 +0000 In-Reply-To: <20151029230413.25219.20869.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 29/10/15 23:04, 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. > > 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: Andrew Cooper > --- > Cc: George Dunlap > Cc: Jan Beulich > --- > 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. > --- > xen/common/sched_credit.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > index b8f28fe..6f71e0d 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -926,7 +926,7 @@ 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; > + spinlock_t *lock; > > SCHED_STAT_CRANK(vcpu_remove); > > @@ -936,15 +936,19 @@ csched_vcpu_remove(const struct scheduler *ops, struct vcpu *vc) > vcpu_unpause(svc->vcpu); > } > > + lock = vcpu_schedule_lock_irq(vc); > + > if ( __vcpu_on_runq(svc) ) > __runq_remove(svc); > > - spin_lock_irqsave(&(prv->lock), flags); > + vcpu_schedule_unlock_irq(lock, vc); Actually, at this point the domain should be either paused or in the middle of being destroyed, so it shouldn't be possible for the vcpu to be on the runqueue, should it? Should we instead change that if() to an ASSERT(!__vcpu_on_runqueue(svc))? -George