Re: [PATCH RFC V4 06/10] jump_label: add arch_jump_label_transform_static() to optimise non-live code updates

[Jason Baron <jbaron@redhat.com>]

On Thu, Oct 13, 2011 at 05:29:17PM +0200, Peter Zijlstra wrote:
> On Thu, 2011-10-13 at 09:54 -0400, Jason Baron wrote:
> > 
> > > So I got myself a little confused wrt the early jump_label_apply_nops()
> > > call and the MODULE_COMING notifiers.
> > > 
> > > It looks to me like jump_label_apply_nops() is called way early and is
> > > in fact called before _any_ of the module code has had a chance of
> > > running. However it simply NOPs out all jump_labels.
> > > 
> > 
> > yes.
> > 
> > > The jump_label_add_module() thing, which is ran on the MODULE_COMING
> > > callback will then set up stuff and do the proper patch-up.
> > > 
> > 
> > yes, only for the enabled ones.
> 
> But we could make it nop out the disabled ones quite trivially.
> 
> > > Now the only bit of the module text that can be ran between those two
> > > calls appears to be the module argument parsing stuff, but since
> > > jump_labels are non-functional it can't rely on them, so why do we do
> > > the early patch up again?
> > > 
> > > 
> > 
> > The 'early patch' is for putting in the 'ideal' no-ops into the module
> > code. These 'ideal' no-ops are discovered at run-time, not boot-time.
> 
> Sure, but since we can't use static_branch() and friends from very early
> module code (arg parsing) anyway, it doesn't matter what NOP they
> encounter, so we might as well do something like the below, right?
> 
> > The code is optimized (hopefully) for the most common case. The
> > jump labels are by nature expected to be off, 
> 
> I actually need them to be either way.. no preference between on or off
> just a means of very _very_ infrequent runtime change in behaviour.
> 

ok, this is a new use case, all the current users are biased with gcc
out-of-lining the infrequent case.

> If we can push jump_label init to before sched_init() all I need is a
> static_branch() without the unlikely() in to avoid GCC out-of-lining the
> branch.
> 

hmmm....the current code (I believe) is biased  b/c gcc sees the
branch as always false, see: arch_static_branch() - its not b/c we have
an unlikely there. Without open coding the label, like we had before
everybody hated, I'll have to play around and see what will create an
unbiased branch...perhaps, somebody has an idea? 

> > and by patching them early
> > like this, at least for x86, we can avoid the stop machine calls. So its
> > the combination of most are expected to be off and no sense to call extra
> > stop machines that lead the code to its present state.
> 
> But we could use arch_jump_label_transform_static because its before we
> actually execute any module text (sans the arg crap) which is
> stomp-machine free, removing that obstacle.
> 
> Or am I confused more?
> 

The MODULE_COMING callback happens *after* the call to flush_module_icache(mod),
so I'm not sure that is safe...

thanks,

-Jason


