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

[Jason Baron <jbaron@redhat.com>]

On Tue, Sep 21, 2010 at 03:12:32PM +0200, Andi Kleen wrote:
> On Fri, Sep 17, 2010 at 11:09:00AM -0400, Jason Baron wrote:
> > +extern void arch_jump_label_transform(struct jump_entry *entry,
> > +				 enum jump_label_type type);
> > +extern void jump_label_update(unsigned long key, enum jump_label_type type);
> > +extern void jump_label_apply_nops(struct module *mod);
> > +extern void arch_jump_label_text_poke_early(jump_label_t addr);
> 
> These function names are too long.
> 
> Also it would be better if the types for the pointers are kept
> instead of casting to unsigned long. All the variables
> are ints right?
> 

So far, yes. But I didn't want to force this, in case the users of the
API wanted to use other types. But I'm ok with 'int *' here.

> > +#define JUMP_LABEL_HASH_BITS 6
> > +#define JUMP_LABEL_TABLE_SIZE (1 << JUMP_LABEL_HASH_BITS)
> > +static struct hlist_head jump_label_table[JUMP_LABEL_TABLE_SIZE];
> 
> It's not clear to me why this hash table is needed. There should
> not be that many trace points, is it that big a problem to simply
> walk all the sections when something is changed?
> 
> Or maybe the sections could be just sorted and a binary search used
> like with exception tables.
> 
> I suspect that would simplify a lot of code.
> 
> Overall I like the idea, but the current code is too complicated
> for the benefit I think.
> 
> Can it be put on a diet?
> 
> -Andi

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.

Also, I think the hash table deals nicely with modules. I create a
linked list of only those module sections that are relevant to each hash
bucket. If you search through all the section on each enable/disable,
its going to be proportional to the number of modules as well.

thanks,

-Jason