From patchwork Wed Sep 20 11:44:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Dario Faggioli X-Patchwork-Id: 9961337 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id AC79960234 for ; Wed, 20 Sep 2017 11:48:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9C80528E91 for ; Wed, 20 Sep 2017 11:48:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 915B2290F0; Wed, 20 Sep 2017 11:48:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 16F2B28E91 for ; Wed, 20 Sep 2017 11:48:39 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dudRb-000733-ST; Wed, 20 Sep 2017 11:45:31 +0000 Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dudRa-00072x-Kd for xen-devel@lists.xenproject.org; Wed, 20 Sep 2017 11:45:30 +0000 Received: from [85.158.143.35] by server-9.bemta-6.messagelabs.com id 8D/BB-03422-9D452C95; Wed, 20 Sep 2017 11:45:29 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrIKsWRWlGSWpSXmKPExsXitHRDpO7NkEO RBnc38Fh83zKZyYHR4/CHKywBjFGsmXlJ+RUJrBn3Z19iL7ihW9H6opO9gXGzRhcjJ4eEQIjE /aWHmUBsXgFDiQ1nHoDZwgIuEq9nPWUHsdkEDCTe7NjLCmKLCPhJdOz+DGYzC7QySpz5qglis wioSlx+PBesl1PATuJx01rGLkYuDiGBLYwSGyY3gg3iF5CUuPXlIzNEc7VE/7wdrBBHaEscOd fPDnGEoMTJmU9YQGwhATWJGXMvs05g5JuFpGUWkjKIuIPE4RsdbBC2pkTr9t/sELa2xLKFr5k hbFuJlktboOpdJC7P+wBmw/w2C+qHhne/wHqFBYQlDi84yD4LGi5/pz4GsjnA9v7dIYzN+HXr 3kONt5HYdHUBI4QtL7H97RzmBYwCqxg1ilOLylKLdA0t9JKKMtMzSnITM3N0DQ3M9HJTi4sT0 1NzEpOK9ZLzczcxAqOUAQh2MN7cGHCIUZKDSUmUtyrgUKQQX1J+SmVGYnFGfFFpTmrxIUYNDg 6BCWfnTmeSYsnLz0tVkuDlASYDIcGi1PTUirTMHGAagSmV4OBREuENB0nzFhck5hZnpkOkTjH qcnTcvPuHSQhshpQ4rypIkQBIUUZpHtwIWEq7xCgrJczLCHSgEE9BalFuZgmq/CtGcQ5GJWHe /GCgKTyZeSVwm14BHcEEdET2hgMgR5QkIqSkGhi510sy63CkLD+z5uLNpwWffdIXFa/YnjTF2 cC3cd0SzfhkoYd2T2+vclLZyVnFu9XkfeakWY/W3rJ9seq8ut3RjgkaJ84pRE69v9oyQeRlyx S7xfP2sYc9ONre+dLixqMHbC0yNSc+Tti6g3flvgVff0/LP6v28eEd6/lLnnPNXL32x3n9qxu T7yqxFGckGmoxFxUnAgCQmkK4ZAMAAA== X-Env-Sender: prvs=4292b0fa5=dario.faggioli@citrix.com X-Msg-Ref: server-10.tower-21.messagelabs.com!1505907927!77126165!1 X-Originating-IP: [66.165.176.89] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni44OSA9PiAyMDMwMDc=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 9.4.45; banners=-,-,- X-VirusChecked: Checked Received: (qmail 13055 invoked from network); 20 Sep 2017 11:45:28 -0000 Received: from smtp.citrix.com (HELO SMTP.CITRIX.COM) (66.165.176.89) by server-10.tower-21.messagelabs.com with RC4-SHA encrypted SMTP; 20 Sep 2017 11:45:28 -0000 X-IronPort-AV: E=Sophos;i="5.42,421,1500940800"; d="asc'?scan'208";a="440478520" Message-ID: <1505907844.3483.12.camel@citrix.com> From: Dario Faggioli To: George Dunlap , Date: Wed, 20 Sep 2017 13:44:04 +0200 In-Reply-To: <3c1b015f-ad2a-9bc7-7414-3d51c13e15c1@citrix.com> References: <150578708843.32006.17195420852157192880.stgit@Solace.fritz.box> <68900559-7c07-0009-3a61-a6fe82ddd2b5@citrix.com> <3c1b015f-ad2a-9bc7-7414-3d51c13e15c1@citrix.com> Organization: Citrix Inc. X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) MIME-Version: 1.0 Cc: George Dunlap , Wei Liu , osstest service owner Subject: Re: [Xen-devel] [PATCH] xen: credit2: fix spinlock irq-safety violation X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 2017-09-20 at 11:04 +0100, George Dunlap wrote: > On 09/20/2017 10:19 AM, George Dunlap wrote: > > > diff --git a/xen/common/sched_credit2.c > > > b/xen/common/sched_credit2.c > > > index 32234ac..7a550db 100644 > > > --- a/xen/common/sched_credit2.c > > > +++ b/xen/common/sched_credit2.c > > > @@ -2923,13 +2923,13 @@ csched2_free_domdata(const struct > > > scheduler *ops, void *data) > > >   > > >      write_lock_irqsave(&prv->lock, flags); > > >   > > > -    kill_timer(sdom->repl_timer); > > > -    xfree(sdom->repl_timer); > > > - > > >      list_del_init(&sdom->sdom_elem); > > >   > > >      write_unlock_irqrestore(&prv->lock, flags); > > >   > > > +    kill_timer(sdom->repl_timer); > > > +    xfree(sdom->repl_timer); > > > > Any particular reason for moving the kill_timer() as well as the > > xfree > > outside the lock?  What happens if the timer goes off after the > > irqrestore but before the kill_timer? > > Looks like if the timer fires, nothing terribly bad will happen; it > will > just do a useless budget replenishment. > It's just that it has not reason to be there, as nothing that it does is serialized by prv->lock, so it only makes the critical section (with IRQ disabled) longer, for no reason, which is bad, as this being a write_lock(), it'll stop readers too. I think it was (my) mistake to put it there in the first place. > It looks like kill_timer() disables irqs, so it could be moved inside > the critical section.  I'm inclined to say we should do so.  I don't > anticipate the budget replenishment ever to need to walk the domain > list, but should that change, this would be a bear of a bug to find. > Indeed it's rather unlikely for the replenishment handler to have to use sdom_elem to go through the list of domains. IAC, if you're concerned about that, I'd much rather put both kill_timer() and xfree() before the critical section, rather than after, like in the attached patch. Dario diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index 32234ac..32b0363 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -2921,11 +2921,11 @@ csched2_free_domdata(const struct scheduler *ops, void *data) struct csched2_dom *sdom = data; struct csched2_private *prv = csched2_priv(ops); - write_lock_irqsave(&prv->lock, flags); - kill_timer(sdom->repl_timer); xfree(sdom->repl_timer); + write_lock_irqsave(&prv->lock, flags); + list_del_init(&sdom->sdom_elem); write_unlock_irqrestore(&prv->lock, flags);