Messages in this thread | | | Date | Tue, 21 Sep 2010 14:24:32 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH 03/10] jump label v11: base patch |
| |
* Steven Rostedt (rostedt@goodmis.org) wrote: > On Tue, 2010-09-21 at 19:36 +0200, Andi Kleen wrote: > > > On Tue, 2010-09-21 at 16:41 +0200, Andi Kleen wrote: > > >> > > > >> > So there are ~150 tracepoints, but this code is also being proposed > > >> for > > >> > use with 'dynamic debug' of which there are > 1000, and I'm hoping for > > >> > more users moving forward. > > >> > > >> Even 1000 is fine to walk, but if it was sorted a binary search > > >> would be much faster anyways. That is then you would still > > >> need to search for each module, but that is a relatively small > > >> number (< 100) > > > > > > xfs has > 100 tracepoints > > > > Doesn > > I suppose you were missing a 't'. > > Anyway: > > $ find fs/xfs/ -name "*.c" ! -type d | xargs grep "[ ^I]trace_" | wc -l > 313 > > The jump label occurs at the calling sight, not for defined tracepoints > (which can be used in multiple places). > > Also take a look at fs/xfs/linux-2.6/xfs_trace.h, you will be surprised.
Yep, I'd be surprised to see how many tracepoints we can end up having with stuff like kmem tracing (trace_kmalloc). Each instance of the inline function will generate an entry. (!)
> > > > > > > >> > > >> > Also, I think the hash table deals nicely with modules. > > >> > > >> Maybe but it's also a lot of code. And it seems to me > > >> that it is optimizing the wrong thing. Simpler is nicer. > > > > > > I guess simplicity is in the eye of the beholder. I find hashes easier > > > to deal with than binary searching sorted lists. Every time you add a > > > tracepoint, you need to resort the list. > > > > The only time you add one is when you load a module, right? When you do > > that you only sort the section of the new module. > > And on removing a module. > > > > > > Hashes are much easier to deal with and scale nicely. I don't think > > > there's enough rational to switch this to a binary list. > > > > Well problem is that the code is very complicated today. I suspect > > this could be done much simpler if it wasn't so overengin > > > > Perhaps it can be cleaned up. But I have no issues with it now, and > using a hash (basic data structures 101) is not where the complexity > comes in.
I agree with Steven, Peter and Jason: due to the large amount of tracepoints we can end up patching, we should keep the hash tables. This code is very similar to what I have in the tracepoints already and in the immediate values. So this code is solid and has been tested over a large user base for quite some time already.
One change I would recommend is to use a separate memory pool to allocate the struct jump_label_entry, to favor better locality. I did not do it in tracepoints and markers because each entry have a variable length, but given that struct jump_label_entry seems to be fixed-size, then we should definitely go for a kmem_cache_alloc().
Thanks,
Mathieu
> > -- Steve > >
-- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com
| |