Re: [RFC][PATCH] tracing/jump-labels: Add message when a trace event has no instances

[Steven Rostedt]

On Wed, 11 Oct 2017 09:21:09 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Tue, Oct 10, 2017 at 06:56:30PM -0400, Steven Rostedt wrote:
> 
> > +/* Without jump labels we don't know how many instances, just assume one */
> > +static inline int static_key_instances(struct static_key *key)
> > +{
> > +	return 1;
> > +}  
> 
> That's a tad dodgy.. it works for your current usage, but..

Yeah, but I did add a comment to explain my dodginess.

> 
> 
> > +int static_key_instances(struct static_key *key)
> > +{
> > +	return jump_label_update(key, true);
> >  }  
> 
> That's a bit yuck too :/ But I'm not entirely sure something like the
> below is actually better...

This patch is actually my third iteration.

1st iteration simply copied the update code to an instance version. But
I didn't like the duplication of the code, as one could be modified
without the other.

My 2nd iteration did exactly what you did below. Have the code iterate
through the entities and pass a function to process them. Half way
through doing this, I realized that it was a huge hammer for such a
small nail. When I saw that it was just one little function, and adding
a counter would be trivial, I thought the third way (my patch) was
actually the simplest solution. Yes, a bit icky, but still the
simplest. I don't foresee any other functions being added to iterate
over this, thus I picked the simplest solution.

I think this is a bit too much overkill for the solution of the problem.

-- Steve


> 
> ---
>  kernel/jump_label.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 0bf2e8f5244a..97286ed6c93d 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -354,9 +354,17 @@ static enum jump_label_type jump_label_type(struct jump_entry *entry)
>  	return enabled ^ branch;
>  }
>  
> -static void __jump_label_update(struct static_key *key,
> -				struct jump_entry *entry,
> -				struct jump_entry *stop)
> +typedef void (*jump_label_visit_f)(struct jump_entry *entry, void *data);
> +
> +void __jump_label_update(struct jump_entry *entry, void *data)
> +{
> +	arch_jump_label_transform(entry, jump_label_type(entry));
> +}
> +
> +static void
> +__jump_label_visit(struct static_key *key, struct jump_entry *entry,
> +		   struct jump_entry *stop, jump_label_visit_f *visit,
> +		   void *data)
>  {
>  	for (; (entry < stop) && (jump_entry_key(entry) == key); entry++) {
>  		/*
> @@ -365,7 +373,7 @@ static void __jump_label_update(struct static_key *key,
>  		 * init code, see jump_label_invalidate_module_init().
>  		 */
>  		if (entry->code && kernel_text_address(entry->code))
> -			arch_jump_label_transform(entry, jump_label_type(entry));
> +			visit(entry, data);
>  	}
>  }
>  
> @@ -470,7 +478,8 @@ static int __jump_label_mod_text_reserved(void *start, void *end)
>  				start, end);
>  }
>  
> -static void __jump_label_mod_update(struct static_key *key)
> +static void
> +__jump_label_mod_visit(struct static_key *key, jump_label_visit_f visit, void *data)
>  {
>  	struct static_key_mod *mod;
>  
> @@ -490,7 +499,8 @@ static void __jump_label_mod_update(struct static_key *key)
>  			stop = __stop___jump_table;
>  		else
>  			stop = m->jump_entries + m->num_jump_entries;
> -		__jump_label_update(key, mod->entries, stop);
> +
> +		__jump_label_visit(key, mod->entries, stop, visit, data);
>  	}
>  }
>  
> @@ -571,7 +581,7 @@ static int jump_label_add_module(struct module *mod)
>  
>  		/* Only update if we've changed from our initial state */
>  		if (jump_label_type(iter) != jump_label_init_type(iter))
> -			__jump_label_update(key, iter, iter_stop);
> +			__jump_label_visit(key, iter, iter_stop, __jump_label_update, NULL);
>  	}
>  
>  	return 0;
> @@ -711,7 +721,7 @@ int jump_label_text_reserved(void *start, void *end)
>  	return ret;
>  }
>  
> -static void jump_label_update(struct static_key *key)
> +static void jump_label_visit(struct static_key *key, jump_label_visit_f visit, void *data)
>  {
>  	struct jump_entry *stop = __stop___jump_table;
>  	struct jump_entry *entry;
> @@ -719,7 +729,7 @@ static void jump_label_update(struct static_key *key)
>  	struct module *mod;
>  
>  	if (static_key_linked(key)) {
> -		__jump_label_mod_update(key);
> +		__jump_label_mod_visit(key, visit, data);
>  		return;
>  	}
>  
> @@ -732,7 +742,27 @@ static void jump_label_update(struct static_key *key)
>  	entry = static_key_entries(key);
>  	/* if there are no users, entry can be NULL */
>  	if (entry)
> -		__jump_label_update(key, entry, stop);
> +		__jump_label_visit(key, entry, stop, visit, data);
> +}
> +
> +static void jump_label_update(struct static_key *key)
> +{
> +	jump_label_visit(key, __jump_label_update, NULL);
> +}
> +
> +static void __jump_label_count(struct jump_entry *entry, void *data)
> +{
> +	unsigned int *count = data;
> +	count++;
> +}
> +
> +unsigned int static_key_instances(struct static_key *key)
> +{
> +	unsigned int count = 0;
> +
> +	jump_label_visit(key, __jump_label_count, &count);
> +
> +	return count;
>  }
>  
>  #ifdef CONFIG_STATIC_KEYS_SELFTEST