From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Zijlstra Subject: Re: Preventing IPI sending races in arch code Date: Mon, 25 Nov 2013 14:57:25 +0100 Message-ID: <20131125135725.GA10022@twins.programming.kicks-ass.net> References: <52932BE2.5010201@synopsys.com> <20131125110006.GU3866@twins.programming.kicks-ass.net> <529334CA.1000401@synopsys.com> <20131125122726.GZ10022@twins.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Vineet Gupta Cc: "linux-arch@vger.kernel.org" , Gilad Ben-Yossef , Noam Camus , David Daney , James Hogan , thomas Gleixner , lkml , Richard Kuo List-Id: linux-arch.vger.kernel.org On Mon, Nov 25, 2013 at 01:35:38PM +0000, Vineet Gupta wrote: > On 11/25/2013 05:57 PM, Peter Zijlstra wrote: > > So sure, then someone can again assert the interrupt, but given we just > > established a protocol for raising the thing; namely something like > > this: > > > > void arch_send_ipi(int cpu, int type) > > { > > u32 *pending_ptr = per_cpu_ptr(ipi_bits, cpu); > > u32 new, old; > > > > do { > > new = old = *pending_ptr; > > new |= 1U << type; > > } while (cmpxchg(pending_ptr, old, new) != old) > > > > if (!old) /* only raise the actual IPI if we set the first bit */ > > raise_ipi(cpu); > > } > > > > Who would re-assert it if we have !0 pending? > > I see your point. So in receiver, it is OK to de-assert the IPI before processing > the msg itself. > > Actually your code seems to be optimizing away asserting an IPI, if sender already > had a pending msg (assuming we retain the xchg loop in receiver). Was that an > intended optimization - or just a side effect of your code ;-) No, full intention. As you mentioned you wanted to avoid sending IPIs where none were needed. > >> IMO the while loop is > >> completely useless specially if IPIs are not coalesced in h/w. > > Agreed, the while loops seems superfluous. > > Not with your version of sender, since we need it as described above. No, even with my code; the receiving end should look like: void handle_ipi(struct pt_regs *regs) { u32 pending; ipi_clear(irq); pending = xchg(this_cpu_ptr(ipi_bits), 0); while (pending) { bit = ffs(pending); /* handle bit */ pending &= ~(1U << bit); } } So while it does have a while() loop, it only does a single xchg(). The version you showed before had the xchg() in the loop, that is not required. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from merlin.infradead.org ([205.233.59.134]:41809 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756634Ab3KYN5j (ORCPT ); Mon, 25 Nov 2013 08:57:39 -0500 Date: Mon, 25 Nov 2013 14:57:25 +0100 From: Peter Zijlstra Subject: Re: Preventing IPI sending races in arch code Message-ID: <20131125135725.GA10022@twins.programming.kicks-ass.net> References: <52932BE2.5010201@synopsys.com> <20131125110006.GU3866@twins.programming.kicks-ass.net> <529334CA.1000401@synopsys.com> <20131125122726.GZ10022@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Vineet Gupta Cc: "linux-arch@vger.kernel.org" , Gilad Ben-Yossef , Noam Camus , David Daney , James Hogan , thomas Gleixner , lkml , Richard Kuo Message-ID: <20131125135725.Z5rpFK5vf6txpzQ_H433rj0owA7Ajp3kjRZ04CXGKyY@z> On Mon, Nov 25, 2013 at 01:35:38PM +0000, Vineet Gupta wrote: > On 11/25/2013 05:57 PM, Peter Zijlstra wrote: > > So sure, then someone can again assert the interrupt, but given we just > > established a protocol for raising the thing; namely something like > > this: > > > > void arch_send_ipi(int cpu, int type) > > { > > u32 *pending_ptr = per_cpu_ptr(ipi_bits, cpu); > > u32 new, old; > > > > do { > > new = old = *pending_ptr; > > new |= 1U << type; > > } while (cmpxchg(pending_ptr, old, new) != old) > > > > if (!old) /* only raise the actual IPI if we set the first bit */ > > raise_ipi(cpu); > > } > > > > Who would re-assert it if we have !0 pending? > > I see your point. So in receiver, it is OK to de-assert the IPI before processing > the msg itself. > > Actually your code seems to be optimizing away asserting an IPI, if sender already > had a pending msg (assuming we retain the xchg loop in receiver). Was that an > intended optimization - or just a side effect of your code ;-) No, full intention. As you mentioned you wanted to avoid sending IPIs where none were needed. > >> IMO the while loop is > >> completely useless specially if IPIs are not coalesced in h/w. > > Agreed, the while loops seems superfluous. > > Not with your version of sender, since we need it as described above. No, even with my code; the receiving end should look like: void handle_ipi(struct pt_regs *regs) { u32 pending; ipi_clear(irq); pending = xchg(this_cpu_ptr(ipi_bits), 0); while (pending) { bit = ffs(pending); /* handle bit */ pending &= ~(1U << bit); } } So while it does have a while() loop, it only does a single xchg(). The version you showed before had the xchg() in the loop, that is not required.