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

Peter Zijlstra
2011-10-13T15:29:55Z

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.

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.

> 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?

---
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