From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: [PATCH v4 1/6] xen: sched: fix locking of remove_vcpu() in credit1 Date: Wed, 04 Nov 2015 18:17:33 +0100 Message-ID: <20151104171733.20002.86229.stgit@Solace.station> References: <20151104170839.20002.15551.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 1Zu1go-0002MA-2c for xen-devel@lists.xenproject.org; Wed, 04 Nov 2015 17:17:38 +0000 Received: by wicll6 with SMTP id ll6so94197112wic.0 for ; Wed, 04 Nov 2015 09:17:36 -0800 (PST) In-Reply-To: <20151104170839.20002.15551.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: xen-devel@lists.xenproject.org Cc: George Dunlap , Andrew Cooper , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 --- 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