Messages in this thread | | | Date | Wed, 4 Oct 2017 07:17:45 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH 3/3] early_printk: Add simple serialization to early_vprintk() |
| |
On Wed, Oct 04, 2017 at 09:04:01AM -0400, Steven Rostedt wrote: > On Wed, 4 Oct 2017 11:08:30 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Tue, Oct 03, 2017 at 06:24:22PM -0400, Steven Rostedt wrote: > > > On Thu, 28 Sep 2017 14:18:26 +0200 > > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > static int early_vprintk(const char *fmt, va_list args) > > > > { > > > > + int n, cpu, old; > > > > char buf[512]; > > > > + > > > > + cpu = get_cpu(); > > > > + /* > > > > + * Test-and-Set inter-cpu spinlock with recursion. > > > > + */ > > > > + for (;;) { > > > > + /* > > > > + * c-cas to avoid the exclusive bouncing on spin. > > > > + * Depends on the memory barrier implied by cmpxchg > > > > + * for ACQUIRE semantics. > > > > + */ > > > > + old = READ_ONCE(early_printk_cpu); > > > > + if (old == -1) { > > > > > > If old != -1 and old != cpu, is it possible that the CPU could have > > > fetched an old value, and never try to fetch it again? > > > > What? If old != -1 and old != cpu, we'll hit the cpu_relax() and do the > > READ_ONCE() again. The READ_ONCE() guarantees we'll do the load again, > > as does the barrier() implied by cpu_relax(). > > I'm more worried about other architectures that don't have as strong of > a cache coherency. > > [ Added Paul as he knows a lot about odd architectures ] > > Is there any architecture that we support that can have the following: > > CPU0 CPU1 > ---- ---- > early_printk_cpu = 1 > for (;;) > old = READ_ONCE(early_printk_cpu); > [ old = 1 ] > > early_printk_cpu = -1 > > [...] > cpu_relax(); > old = READ_ONCE(early_printk_cpu); > > [ but the CPU uses the cache and not the memory? ] > > old = 1;
If you use READ_ONCE(), then all architectures I know of enforce full ordering for accesses to a single variable. (If you don't use READ_ONCE(), then in theory Itanium can reorder reads.) Me, I would argue for WRITE_ONCE() as well to prevent store tearing.
It is only when you have at least two variables and at least two threads than things start getting really "interesting". ;-)
Thanx, Paul
> -- Steve > > > > > > > The cmpxchg memory barrier only happens when old == -1. > > > > Yeah, so? > > > > > > + old = cmpxchg(&early_printk_cpu, -1, cpu); > > > > + if (old == -1) > > > > + break; > > > > + } > > > > + /* > > > > + * Allow recursion for interrupts and the like. > > > > + */ > > > > + if (old == cpu) > > > > + break; > > > > + > > > > + cpu_relax(); > > > > + } > > > > > > > > n = vscnprintf(buf, sizeof(buf), fmt, args); > > > > early_console->write(early_console, buf, n); > > > > > > > > + /* > > > > + * Unlock -- in case @old == @cpu, this is a no-op. > > > > + */ > > > > + smp_store_release(&early_printk_cpu, old); > > > > + put_cpu(); > > > > + > > > > return n; > > > > } >
| |