| Message ID | 20180627160604.8154-3-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | add support for relative references in jump tables | expand |
On Wed, Jun 27, 2018 at 06:06:01PM +0200, Ard Biesheuvel wrote: > diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h > index 86ec0652d3b1..aa203dffe72c 100644 > --- a/include/linux/jump_label.h > +++ b/include/linux/jump_label.h > @@ -121,6 +121,32 @@ struct static_key { > #include <asm/jump_label.h> > > #ifndef __ASSEMBLY__ > +#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE > + > +struct jump_entry { > + int code; > + int target; > + int key; > +}; I much prefer you use 'u32' there. > +static void jump_label_swap(void *a, void *b, int size) > +{ > + long delta = (unsigned long)a - (unsigned long)b; > + struct jump_entry *jea = a; > + struct jump_entry *jeb = b; > + struct jump_entry tmp = *jea; > + > + jea->code = jeb->code - delta; > + jea->target = jeb->target - delta; > + jea->key = jeb->key - delta; > + > + jeb->code = tmp.code + delta; > + jeb->target = tmp.target + delta; > + jeb->key = tmp.key + delta; > +} > + > static void > jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) > { > @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) > > size = (((unsigned long)stop - (unsigned long)start) > / sizeof(struct jump_entry)); > - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL); > + sort(start, size, sizeof(struct jump_entry), jump_label_cmp, > + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap > + : NULL); > } That will result in jump_label_swap being an unused symbol for some compile options. Would it not be much nicer to write that like: static void jump_label_swap(void *a, void *b, int size) { struct jump_entry *jea = a, *jeb = b; #ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE long delta = a - b; jea->code += delta; jea->target += delta; jea->key += delta; jeb->code -= delta; jeb->target -= delta; jeb->key -= delta; #else swap(*jea, *jeb); } And then unconditionally use jump_label_swap().
On 28 June 2018 at 10:50, Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Jun 27, 2018 at 06:06:01PM +0200, Ard Biesheuvel wrote: >> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h >> index 86ec0652d3b1..aa203dffe72c 100644 >> --- a/include/linux/jump_label.h >> +++ b/include/linux/jump_label.h >> @@ -121,6 +121,32 @@ struct static_key { >> #include <asm/jump_label.h> >> >> #ifndef __ASSEMBLY__ >> +#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE >> + >> +struct jump_entry { >> + int code; >> + int target; >> + int key; >> +}; > > I much prefer you use 'u32' there. > Actually, they are signed so that would be s32. But yeah, I can change that. > >> +static void jump_label_swap(void *a, void *b, int size) >> +{ >> + long delta = (unsigned long)a - (unsigned long)b; >> + struct jump_entry *jea = a; >> + struct jump_entry *jeb = b; >> + struct jump_entry tmp = *jea; >> + >> + jea->code = jeb->code - delta; >> + jea->target = jeb->target - delta; >> + jea->key = jeb->key - delta; >> + >> + jeb->code = tmp.code + delta; >> + jeb->target = tmp.target + delta; >> + jeb->key = tmp.key + delta; >> +} >> + >> static void >> jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) >> { >> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) >> >> size = (((unsigned long)stop - (unsigned long)start) >> / sizeof(struct jump_entry)); >> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL); >> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp, >> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap >> + : NULL); >> } > > That will result in jump_label_swap being an unused symbol for some > compile options. > No, and isn't that the point of IS_ENABLED()? The compiler sees a reference to jump_label_swap(), so it won't complain about it being unused. > Would it not be much nicer to write that like: > > static void jump_label_swap(void *a, void *b, int size) > { > struct jump_entry *jea = a, *jeb = b; > > #ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE > long delta = a - b; > > jea->code += delta; > jea->target += delta; > jea->key += delta; > > jeb->code -= delta; > jeb->target -= delta; > jeb->key -= delta; > #else > > swap(*jea, *jeb); > } > > And then unconditionally use jump_label_swap(). Meh. I thought IS_ENABLED() was preferred over #ifdef, no? That way, the compiler always sees the code, and simply discards it without complaining if it ends up left unused.
On 28 June 2018 at 11:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 28 June 2018 at 10:50, Peter Zijlstra <peterz@infradead.org> wrote: >> On Wed, Jun 27, 2018 at 06:06:01PM +0200, Ard Biesheuvel wrote: >>> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h >>> index 86ec0652d3b1..aa203dffe72c 100644 >>> --- a/include/linux/jump_label.h >>> +++ b/include/linux/jump_label.h >>> @@ -121,6 +121,32 @@ struct static_key { >>> #include <asm/jump_label.h> >>> >>> #ifndef __ASSEMBLY__ >>> +#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE >>> + >>> +struct jump_entry { >>> + int code; >>> + int target; >>> + int key; >>> +}; >> >> I much prefer you use 'u32' there. >> > > Actually, they are signed so that would be s32. But yeah, I can change that. > >> >>> +static void jump_label_swap(void *a, void *b, int size) >>> +{ >>> + long delta = (unsigned long)a - (unsigned long)b; >>> + struct jump_entry *jea = a; >>> + struct jump_entry *jeb = b; >>> + struct jump_entry tmp = *jea; >>> + >>> + jea->code = jeb->code - delta; >>> + jea->target = jeb->target - delta; >>> + jea->key = jeb->key - delta; >>> + >>> + jeb->code = tmp.code + delta; >>> + jeb->target = tmp.target + delta; >>> + jeb->key = tmp.key + delta; >>> +} >>> + >>> static void >>> jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) >>> { >>> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) >>> >>> size = (((unsigned long)stop - (unsigned long)start) >>> / sizeof(struct jump_entry)); >>> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL); >>> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp, >>> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap >>> + : NULL); >>> } >> >> That will result in jump_label_swap being an unused symbol for some >> compile options. >> > > No, and isn't that the point of IS_ENABLED()? The compiler sees a > reference to jump_label_swap(), so it won't complain about it being > unused. > >> Would it not be much nicer to write that like: >> >> static void jump_label_swap(void *a, void *b, int size) >> { >> struct jump_entry *jea = a, *jeb = b; >> >> #ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE >> long delta = a - b; >> >> jea->code += delta; >> jea->target += delta; >> jea->key += delta; >> >> jeb->code -= delta; >> jeb->target -= delta; >> jeb->key -= delta; >> #else >> >> swap(*jea, *jeb); >> } >> >> And then unconditionally use jump_label_swap(). > > Meh. I thought IS_ENABLED() was preferred over #ifdef, no? That way, > the compiler always sees the code, and simply discards it without > complaining if it ends up left unused. ... and it means the sort() routine will unconditionally perform an indirect function call even if the arch does not require it.
On Thu, Jun 28, 2018 at 11:04:45AM +0200, Ard Biesheuvel wrote: > On 28 June 2018 at 11:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >>> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) > >>> > >>> size = (((unsigned long)stop - (unsigned long)start) > >>> / sizeof(struct jump_entry)); > >>> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL); > >>> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp, > >>> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap > >>> + : NULL); > >>> } > >> > >> That will result in jump_label_swap being an unused symbol for some > >> compile options. > > > > No, and isn't that the point of IS_ENABLED()? The compiler sees a > > reference to jump_label_swap(), so it won't complain about it being > > unused. Ah, ok. I hadn't figured it was quite that smart about it. > > Meh. I thought IS_ENABLED() was preferred over #ifdef, no? Dunno, I just reacted to the proposed code's uglyness :-) > ... and it means the sort() routine will unconditionally perform an > indirect function call even if the arch does not require it. Yeah, not sure I care about that here, this is a one time affair, very far away from any fast paths.
On 28 June 2018 at 11:25, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Jun 28, 2018 at 11:04:45AM +0200, Ard Biesheuvel wrote: >> On 28 June 2018 at 11:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >>> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) >> >>> >> >>> size = (((unsigned long)stop - (unsigned long)start) >> >>> / sizeof(struct jump_entry)); >> >>> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL); >> >>> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp, >> >>> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap >> >>> + : NULL); >> >>> } >> >> >> >> That will result in jump_label_swap being an unused symbol for some >> >> compile options. >> > >> > No, and isn't that the point of IS_ENABLED()? The compiler sees a >> > reference to jump_label_swap(), so it won't complain about it being >> > unused. > > Ah, ok. I hadn't figured it was quite that smart about it. > Yeah. I could use a temp variable to make the indentation less obnoxious, but since this is an opt-in feature, I'd like to preserve the NULL (*swap)() argument for the existing users. >> > Meh. I thought IS_ENABLED() was preferred over #ifdef, no? > > Dunno, I just reacted to the proposed code's uglyness :-) > I will try to come up with something that rhymes, ok? :-) >> ... and it means the sort() routine will unconditionally perform an >> indirect function call even if the arch does not require it. > > Yeah, not sure I care about that here, this is a one time affair, very > far away from any fast paths. > Fair enough.
On Thu, Jun 28, 2018 at 11:29:40AM +0200, Ard Biesheuvel wrote: > On 28 June 2018 at 11:25, Peter Zijlstra <peterz@infradead.org> wrote: > >> > Meh. I thought IS_ENABLED() was preferred over #ifdef, no? > > > > Dunno, I just reacted to the proposed code's uglyness :-) > > > > I will try to come up with something that rhymes, ok? :-) :-) > >> ... and it means the sort() routine will unconditionally perform an > >> indirect function call even if the arch does not require it. > > > > Yeah, not sure I care about that here, this is a one time affair, very > > far away from any fast paths. > > > > Fair enough. I recently had thoughts about doing the sort at link time, but then I figured I didn't care enough to go write the tool to do that. The reason was in that other patch-set that might conflict, that wants to use jump labels _very_ early (for x86), so we had to lift the ideal nops stuff and the cpu feature bits it relies upon to early code. All in all the patch wasn't terrible and that made me completely loose interest about doing the link-time thing again. However, if someone is 'concerned' about the boot time performance, and figures avoiding the sort is somehow worth it, they can look into it.
diff --git a/arch/Kconfig b/arch/Kconfig index 2b8b70820002..22fa3792626e 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -348,6 +348,9 @@ config HAVE_PERF_USER_STACK_DUMP config HAVE_ARCH_JUMP_LABEL bool +config HAVE_ARCH_JUMP_LABEL_RELATIVE + bool + config HAVE_RCU_TABLE_FREE bool diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index 86ec0652d3b1..aa203dffe72c 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -121,6 +121,32 @@ struct static_key { #include <asm/jump_label.h> #ifndef __ASSEMBLY__ +#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE + +struct jump_entry { + int code; + int target; + int key; +}; + +static inline unsigned long jump_entry_code(const struct jump_entry *entry) +{ + return (unsigned long)&entry->code + entry->code; +} + +static inline unsigned long jump_entry_target(const struct jump_entry *entry) +{ + return (unsigned long)&entry->target + entry->target; +} + +static inline struct static_key *jump_entry_key(const struct jump_entry *entry) +{ + unsigned long key = (unsigned long)&entry->key + entry->key; + + return (struct static_key *)(key & ~1UL); +} + +#else struct jump_entry; /* defined by the architecture */ @@ -139,6 +165,8 @@ static inline struct static_key *jump_entry_key(const struct jump_entry *entry) return (struct static_key *)((unsigned long)entry->key & ~1UL); } +#endif + static inline bool jump_entry_is_branch(const struct jump_entry *entry) { return (unsigned long)entry->key & 1UL; diff --git a/kernel/jump_label.c b/kernel/jump_label.c index c3524c9b3004..285eff13ecd1 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -49,6 +49,22 @@ static int jump_label_cmp(const void *a, const void *b) return 0; } +static void jump_label_swap(void *a, void *b, int size) +{ + long delta = (unsigned long)a - (unsigned long)b; + struct jump_entry *jea = a; + struct jump_entry *jeb = b; + struct jump_entry tmp = *jea; + + jea->code = jeb->code - delta; + jea->target = jeb->target - delta; + jea->key = jeb->key - delta; + + jeb->code = tmp.code + delta; + jeb->target = tmp.target + delta; + jeb->key = tmp.key + delta; +} + static void jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) { @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop) size = (((unsigned long)stop - (unsigned long)start) / sizeof(struct jump_entry)); - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL); + sort(start, size, sizeof(struct jump_entry), jump_label_cmp, + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap + : NULL); } static void jump_label_update(struct static_key *key);
To reduce the size taken up by absolute references in jump label entries themselves and the associated relocation records in the .init segment, add support for emitting them as 32-bit relative references instead. Note that this requires some extra care in the sorting routine, given that the offsets change when entries are moved around in the jump_entry table. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/Kconfig | 3 +++ include/linux/jump_label.h | 28 ++++++++++++++++++++ kernel/jump_label.c | 20 +++++++++++++- 3 files changed, 50 insertions(+), 1 deletion(-) -- 2.11.0