Re: [PATCHv3] jump_label,x86: make batch update of jump_label entries

[Jiri Olsa <jolsa@redhat.com>]

hi,
any feedback?

thanks,
jirka

On Mon, May 09, 2011 at 03:27:26PM -0400, Jason Baron wrote:
> On Mon, May 09, 2011 at 08:38:16PM +0200, Jiri Olsa wrote:
> > On Wed, May 04, 2011 at 11:41:41AM +0200, Jiri Olsa wrote:
> > > hi,
> > > 
> > > I'm changing the jump label update code to use batch processing
> > > for x86 architectures. 
> > > 
> > > Currently each jump label update calls text_poke_smp for each
> > > jump label key entry. Thus one key update ends up calling stop
> > > machine multiple times.
> > > 
> > > This patch is using text_poke_smp_batch, which is called for
> > > all the key's entries. Thus ensuring the stop machine is called
> > > only once per jump_label key.
> > > 
> > > attached patches:
> > > 1/2 - jump_label,x86: use text_poke_smp_batch for entries update
> > > 	- added jump_label_update_end function which is paired with
> > > 	the key's entries update
> > > 	- jump_label_update_end calls arch_jump_label_update_end which
> > > 	is overloaded by x86 arch and makes the batch update of all the
> > > 	entries queued by arch_jump_label_transform function.
> > > 
> > > 2/2 - jump_label,x86: using static arrays before dynamic allocation is needed
> > > 	- in the first patch, the queue array, which stores jump_label
> > > 	entries is allocated/resized dynamically.
> > > 	- due to the fact that many jump_label entries have low number
> > > 	of callers, it seems appropriate to use static sized array
> > > 	when the update starts and if needed (in case of high number
> > > 	of jump_label entries) allocate/use the dynamic array
> > > 
> > > 
> > > Patch 2/2 and could be ommited if the benefit/complexity ratio
> > > would seem too low.. ;)
> > > 
> > > I tested this on x86 and s390 archs.
> > > 
> > > v2 changes:
> > >  - queueing all entries for single key and process them
> > >    all at one time
> > > 
> > > wrb,
> > > jirka
> > > ---
> > >  arch/x86/kernel/jump_label.c |  177 +++++++++++++++++++++++++++++++++++++++--
> > >  include/linux/jump_label.h   |    1 +
> > >  kernel/jump_label.c          |   16 ++++-
> > >  3 files changed, 183 insertions(+), 11 deletions(-)
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> > hi,
> > 
> > I did jump_label entries statistics on allyesconfig kernel
> > and got following numbers:
> > 
> > callers - keys
> >       1 - 964
> >       2 - 28
> >       3 - 5
> >       4 - 1
> >       6 - 1
> >      11 - 2
> >      12 - 1
> >      14 - 2
> >      17 - 2
> >      21 - 1
> >     170 - 1
> > 
> > 
> > So the maximum is 170 callers and just for one key,
> > which is the key used in trace_module_get function.
> > 
> > Jason suggested we might stay with the static way because for most
> > of the entries the maximum number of callers is up to 21.
> > 
> > I'm attaching the static version for consideration.
> > 
> > wbr,
> > jirka
> > 
> > ---
> > Changing the jump label update code to use batch processing
> > for x86 architectures.
> > 
> > Currently each jump label update calls text_poke_smp for each
> > jump label key entry. Thus one key update ends up calling stop
> > machine multiple times.
> > 
> > This patch is using text_poke_smp_batch, which is called for
> > mmultiple entries at a time.
> > 
> > Added jump_label_update_end function which is paired with
> > the key's entries update.
> > 
> > The jump_label_update_end calls arch_jump_label_update_end
> > (with generic weak definition) which is overloaded by x86
> > arch and makes the batch update of all the entries queued
> > by arch_jump_label_transform function.
> > 
> > The number of entries that can be updated at a single time
> > is set to 30. This number is based on statistics from allyesconfig
> > kernel showing most of the keys having upto 30 callers.
> > 
> > callers - keys
> >       1 - 964
> >       2 - 28
> >       3 - 5
> >       4 - 1
> >       6 - 1
> >      11 - 2
> >      12 - 1
> >      14 - 2
> >      17 - 2
> >      21 - 1
> >     170 - 1
> > 
> > 
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com> 
> > ---
> >  arch/x86/kernel/jump_label.c |   68 ++++++++++++++++++++++++++++++++++++-----
> >  include/linux/jump_label.h   |    1 +
> >  kernel/jump_label.c          |   16 ++++++++-
> >  3 files changed, 74 insertions(+), 11 deletions(-)
> > 
> 
> For me, this version is much simpler (less than half the code size of
> the original patch), while being optimal for 1007/1008 of the keys.
> 
> Acked-by: Jason Baron <jbaron@redhat.com> 
> 
> 
> Thanks.
> 
> > diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> > index 3fee346..bbde5db 100644
> > --- a/arch/x86/kernel/jump_label.c
> > +++ b/arch/x86/kernel/jump_label.c
> > @@ -24,24 +24,74 @@ union jump_code_union {
> >  	} __attribute__((packed));
> >  };
> >  
> > -void arch_jump_label_transform(struct jump_entry *entry,
> > -			       enum jump_label_type type)
> > +struct text_poke_buffer {
> > +	u8 code[JUMP_LABEL_NOP_SIZE];
> > +};
> > +
> > +#define POKE_CNT_MAX 30
> > +
> > +static struct text_poke_param  poke_pars[POKE_CNT_MAX];
> > +static struct text_poke_buffer poke_bufs[POKE_CNT_MAX];
> > +static int poke_cnt;
> > +
> > +static void poke_setup(struct text_poke_param *param, u8 *buf,
> > +		       int enable,
> > +		       struct jump_entry *entry)
> >  {
> > -	union jump_code_union code;
> > +	union jump_code_union *code = (union jump_code_union *) buf;
> >  
> > -	if (type == JUMP_LABEL_ENABLE) {
> > -		code.jump = 0xe9;
> > -		code.offset = entry->target -
> > -				(entry->code + JUMP_LABEL_NOP_SIZE);
> > +	if (enable == JUMP_LABEL_ENABLE) {
> > +		code->jump = 0xe9;
> > +		code->offset = entry->target -
> > +			       (entry->code + JUMP_LABEL_NOP_SIZE);
> >  	} else
> > -		memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
> > +		memcpy(code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
> > +
> > +	param->addr = (void *) entry->code;
> > +	param->opcode = code;
> > +	param->len = JUMP_LABEL_NOP_SIZE;
> > +}
> > +
> > +static void poke_process(void)
> > +{
> >  	get_online_cpus();
> >  	mutex_lock(&text_mutex);
> > -	text_poke_smp((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
> > +
> > +	text_poke_smp_batch(poke_pars, poke_cnt);
> > +	poke_cnt = 0;
> > +
> >  	mutex_unlock(&text_mutex);
> >  	put_online_cpus();
> >  }
> >  
> > +static void poke_end(void)
> > +{
> > +	if (!poke_cnt)
> > +		return;
> > +
> > +	poke_process();
> > +}
> > +
> > +void arch_jump_label_transform(struct jump_entry *entry,
> > +			       enum jump_label_type enable)
> > +{
> > +	if (poke_cnt == POKE_CNT_MAX)
> > +		poke_process();
> > +
> > +	poke_setup(&poke_pars[poke_cnt], poke_bufs[poke_cnt].code,
> > +		   enable, entry);
> > +	poke_cnt++;
> > +}
> > +
> > +/*
> > + * Called after arch_jump_label_transform is called for
> > + * all entries of a single key.
> > + */
> > +void arch_jump_label_update_end(void)
> > +{
> > +	poke_end();
> > +}
> > +
> >  void arch_jump_label_text_poke_early(jump_label_t addr)
> >  {
> >  	text_poke_early((void *)addr, ideal_nops[NOP_ATOMIC5],
> > diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> > index 83e745f..e7a8fa3 100644
> > --- a/include/linux/jump_label.h
> > +++ b/include/linux/jump_label.h
> > @@ -46,6 +46,7 @@ extern void jump_label_unlock(void);
> >  extern void arch_jump_label_transform(struct jump_entry *entry,
> >  				 enum jump_label_type type);
> >  extern void arch_jump_label_text_poke_early(jump_label_t addr);
> > +extern void arch_jump_label_update_end(void);
> >  extern int jump_label_text_reserved(void *start, void *end);
> >  extern void jump_label_inc(struct jump_label_key *key);
> >  extern void jump_label_dec(struct jump_label_key *key);
> > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > index 74d1c09..6657a37 100644
> > --- a/kernel/jump_label.c
> > +++ b/kernel/jump_label.c
> > @@ -125,6 +125,15 @@ void __weak arch_jump_label_text_poke_early(jump_label_t addr)
> >  {
> >  }
> >  
> > +void __weak arch_jump_label_update_end(void)
> > +{
> > +}
> > +
> > +void jump_label_update_end(void)
> > +{
> > +	arch_jump_label_update_end();
> > +}
> > +
> >  static __init int jump_label_init(void)
> >  {
> >  	struct jump_entry *iter_start = __start___jump_table;
> > @@ -244,10 +253,11 @@ static int jump_label_add_module(struct module *mod)
> >  		jlm->next = key->next;
> >  		key->next = jlm;
> >  
> > -		if (jump_label_enabled(key))
> > +		if (jump_label_enabled(key)) {
> >  			__jump_label_update(key, iter, JUMP_LABEL_ENABLE);
> > +			jump_label_update_end();
> > +		}
> >  	}
> > -
> >  	return 0;
> >  }
> >  
> > @@ -376,6 +386,8 @@ static void jump_label_update(struct jump_label_key *key, int enable)
> >  #ifdef CONFIG_MODULES
> >  	__jump_label_mod_update(key, enable);
> >  #endif
> > +
> > +	jump_label_update_end();
> >  }
> >  
> >  #endif
> > -- 
> > 1.7.1
> >