Messages in this thread | | | Date | Mon, 23 May 2011 18:45:41 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCHv3] jump_label,x86: make batch update of jump_label entries |
| |
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 > >
| |