From: Stephen Boyd <sboyd@codeaurora.org> To: paulmck@linux.vnet.ibm.com Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>, Krzysztof Kozlowski <k.kozlowski@samsung.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Mark Rutland <mark.rutland@arm.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>, Marek Szyprowski <m.szyprowski@samsung.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com> Subject: Re: [PATCH v2] ARM: Don't use complete() during __cpu_die Date: Mon, 09 Feb 2015 18:05:55 -0800 [thread overview] Message-ID: <54D96783.9050600@codeaurora.org> (raw) In-Reply-To: <20150210013723.GL4166@linux.vnet.ibm.com> On 02/09/15 17:37, Paul E. McKenney wrote: > On Mon, Feb 09, 2015 at 05:24:08PM -0800, Stephen Boyd wrote: >> On 02/05/15 08:11, Russell King - ARM Linux wrote: >>> On Thu, Feb 05, 2015 at 06:29:18AM -0800, Paul E. McKenney wrote: >>>> Works for me, assuming no hidden uses of RCU in the IPI code. ;-) >>> Sigh... I kind'a new it wouldn't be this simple. The gic code which >>> actually raises the IPI takes a raw spinlock, so it's not going to be >>> this simple - there's a small theoretical window where we have taken >>> this lock, written the register to send the IPI, and then dropped the >>> lock - the update to the lock to release it could get lost if the >>> CPU power is quickly cut at that point. >> Hm.. at first glance it would seem like a similar problem exists with >> the completion variable. But it seems that we rely on the call to >> complete() fom the dying CPU to synchronize with wait_for_completion() >> on the killing CPU via the completion's wait.lock. >> >> void complete(struct completion *x) >> { >> unsigned long flags; >> >> spin_lock_irqsave(&x->wait.lock, flags); >> x->done++; >> __wake_up_locked(&x->wait, TASK_NORMAL, 1); >> spin_unlock_irqrestore(&x->wait.lock, flags); >> } >> >> and >> >> static inline long __sched >> do_wait_for_common(struct completion *x, >> long (*action)(long), long timeout, int state) >> ... >> spin_unlock_irq(&x->wait.lock); >> timeout = action(timeout); >> spin_lock_irq(&x->wait.lock); >> >> >> so the power can't really be cut until the killing CPU sees the lock >> released either explicitly via the second cache flush in cpu_die() or >> implicitly via hardware. Maybe we can do the same thing here by using a >> spinlock for synchronization between the IPI handler and the dying CPU? >> So lock/unlock around the IPI sending from the dying CPU and then do a >> lock/unlock on the killing CPU before continuing. >> >> It would be nice if we didn't have to do anything at all though so >> perhaps we can make it a nop on configs where there isn't a big little >> switcher. Yeah it's some ugly coupling between these two pieces of code, >> but I'm not sure how we can do better. > The default ugly-but-known-to-work approach is to set a variable in > the dying CPU that the surviving CPU periodically polls. If all else > fails and all that. So it isn't the ugliest. Good. > >>> Also, we _do_ need the second cache flush in place to ensure that the >>> unlock is seen to other CPUs. >>> >>> We could work around that by taking and releasing the lock in the IPI >>> processing function... but this is starting to look less attractive >>> as the lock is private to irq-gic.c. >> With Daniel Thompson's NMI fiq patches at least the lock would almost >> always be gone, except for the bL switcher users. Another solution might >> be to put a hotplug lock around the bL switcher code and then skip >> taking the lock in gic_raise_softirq() if the IPI is our special hotplug >> one. Conditional locking is pretty ugly though, so perhaps this isn't >> such a great idea. > Which hotplug lock are you suggesting? We cannot use sleeplocks, because > releasing them can go through the scheduler, which is not legal at this > point. > I'm not suggesting we take any hotplug locks here in the cpu_die() path. I'm thinking we make the bL switcher code hold a hotplug lock or at least prevent hotplug from happening while it's moving IPIs from the outgoing CPU to the incoming CPU (see code in arch/arm/common/bL_switcher.c). Actually, I seem to recall that hotplug can't happen if preemption/irqs are disabled, so maybe nothing needs to change there and we can just assume that if we're sending the hotplug IPI we don't need to worry about taking the spinlock in gic_raise_softirq()? We still have conditional locking so it's still fragile. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
WARNING: multiple messages have this Message-ID (diff)
From: sboyd@codeaurora.org (Stephen Boyd) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2] ARM: Don't use complete() during __cpu_die Date: Mon, 09 Feb 2015 18:05:55 -0800 [thread overview] Message-ID: <54D96783.9050600@codeaurora.org> (raw) In-Reply-To: <20150210013723.GL4166@linux.vnet.ibm.com> On 02/09/15 17:37, Paul E. McKenney wrote: > On Mon, Feb 09, 2015 at 05:24:08PM -0800, Stephen Boyd wrote: >> On 02/05/15 08:11, Russell King - ARM Linux wrote: >>> On Thu, Feb 05, 2015 at 06:29:18AM -0800, Paul E. McKenney wrote: >>>> Works for me, assuming no hidden uses of RCU in the IPI code. ;-) >>> Sigh... I kind'a new it wouldn't be this simple. The gic code which >>> actually raises the IPI takes a raw spinlock, so it's not going to be >>> this simple - there's a small theoretical window where we have taken >>> this lock, written the register to send the IPI, and then dropped the >>> lock - the update to the lock to release it could get lost if the >>> CPU power is quickly cut at that point. >> Hm.. at first glance it would seem like a similar problem exists with >> the completion variable. But it seems that we rely on the call to >> complete() fom the dying CPU to synchronize with wait_for_completion() >> on the killing CPU via the completion's wait.lock. >> >> void complete(struct completion *x) >> { >> unsigned long flags; >> >> spin_lock_irqsave(&x->wait.lock, flags); >> x->done++; >> __wake_up_locked(&x->wait, TASK_NORMAL, 1); >> spin_unlock_irqrestore(&x->wait.lock, flags); >> } >> >> and >> >> static inline long __sched >> do_wait_for_common(struct completion *x, >> long (*action)(long), long timeout, int state) >> ... >> spin_unlock_irq(&x->wait.lock); >> timeout = action(timeout); >> spin_lock_irq(&x->wait.lock); >> >> >> so the power can't really be cut until the killing CPU sees the lock >> released either explicitly via the second cache flush in cpu_die() or >> implicitly via hardware. Maybe we can do the same thing here by using a >> spinlock for synchronization between the IPI handler and the dying CPU? >> So lock/unlock around the IPI sending from the dying CPU and then do a >> lock/unlock on the killing CPU before continuing. >> >> It would be nice if we didn't have to do anything at all though so >> perhaps we can make it a nop on configs where there isn't a big little >> switcher. Yeah it's some ugly coupling between these two pieces of code, >> but I'm not sure how we can do better. > The default ugly-but-known-to-work approach is to set a variable in > the dying CPU that the surviving CPU periodically polls. If all else > fails and all that. So it isn't the ugliest. Good. > >>> Also, we _do_ need the second cache flush in place to ensure that the >>> unlock is seen to other CPUs. >>> >>> We could work around that by taking and releasing the lock in the IPI >>> processing function... but this is starting to look less attractive >>> as the lock is private to irq-gic.c. >> With Daniel Thompson's NMI fiq patches at least the lock would almost >> always be gone, except for the bL switcher users. Another solution might >> be to put a hotplug lock around the bL switcher code and then skip >> taking the lock in gic_raise_softirq() if the IPI is our special hotplug >> one. Conditional locking is pretty ugly though, so perhaps this isn't >> such a great idea. > Which hotplug lock are you suggesting? We cannot use sleeplocks, because > releasing them can go through the scheduler, which is not legal at this > point. > I'm not suggesting we take any hotplug locks here in the cpu_die() path. I'm thinking we make the bL switcher code hold a hotplug lock or at least prevent hotplug from happening while it's moving IPIs from the outgoing CPU to the incoming CPU (see code in arch/arm/common/bL_switcher.c). Actually, I seem to recall that hotplug can't happen if preemption/irqs are disabled, so maybe nothing needs to change there and we can just assume that if we're sending the hotplug IPI we don't need to worry about taking the spinlock in gic_raise_softirq()? We still have conditional locking so it's still fragile. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2015-02-10 2:05 UTC|newest] Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-02-05 10:14 [PATCH v2] ARM: Don't use complete() during __cpu_die Krzysztof Kozlowski 2015-02-05 10:14 ` Krzysztof Kozlowski 2015-02-05 10:50 ` Russell King - ARM Linux 2015-02-05 10:50 ` Russell King - ARM Linux 2015-02-05 11:00 ` Krzysztof Kozlowski 2015-02-05 11:00 ` Krzysztof Kozlowski 2015-02-05 11:08 ` Russell King - ARM Linux 2015-02-05 11:08 ` Russell King - ARM Linux 2015-02-05 11:28 ` Mark Rutland 2015-02-05 11:28 ` Mark Rutland 2015-02-05 11:30 ` Russell King - ARM Linux 2015-02-05 11:30 ` Russell King - ARM Linux 2015-02-05 14:29 ` Paul E. McKenney 2015-02-05 14:29 ` Paul E. McKenney 2015-02-05 16:11 ` Russell King - ARM Linux 2015-02-05 16:11 ` Russell King - ARM Linux 2015-02-05 17:02 ` Paul E. McKenney 2015-02-05 17:02 ` Paul E. McKenney 2015-02-05 17:34 ` Russell King - ARM Linux 2015-02-05 17:34 ` Russell King - ARM Linux 2015-02-05 17:54 ` Paul E. McKenney 2015-02-05 17:54 ` Paul E. McKenney 2015-02-10 1:24 ` Stephen Boyd 2015-02-10 1:24 ` Stephen Boyd 2015-02-10 1:37 ` Paul E. McKenney 2015-02-10 1:37 ` Paul E. McKenney 2015-02-10 2:05 ` Stephen Boyd [this message] 2015-02-10 2:05 ` Stephen Boyd 2015-02-10 3:05 ` Paul E. McKenney 2015-02-10 3:05 ` Paul E. McKenney 2015-02-10 15:14 ` Mark Rutland 2015-02-10 15:14 ` Mark Rutland 2015-02-10 20:48 ` Stephen Boyd 2015-02-10 20:48 ` Stephen Boyd 2015-02-10 21:04 ` Stephen Boyd 2015-02-10 21:04 ` Stephen Boyd 2015-02-10 21:15 ` Russell King - ARM Linux 2015-02-10 21:15 ` Russell King - ARM Linux 2015-02-10 21:49 ` Stephen Boyd 2015-02-10 21:49 ` Stephen Boyd 2015-02-10 22:05 ` Stephen Boyd 2015-02-10 22:05 ` Stephen Boyd 2015-02-13 15:52 ` Mark Rutland 2015-02-13 15:52 ` Mark Rutland 2015-02-13 16:27 ` Russell King - ARM Linux 2015-02-13 16:27 ` Russell King - ARM Linux 2015-02-13 17:21 ` Mark Rutland 2015-02-13 17:21 ` Mark Rutland 2015-02-13 17:30 ` Russell King - ARM Linux 2015-02-13 17:30 ` Russell King - ARM Linux 2015-02-13 16:28 ` Stephen Boyd 2015-02-13 16:28 ` Stephen Boyd 2015-02-13 15:38 ` Mark Rutland 2015-02-13 15:38 ` Mark Rutland 2015-02-10 20:58 ` Russell King - ARM Linux 2015-02-10 20:58 ` Russell King - ARM Linux 2015-02-10 15:41 ` Russell King - ARM Linux 2015-02-10 15:41 ` Russell King - ARM Linux 2015-02-10 18:33 ` Stephen Boyd 2015-02-10 18:33 ` Stephen Boyd 2015-02-25 12:56 ` Russell King - ARM Linux 2015-02-25 12:56 ` Russell King - ARM Linux 2015-02-25 16:47 ` Nicolas Pitre 2015-02-25 16:47 ` Nicolas Pitre 2015-02-25 17:00 ` Russell King - ARM Linux 2015-02-25 17:00 ` Russell King - ARM Linux 2015-02-25 18:13 ` Nicolas Pitre 2015-02-25 18:13 ` Nicolas Pitre 2015-02-25 20:16 ` Nicolas Pitre 2015-02-25 20:16 ` Nicolas Pitre 2015-02-26 1:05 ` Paul E. McKenney 2015-02-26 1:05 ` Paul E. McKenney 2015-03-22 23:30 ` Paul E. McKenney 2015-03-22 23:30 ` Paul E. McKenney 2015-03-23 12:55 ` Russell King - ARM Linux 2015-03-23 12:55 ` Russell King - ARM Linux 2015-03-23 13:21 ` Paul E. McKenney 2015-03-23 13:21 ` Paul E. McKenney 2015-03-23 14:00 ` Russell King - ARM Linux 2015-03-23 14:00 ` Russell King - ARM Linux 2015-03-23 15:37 ` Paul E. McKenney 2015-03-23 15:37 ` Paul E. McKenney 2015-03-23 16:56 ` Paul E. McKenney 2015-03-23 16:56 ` Paul E. McKenney 2015-02-26 19:14 ` Daniel Thompson 2015-02-26 19:14 ` Daniel Thompson 2015-02-26 19:47 ` Nicolas Pitre 2015-02-26 19:47 ` Nicolas Pitre 2015-02-05 10:53 ` Mark Rutland 2015-02-05 10:53 ` Mark Rutland 2015-02-05 10:59 ` Krzysztof Kozlowski 2015-02-05 10:59 ` Krzysztof Kozlowski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=54D96783.9050600@codeaurora.org \ --to=sboyd@codeaurora.org \ --cc=arnd@arndb.de \ --cc=b.zolnierkie@samsung.com \ --cc=catalin.marinas@arm.com \ --cc=k.kozlowski@samsung.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=m.szyprowski@samsung.com \ --cc=mark.rutland@arm.com \ --cc=paulmck@linux.vnet.ibm.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.