> ---
>  arch/mips/kernel/module.c  |    3 ---
>  arch/sparc/kernel/module.c |    3 ---
>  arch/x86/kernel/module.c   |    3 ---
>  include/linux/jump_label.h |    6 ------
>  kernel/jump_label.c        |   22 +++++++++++++---------
>  5 files changed, 13 insertions(+), 24 deletions(-)
> 
> Index: linux-2.6/arch/mips/kernel/module.c
> ===================================================================
> --- linux-2.6.orig/arch/mips/kernel/module.c
> +++ linux-2.6/arch/mips/kernel/module.c
> @@ -368,9 +368,6 @@ int module_finalize(const Elf_Ehdr *hdr,
>  	const Elf_Shdr *s;
>  	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
>  
> -	/* Make jump label nops. */
> -	jump_label_apply_nops(me);
> -
>  	INIT_LIST_HEAD(&me->arch.dbe_list);
>  	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
>  		if (strcmp("__dbe_table", secstrings + s->sh_name) != 0)
> Index: linux-2.6/arch/sparc/kernel/module.c
> ===================================================================
> --- linux-2.6.orig/arch/sparc/kernel/module.c
> +++ linux-2.6/arch/sparc/kernel/module.c
> @@ -207,9 +207,6 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		    const Elf_Shdr *sechdrs,
>  		    struct module *me)
>  {
> -	/* make jump label nops */
> -	jump_label_apply_nops(me);
> -
>  	/* Cheetah's I-cache is fully coherent.  */
>  	if (tlb_type == spitfire) {
>  		unsigned long va;
> Index: linux-2.6/arch/x86/kernel/module.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/module.c
> +++ linux-2.6/arch/x86/kernel/module.c
> @@ -194,9 +194,6 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		apply_paravirt(pseg, pseg + para->sh_size);
>  	}
>  
> -	/* make jump label nops */
> -	jump_label_apply_nops(me);
> -
>  	return 0;
>  }
>  
> Index: linux-2.6/include/linux/jump_label.h
> ===================================================================
> --- linux-2.6.orig/include/linux/jump_label.h
> +++ linux-2.6/include/linux/jump_label.h
> @@ -52,7 +52,6 @@ extern int jump_label_text_reserved(void
>  extern void jump_label_inc(struct jump_label_key *key);
>  extern void jump_label_dec(struct jump_label_key *key);
>  extern bool jump_label_enabled(struct jump_label_key *key);
> -extern void jump_label_apply_nops(struct module *mod);
>  
>  #else  /* !HAVE_JUMP_LABEL */
>  
> @@ -97,11 +96,6 @@ static inline bool jump_label_enabled(st
>  {
>  	return !!atomic_read(&key->enabled);
>  }
> -
> -static inline int jump_label_apply_nops(struct module *mod)
> -{
> -	return 0;
> -}
>  #endif	/* HAVE_JUMP_LABEL */
>  
>  #endif	/* _LINUX_JUMP_LABEL_H */
> Index: linux-2.6/kernel/jump_label.c
> ===================================================================
> --- linux-2.6.orig/kernel/jump_label.c
> +++ linux-2.6/kernel/jump_label.c
> @@ -116,9 +116,15 @@ void __weak arch_jump_label_transform_st
>  	arch_jump_label_transform(entry, type);	
>  }
>  
> +static inline enum jump_label_type jump_label_dyn_type(struct jump_label_key *key)
> +{
> +	return jump_label_enabled(key) ? JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE;
> +}
> +
>  static void __jump_label_update(struct jump_label_key *key,
>  				struct jump_entry *entry,
> -				struct jump_entry *stop, int enable)
> +				struct jump_entry *stop, int enable,
> +				void (*transform)(struct jump_entry *, enum jump_label_type))
>  {
>  	for (; (entry < stop) &&
>  	      (entry->key == (jump_label_t)(unsigned long)key);
> @@ -129,7 +135,7 @@ static void __jump_label_update(struct j
>  		 * init code, see jump_label_invalidate_module_init().
>  		 */
>  		if (entry->code && kernel_text_address(entry->code))
> -			arch_jump_label_transform(entry, enable);
> +			transform(entry, enable);
>  	}
>  }
>  
> @@ -147,8 +153,7 @@ void __init jump_label_init(void)
>  		struct jump_label_key *iterk;
>  
>  		iterk = (struct jump_label_key *)(unsigned long)iter->key;
> -		arch_jump_label_transform_static(iter, jump_label_enabled(iterk) ?
> -						 JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
> +		arch_jump_label_transform_static(iter, jump_label_dyn_type(iterk));
>  		if (iterk == key)
>  			continue;
>  
> @@ -193,7 +198,7 @@ static void __jump_label_mod_update(stru
>  
>  		__jump_label_update(key, mod->entries,
>  				    m->jump_entries + m->num_jump_entries,
> -				    enable);
> +				    enable, arch_jump_label_transform);
>  		mod = mod->next;
>  	}
>  }
> @@ -256,9 +261,8 @@ static int jump_label_add_module(struct
>  		jlm->next = key->next;
>  		key->next = jlm;
>  
> -		if (jump_label_enabled(key))
> -			__jump_label_update(key, iter, iter_stop,
> -					    JUMP_LABEL_ENABLE);
> +		__jump_label_update(key, iter, iter_stop, jump_label_dyn_type(key),
> +				arch_jump_label_transform_static);
>  	}
>  
>  	return 0;
> @@ -392,7 +396,7 @@ static void jump_label_update(struct jum
>  #endif
>  	/* if there are no users, entry can be NULL */
>  	if (entry)
> -		__jump_label_update(key, entry, stop, enable);
> +		__jump_label_update(key, entry, stop, enable, arch_jump_label_transform);
>  }
>  
>  #endif
>