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 -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
--- Begin Message ---[PATCH v2] xen: credit2: fix spinlock irq-safety violation In commit ad4b3e1e9df34 ("xen: credit2: implement utilization cap") xfree() was being called (for deallocating the budget replenishment timer, during domain destruction) inside an IRQ disabled critical section. That must not happen, as it uses the mem-pool's lock, which needs to be taken with IRQ enabled. And, in fact, we crash (in debug builds): (XEN) **************************************** (XEN) Panic on CPU 0: (XEN) Xen BUG at spinlock.c:47 (XEN) **************************************** Let's, therefore, kill and deallocate the timer outside of the critical sections, when IRQs are enabled. Signed-off-by: Dario Faggioli <dario.faggi...@citrix.com> --- Cc: osstest service owner <osstest-ad...@xenproject.org> Cc: George Dunlap <george.dun...@eu.citrix.com> Cc: Wei Liu <wei.l...@citrix.com> --- This was spotted by OSSTest's flight 113562: http://logs.test-lab.xenproject.org/osstest/logs/113562/ http://logs.test-lab.xenproject.org/osstest/logs/113562/test-amd64-amd64-xl-credit2/serial-godello0.log --- Changes from v1: - kill_timer() and xfree() moved above critical section, instead than below. 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);
--- End Message ---
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
