Messages in this thread | | | Date | Fri, 27 Mar 2015 16:11:33 -0400 | From | Steven Rostedt <> | Subject | Re: [for-next][PATCH 1/4] ring-buffer: Replace this_cpu_*() with __this_cpu_*() |
| |
On Fri, 27 Mar 2015 14:41:44 -0500 (CDT) Christoph Lameter <cl@linux.com> wrote:
> On Wed, 25 Mar 2015, Steven Rostedt wrote: > > > It has come to my attention that this_cpu_read/write are horrible on > > architectures other than x86. Worse yet, they actually disable > > preemption or interrupts! This caused some unexpected tracing results > > on ARM. > > This isnt something new and I thought the comment was dropped from the > patch? This is a plain error in using this_cpu_* where __this_cpu_* would > have been sufficient. Code was uselessly disabling preemption twice. >
Where in the patch do you see the comment? Or were you talking about the change log? The original patch did have a comment, an it was dropped, that's what I thought you were talking about.
> > Which is unacceptable for locations that know they are within preempt > > disabled or interrupt disabled locations. > > Well yes. Thats why the __this_cpu ops are there to avoid this > overhead. > > > I also changed the recursive_unlock() to use two local variables instead > > of accessing the per_cpu variable twice. > > Ok gotta look at that. > > > static __always_inline void trace_recursive_unlock(void) > > { > > - unsigned int val = this_cpu_read(current_context); > > + unsigned int val = __this_cpu_read(current_context); > > > > - val--; > > - val &= this_cpu_read(current_context); > > - this_cpu_write(current_context, val); > > + val &= val & (val - 1); > > + __this_cpu_write(current_context, val); > > } > > Ummm... This is does not look like an equivalent thing. Should this not > be: > > unsigned int val = __this_cpu_read(current_context); > unsigned int newval = val - 1; > > newval &= val; > __this_cpu_write(current_context, newval);
Actually, it is equivalent, but I do see a issue with my patch.
val &= val & (val - 1);
is the same as the more reasonable:
val &= val - 1;
I think I meant to replace &= with = :-/
> > or more compact > > unsigned int val = __this_cpu_read(current_context); > > __this_cpu_write(current_context, val & (val - 1));
Maybe I'll just use your compact version.
Thanks,
-- Steve
| |