Messages in this thread | | | Date | Fri, 1 May 2009 10:23:40 -0400 (EDT) | From | Steven Rostedt <> | Subject | Re: [PATCH 3/3] ring-buffer: make cpu buffer entries counter atomic |
| |
On Fri, 1 May 2009, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote: > > > From: Steven Rostedt <srostedt@redhat.com> > > > > The entries counter in cpu buffer is not atomic. Although it only > > gets updated by a single CPU, interrupts may come in and update > > the counter too. This would cause missing entries to be added. > > > - unsigned long entries; > > + atomic_t entries; > > Hm, that's not really good as atomics can be rather expensive and > this is the fastpath.
Actually, it could be local_t. I used that in a lot of the other places. The race is with on CPU not other CPUs, and on archs like x86 there is not cost of the "LOCK".
> > This is the upteenth time or so that the fact that we do not disable > irqs while generating trace entries bites us in one way or another. > IRQs can come in and confuse function trace output, etc. etc.
Note, this race is on a simple counter used for stats. It never was exposed to user land except in the latency output, and that tracer disables interrupts anyway.
> > Please lets do what i suggested a long time ago: disable irqs _once_ > in any trace point and run atomically from that point on, and enable > them once, at the end. > > The cost is very small and it turns into a win immediately by > elimination of a _single_ atomic instruction. (even on Nehalem they > cost 20 cycles. More on older CPUs.) We can drop the preempt-count > disable/enable as well and a lot of racy code as well. Please.
If we punt and simply disable interrupts in the ring buffer, I would then have to disable all tracing of NMIs. Yes it will make the code simpler, but the new code would also have:
ring_buffer_lock_reserve() {
if (in_nmi()) return NULL;
If that is acceptible, then fine. I'll make the change.
I will also throw away the lockless ring buffer since it would no long er be needed.
-- Steve
| |