Re: [PATCH 03/10] jump label v11: base patch

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* 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