jump label v9
Subsystems (13)
cover jump label v9
Hi,
updates for -v9:
*build time detection of gcc 'asm goto' feature via scripts/gcc-goto.sh, which basically tries to compile a simple 'asm goto' c program, and if it succeeds we define CC_HAVE_ASM_GOTO. In this way we detect the 4.5 gcc feature if its backported, as it has been done to 4.4 gcc in fedora.
*misc cleanups and simplifications.
thanks,
-Jason
David S. Miller (1):
sparc64: Add jump_label support
Jason Baron (11):
jump label: base patch
jump label: x86 support
jump label: tracepoint support
jump label: add module support
jump label: move ftrace_dyn_arch_init to common code jump label: sort jump table at build-time
jump label: initialize workqueue tracepoints *before* they are
registered
jump label: jump_label_text_reserved() to reserve our jump points jump label: convert jump label to use a key
jump label: convert dynamic debug to use jump labels. jump label: add docs
Mathieu Desnoyers (1):
jump label: notifier atomic call chain notrace
Documentation/jump-label.txt | 151 +++++++++++++ Makefile | 11 +- arch/Kconfig | 3 + arch/sparc/Kconfig | 1 + arch/sparc/include/asm/jump_label.h | 32 +++ arch/sparc/kernel/Makefile | 2 + arch/sparc/kernel/jump_label.c | 38 +++ arch/sparc/kernel/module.c | 6 + arch/x86/Kconfig | 1 + arch/x86/include/asm/alternative.h | 14 ++ arch/x86/include/asm/jump_label.h | 47 ++++ arch/x86/kernel/Makefile | 2 +- arch/x86/kernel/alternative.c | 72 ++++++- arch/x86/kernel/ftrace.c | 70 +------ arch/x86/kernel/jump_label.c | 47 ++++ arch/x86/kernel/kprobes.c | 3 +- arch/x86/kernel/module.c | 3 + arch/x86/kernel/ptrace.c | 1 + arch/x86/kernel/setup.c | 3 + include/asm-generic/vmlinux.lds.h | 22 ++- include/linux/dynamic_debug.h | 42 ++-- include/linux/jump_label.h | 59 +++++ include/linux/module.h | 5 +- include/linux/tracepoint.h | 7 +- kernel/Makefile | 2 +- kernel/jump_label.c | 426 +++++++++++++++++++++++++++++++++++ kernel/kprobes.c | 3 +- kernel/module.c | 7 + kernel/notifier.c | 6 +- kernel/trace/ftrace.c | 13 +- kernel/trace/trace_workqueue.c | 10 +- kernel/tracepoint.c | 8 + lib/dynamic_debug.c | 42 +---- scripts/Makefile.lib | 11 +- scripts/basic/Makefile | 2 +- scripts/basic/hash.c | 64 ------ scripts/gcc-goto.sh | 5 + scripts/mod/Makefile | 1 + scripts/mod/modpost.c | 73 ++++++- scripts/mod/modpost.h | 1 + 40 files changed, 1077 insertions(+), 237 deletions(-) create mode 100644 Documentation/jump-label.txt create mode 100644 arch/sparc/include/asm/jump_label.h create mode 100644 arch/sparc/kernel/jump_label.c create mode 100644 arch/x86/include/asm/jump_label.h create mode 100644 arch/x86/kernel/jump_label.c create mode 100644 include/linux/jump_label.h create mode 100644 kernel/jump_label.c delete mode 100644 scripts/basic/hash.c create mode 100644 scripts/gcc-goto.sh
PATCH 1/13 jump label v9: notifier atomic call chain notrace
From: Mathieu Desnoyers <[email protected]>
In LTTng, being able to use the atomic notifier from cpu idle entry to ensure the tracer flush the last events in the current subbuffer requires the rcu read-side to be marked "notrace", otherwise it can end up calling back into lockdep and the tracer.
Also apply to the the die notifier.
--- kernel/notifier.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
kernel/notifier.c +3 -3
@@ -148,7 +148,7 @@ int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh,spin_lock_irqsave (&nh->lock, flags); ret =notifier_chain_unregister (&nh->head, n);spin_unlock_irqrestore (&nh->lock, flags); -synchronize_rcu (); +synchronize_sched ();return ret; }EXPORT_SYMBOL_GPL (atomic_notifier_chain_unregister);
@@ -178,9 +178,9 @@ int __kprobes __atomic_notifier_call_chain(struct atomic_notifier_head *nh, {int ret; -rcu_read_lock (); +rcu_read_lock_sched_notrace (); ret =notifier_call_chain (&nh->head, val, v, nr_to_call, nr_calls); -rcu_read_unlock (); +rcu_read_unlock_sched_notrace ();return ret; }EXPORT_SYMBOL_GPL (__atomic_notifier_call_chain);
Signature · 1 line
1.7.1
Re: [PATCH 01/13] jump label v9: notifier atomic call chain notrace
Quoted 7 lines · Jason Baron
From: Mathieu Desnoyers <[email protected]>
In LTTng, being able to use the atomic notifier from cpu idle entry to ensure the tracer flush the last events in the current subbuffer requires the rcu read-side to be marked "notrace", otherwise it can end up calling back into lockdep and the tracer.
Quoted 6 lines
Also apply to the the die notifier.
Signed-off-by Mathieu Desnoyers <[email protected]>Signed-off-by Jason Baron <[email protected]>Reviewed-by Masami Hiramatsu <[email protected]>---
I don't understand the purpose of this patch. The only notifier I see that is used by jmp label is the module notifier, and I don't see how it can do bad things if we don't use the notrace version.
Re: [PATCH 01/13] jump label v9: notifier atomic call chain notrace
Quoted 17 lines · Frederic Weisbecker
Quoted 7 lines · Jason Baron
From: Mathieu Desnoyers <[email protected]>
In LTTng, being able to use the atomic notifier from cpu idle entry to ensure the tracer flush the last events in the current subbuffer requires the rcu read-side to be marked "notrace", otherwise it can end up calling back into lockdep and the tracer.
Quoted 6 lines
Also apply to the the die notifier.
Signed-off-by Mathieu Desnoyers <[email protected]>Signed-off-by Jason Baron <[email protected]>Reviewed-by Masami Hiramatsu <[email protected]>---
Quoted 5 lines
I don't understand the purpose of this patch. The only notifier I see that is used by jmp label is the module notifier, and I don't see how it can do bad things if we don't use the notrace version.
hmmm...yeah, this patch was introduced b/c of the use of the 'int3' notifier when updating the kernel text. We got into a recursive loop without it. However, since the current implementation is using stop_machine() to do text updates, I don't think this patch is necessary until we have the more optimal text updating mechanism in place. I'll re-test without this patch.
thanks,
-Jason
PATCH 2/13 jump label v9: base patch
In reply to Jason Baronbase patch to implement 'jump labeling'. Based on a new 'asm goto' inline assembly gcc mechanism, we can now branch to labels from an 'asm goto' statment. This allows us to create a 'no-op' fastpath, which can subsequently be patched with a jump to the slowpath code. This is useful for code which might be rarely used, but which we'd like to be able to call, if needed. Tracepoints are the current usecase that these are being implemented for.
--- Makefile | 5 + arch/Kconfig | 3 + include/asm-generic/vmlinux.lds.h | 10 ++- include/linux/jump_label.h | 57 ++++++++++++ kernel/Makefile | 2 +- kernel/jump_label.c | 178 +++++++++++++++++++++++++++++++++++++ scripts/gcc-goto.sh | 5 + 7 files changed, 258 insertions(+), 2 deletions(-) create mode 100644 include/linux/jump_label.h create mode 100644 kernel/jump_label.c create mode 100644 scripts/gcc-goto.sh
Makefile +5 -0
@@ -582,6 +582,11 @@ KBUILD_CFLAGS += $(call cc-option,-fno-strict-overflow) # conserve stack if available KBUILD_CFLAGS += $(call cc-option,-fconserve-stack) +# check for 'asm goto' +ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y) + KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO +endif + # Add user supplied CPPFLAGS, AFLAGS and CFLAGS as the last assignments # But warn user when we do so warn-assign = \
arch/Kconfig +3 -0
@@ -158,4 +158,7 @@ config HAVE_PERF_EVENTS_NMI subsystem. Also has support for calculating CPU cycle events to determine how many clock cycles in a given period. +config HAVE_ARCH_JUMP_LABEL + bool + source "kernel/gcov/Kconfig"
include/asm-generic/vmlinux.lds.h +9 -1
@@ -167,7 +167,8 @@ BRANCH_PROFILE() \ TRACE_PRINTKS() \ FTRACE_EVENTS() \ -TRACE_SYSCALLS () + TRACE_SYSCALLS() \ +JUMP_TABLE () \/* * Data section helpers
@@ -206,6 +207,7 @@ *(__vermagic)/* Kernel version magic */ \ *(__markers_strings)/* Markers: strings */ \ *(__tracepoints_strings)/* Tracepoints: strings */ \ + *(__jump_strings)/* Jump: strings */ \ } \ \ .rodata1 : AT(ADDR(.rodata1) - LOAD_OFFSET) { \
@@ -557,6 +559,12 @@#define BUG_TABLE #endif +#define JUMP_TABLE () \ + . = ALIGN(64); \ + VMLINUX_SYMBOL(__start___jump_table) = .; \ + *(__jump_table) \ + VMLINUX_SYMBOL(__stop___jump_table) = .; \ +#ifdef CONFIG_PM_TRACE #define TRACEDATA \ . = ALIGN(4); \
include/linux/jump_label.h new file +57 -0
57 lines not on this page · raw message
kernel/Makefile +1 -1
@@ -10,7 +10,7 @@ obj-y = sched.o fork.o exec_domain.o panic.o printk.o \ kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \ hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \ notifier.o ksysfs.o pm_qos_params.o sched_clock.o cred.o \ - async.o range.o + async.o range.o jump_label.o obj-$(CONFIG_HAVE_EARLY_RES) += early_res.o obj-y += groups.o
kernel/jump_label.c new file +178 -0
178 lines not on this page · raw message
scripts/gcc-goto.sh new file +5 -0
@@ -0,0 +1,5 @@ +#!/bin/sh +# Test for gcc 'asm goto' suport +# Copyright (C) 2010, Jason Baron <[email protected]> + +echo "int main(void) { entry: asm goto (\"\"::::entry); return 0; }" | $1 -x c - -o /dev/null >/dev/null 2>&1 && echo "y"
Signature · 1 line
1.7.1
Re: [PATCH 02/13] jump label v9: base patch
Quoted 9 lines · Jason Baron
kernel/jump_label.c new file +9
-0+static int build_jump_label_hashtable (struct jump_entry *start,struct jump_entry *stop) +{ +struct jump_entry *iter, *iter_begin; +struct jump_label_entry *entry; +int count; + +sort_jump_label_entries (start, stop); + iter = start; +while (iter < stop) {
kernel/jump_label.c line 122
Quoted 10 lines
kernel/jump_label.c new file +10
-0+ entry =get_jump_label_entry ((char *)iter->name); +if (!entry) { + iter_begin = iter; + count =0 ; +while ((iter < stop) && + (strcmp ((char *)iter->name, + (char *)iter_begin->name) ==0 )) { + iter++; + count++; + }
So, you can have multiple entries with the same name? How can that happen in fact?
kernel/jump_label.c line 129
Quoted 7 lines
kernel/jump_label.c new file +7
-0+ entry =add_jump_label_entry ((char *)iter_begin->name, + count, iter_begin); +if (IS_ERR (entry)) +return PTR_ERR (entry); +continue ; + } +WARN (1 ,KERN_ERR "build_jump_hashtable: unexpected entry!\n" );
It seems you are going to endless loop in this fail case.
Quoted 22 lines
kernel/jump_label.c new file +22
-0+ } +return 0 ; +} + +/*** +* jump_label_update - update jump label text +* @name - name of the jump label +* @type - enum set to JUMP_LABEL_ENABLE or JUMP_LABEL_DISABLE +* +* Will enable/disable the jump for jump label @name, depending on the +* value of @type. +* +*/ + +void jump_label_update (const char *name,enum jump_label_type type) +{ +struct jump_entry *iter; +struct jump_label_entry *entry; +struct hlist_node *module_node; +struct jump_label_module_entry *e_module; +int count; +
kernel/jump_label.c line 161
Quoted 10 lines
kernel/jump_label.c new file +10
-0+mutex_lock (&jump_label_mutex); + entry =get_jump_label_entry (name); +if (entry) { + count = entry->nr_entries; + iter = entry->table; +while (count--) { +if (kernel_text_address (iter->code)) +arch_jump_label_transform (iter, type); + iter++; + }
So, this is going to patch multiple times the same value on the same address in case you have multiple entries for the same name?
That look weird.
BTW, if you can't find the entry, you should perhaps propagate an error.
kernel/jump_label.c line 166
Quoted 5 lines
kernel/jump_label.c new file +5
-0+ } +mutex_unlock (&jump_label_mutex); +} + +static int init_jump_label (void )
This can be __init.
Re: [PATCH 02/13] jump label v9: base patch
Quoted 18 lines · Frederic Weisbecker
Quoted 17 lines · Jason Baron
kernel/jump_label.c new file +17
-0+static int build_jump_label_hashtable (struct jump_entry *start,struct jump_entry *stop) +{ +struct jump_entry *iter, *iter_begin; +struct jump_label_entry *entry; +int count; + +sort_jump_label_entries (start, stop); + iter = start; +while (iter < stop) { + entry =get_jump_label_entry ((char *)iter->name); +if (!entry) { + iter_begin = iter; + count =0 ; +while ((iter < stop) && + (strcmp ((char *)iter->name, + (char *)iter_begin->name) ==0 )) { + iter++;
Quoted 10 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ count++; + }So, you can have multiple entries with the same name? How can that happen in fact?
this is the case where a single tracepoint such as kmalloc(), is used in all over the kernel. So, there is one name or key value associated with a kmalloc tracepoint. however, we have to patch the jump or nop into a bunch of places in the kernel text.
Quoted 14 lines
kernel/jump_label.c line 129
Quoted 9 lines
kernel/jump_label.c new file +7
-0+ entry =add_jump_label_entry ((char *)iter_begin->name, + count, iter_begin); +if (IS_ERR (entry)) +return PTR_ERR (entry); +continue ; + } +WARN (1 ,KERN_ERR "build_jump_hashtable: unexpected entry!\n" );It seems you are going to endless loop in this fail case.
agreed. I need to stick that 'WARN' into the else clause of "if (!entry)" and return an error.
Quoted 40 lines
Quoted 24 lines
kernel/jump_label.c new file +22
-0+ } +return 0 ; +} + +/*** +* jump_label_update - update jump label text +* @name - name of the jump label +* @type - enum set to JUMP_LABEL_ENABLE or JUMP_LABEL_DISABLE +* +* Will enable/disable the jump for jump label @name, depending on the +* value of @type. +* +*/ + +void jump_label_update (const char *name,enum jump_label_type type) +{ +struct jump_entry *iter; +struct jump_label_entry *entry; +struct hlist_node *module_node; +struct jump_label_module_entry *e_module; +int count; +kernel/jump_label.c line 161
Quoted 10 lines
kernel/jump_label.c new file +10
-0+mutex_lock (&jump_label_mutex); + entry =get_jump_label_entry (name); +if (entry) { + count = entry->nr_entries; + iter = entry->table; +while (count--) { +if (kernel_text_address (iter->code)) +arch_jump_label_transform (iter, type); + iter++; + }So, this is going to patch multiple times the same value on the same address in case you have multiple entries for the same name?
Quoted 1 line
That look weird.
no. as mentioned before, there are multiple text addresses potentially associated with a single jump label conditional variable. So we need to patch all of them.
Quoted 5 lines
BTW, if you can't find the entry, you should perhaps propagate an error.
Quoted 10 lines
kernel/jump_label.c line 166
Quoted 5 lines
kernel/jump_label.c new file +5
-0+ } +mutex_unlock (&jump_label_mutex); +} + +static int init_jump_label (void )This can be
__init.
ok.
thanks for the review.
-Jason
Re: [PATCH 02/13] jump label v9: base patch
Quoted 13 lines · Jason Baron
Quoted 12 lines
kernel/jump_label.c line 129
Quoted 7 lines
kernel/jump_label.c new file +7
-0+ entry =add_jump_label_entry ((char *)iter_begin->name, + count, iter_begin); +if (IS_ERR (entry)) +return PTR_ERR (entry); +continue ; + } +WARN (1 ,KERN_ERR "build_jump_hashtable: unexpected entry!\n" );It seems you are going to endless loop in this fail case.
Quoted 2 lines
agreed. I need to stick that 'WARN' into the else clause of "if (!entry)" and return an error.
and make that WARN_ONCE() as well. If it ever triggers it should show up once and not compound the bug with spamming the console and the logs.
Thanks,
Ingo
Re: [PATCH 02/13] jump label v9: base patch
In reply to Jason BaronQuoted 30 lines · Jason Baron
Quoted 18 lines · Frederic Weisbecker
Quoted 17 lines · Jason Baron
kernel/jump_label.c new file +17
-0+static int build_jump_label_hashtable (struct jump_entry *start,struct jump_entry *stop) +{ +struct jump_entry *iter, *iter_begin; +struct jump_label_entry *entry; +int count; + +sort_jump_label_entries (start, stop); + iter = start; +while (iter < stop) { + entry =get_jump_label_entry ((char *)iter->name); +if (!entry) { + iter_begin = iter; + count =0 ; +while ((iter < stop) && + (strcmp ((char *)iter->name, + (char *)iter_begin->name) ==0 )) { + iter++;Quoted 10 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ count++; + }So, you can have multiple entries with the same name? How can that happen in fact?
Quoted 4 lines
this is the case where a single tracepoint such as
kmalloc(), is used in all over the kernel. So, there is one name or key value associated with a kmalloc tracepoint. however, we have to patch the jump or nop into a bunch of places in the kernel text.
Ok, sounds fair then.
Thanks.
Re: [PATCH 02/13] jump label v9: base patch
In reply to Jason Baroninclude/asm-generic/vmlinux.lds.h line 567
Quoted 6 lines · Jason Baron
include/asm-generic/vmlinux.lds.h +6
-0+#define JUMP_TABLE () \ + . = ALIGN(64); \ + VMLINUX_SYMBOL(__start___jump_table) = .; \ + *(__jump_table) \ + VMLINUX_SYMBOL(__stop___jump_table) = .; \ +
Why does it need to be aligned to 64?
Thanks.
Peter Zijlstra
maintainerRe: [PATCH 02/13] jump label v9: base patch
In reply to Jason BaronQuoted 10 lines · Jason Baron
kernel/jump_label.c new file +10
-0+static void sort_jump_label_entries (struct jump_entry *start,struct jump_entry *stop) +{ +int swapped =0 ; +struct jump_entry *iter; +struct jump_entry *iter_next; + + do { + swapped = 0; + iter = start; + iter_next = start;
kernel/jump_label.c line 61
Quoted 10 lines
kernel/jump_label.c new file +10
-0+ iter_next++; +for (; iter_next < stop; iter++, iter_next++) { +if (strcmp ((char *)iter->name, + (char *)iter_next->name) >0 ) { +swap_jump_label_entries (iter, iter_next); + swapped =1 ; + } + } + }while (swapped ==1 ); +}
Would lib/sort.c be of any help?
Peter Zijlstra
maintainerRe: [PATCH 02/13] jump label v9: base patch
In reply to Jason BaronQuoted 10 lines · Jason Baron
(quoted diff) +8
-0+#define DEFINE_JUMP_LABEL(name) \ + const char __jlstrtab_##name[] \ + __used __attribute__((section("__jump_strings"))) = #name; +#define JUMP_LABEL(tag, label, cond) \ +do { \ + if (unlikely(cond)) \ + goto label; \ +} while (0)
Review
Quoted 10 lines
(quoted diff) +10
-0+#define JUMP_LABEL(tag, label, cond) \ + do { \ + extern const char __jlstrtab_##tag[]; \ + asm goto("1:" \ + JUMP_LABEL_NOP \ + ".pushsection __jump_table, \"a\" \n\t"\ + _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \ + ".popsection \n\t" \ + : : "i" (__jlstrtab_##tag) : : label);\ + } while (0)
s/tag/name/ ?
PATCH 3/13 jump label v9: x86 support
In reply to Jason Baronadd x86 support for jump label. I'm keeping this patch separate so its clear to arch maintainers what was required for x86 support this new feature. hopefully, it wouldn't be too painful for other arches.
--- arch/x86/Kconfig | 1 + arch/x86/include/asm/jump_label.h | 48 +++++++++++++++++++++++++++++ arch/x86/kernel/Makefile | 2 +- arch/x86/kernel/jump_label.c | 60 +++++++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 1 deletions(-) create mode 100644 arch/x86/include/asm/jump_label.h create mode 100644 arch/x86/kernel/jump_label.c
arch/x86/Kconfig +1 -0
@@ -59,6 +59,7 @@ config X86 select ANON_INODES select HAVE_ARCH_KMEMCHECK select HAVE_USER_RETURN_NOTIFIER + select HAVE_ARCH_JUMP_LABEL if !CC_OPTIMIZE_FOR_SIZE config INSTRUCTION_DECODER def_bool (KPROBES || PERF_EVENTS)
arch/x86/include/asm/jump_label.h new file +48 -0
48 lines not on this page · raw message
arch/x86/kernel/Makefile +1 -1
@@ -32,7 +32,7 @@ GCOV_PROFILE_paravirt.o := n obj-y := process_$(BITS).o signal.o entry_$(BITS).o obj-y += traps.o irq.o irq_$(BITS).o dumpstack_$(BITS).o obj-y += time.o ioport.o ldt.o dumpstack.o -obj-y += setup.o x86_init.o i8259.o irqinit.o +obj-y += setup.o x86_init.o i8259.o irqinit.o jump_label.o obj-$(CONFIG_X86_VISWS) += visws_quirks.o obj-$(CONFIG_X86_32) += probe_roms_32.o obj-$(CONFIG_X86_32) += sys_i386_32.o i386_ksyms_32.o
arch/x86/kernel/jump_label.c new file +60 -0
60 lines not on this page · raw message
Signature · 1 line
1.7.1
Peter Zijlstra
maintainerRe: [PATCH 03/13] jump label v9: x86 support
Quoted 1 line · Jason Baron
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZE
That deserves a comment somewhere, it basically makes OPTIMIZE_FOR_SIZE useless...
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 5 lines · Peter Zijlstra
Quoted 1 line · Jason Baron
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...
Hm, we need more than a comment for that - distros enable CC_OPTIMIZE_FOR_SIZE all the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?
Thanks,
Ingo
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 4 lines · Ingo Molnar
Quoted 1 line · Peter Zijlstra
On Wed, 2010-06-09 at 17:39 -0400, Jason Baron wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?
Actually the big distros (RHEL, SLES) disable it all the time now. It costs you in some benchmarks. The code generated by -Os is often terrible.
Nearly everytime I investigate some bad asm code being generated by gcc it goes away when that flag is disabled.
A much better to get smaller kernel images is to do more __cold annotations for slow paths. Newer gcc will then simply only do -Os for these functions.
It's already done for __init.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
Peter Zijlstra
maintainerRe: [PATCH 03/13] jump label v9: x86 support
Quoted 1 line · Andi Kleen
The code generated by -Os is often terrible.
Is anybody on the gcc side of things looking into curing that?
I mean, what's the point of having an -Os if its useless in practise.
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 4 lines · Peter Zijlstra
Quoted 1 line · Andi Kleen
The code generated by -Os is often terrible.
Is anybody on the gcc side of things looking into curing that?
The problem is that the smallest code is often terrible. You could often be much better with spending a few more bytes. But -Os means "smallest"
On the other hand -Os could be likely made smaller (it often still is not very good), but I fear that would make things even worse.
We probably would need a -Osmall-but-not-terrible or so, but that's not there.
Quoted 2 lines
I mean, what's the point of having an -Os if its useless in practise.
I think __hot / __cold but keeping the default at -O2 is a better approach anyways. Hot paths should be -O2. It just needs some more work.
It already works for __init/__exit at least.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
Re: [PATCH 03/13] jump label v9: x86 support
In reply to Andi KleenQuoted 5 lines · Andi Kleen
Quoted 3 lines
Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?Actually the big distros (RHEL, SLES) disable it all the time now. [...]
Not all the big distros, Fedora certainly doesnt.
Quoted 1 line
[...] It costs you in some benchmarks. [...]
Microbenchmarks mostly, see below.
Quoted 1 line
The code generated by -Os is often terrible.
But it results in a kernel .text that is ~30% smaller, so considering the fact that most real-life kernel code execution is instruction-cache-cold it's generally a win.
But micro-benchmarks are instruction-cache-hot so that's where the pressure to remove the flag comes from.
Quoted 2 lines
Nearly everytime I investigate some bad asm code being generated by gcc it goes away when that flag is disabled.
That is not fixing anything, it is working bugs around.
Quoted 2 lines
A much better to get smaller kernel images is to do more
__coldannotations for slow paths. Newer gcc will then simply only do -Os for these functions.
That's an opt-in method and we cannot reach the kinds of 30% code size reductions that -Os can achieve. Most code in the kernel is not cache-hot, even on microbenchmarks.
A much better model would be to actively mark hot codepaths with a __hot attribute instead. Then the code size difference can be considered on a case by case basis.
And where GCC produces indefensibly crap code there GCC needs to be fixed. Crap code often increases size so the fix would increase the efficiency of -Os.
Thanks,
Ingo
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 4 lines · Ingo Molnar
Quoted 2 lines
[...] It costs you in some benchmarks. [...]
Microbenchmarks mostly, see below.
I didn't make these decisions, but I assume who made them had good reasons and enough data on larger benchmarks too.
Quoted 6 lines
Quoted 2 lines
A much better to get smaller kernel images is to do more
__coldannotations for slow paths. Newer gcc will then simply only do -Os for these functions.That's an opt-in method and we cannot reach the kinds of 30% code size reductions that -Os can achieve. Most code in the kernel is not cache-hot, even on microbenchmarks.
Maybe, maybe not. But yes it can be approached from both ways.
Personally I would prefer to simply write less bloated code to get code reductions. Simpler code is often faster too.
Quoted 4 lines
A much better model would be to actively mark hot codepaths with a
__hotattribute instead. Then the code size difference can be considered on a case by case basis.
Yes that works too for those who still use -Os.
e.g. marking the scheduler and a few mm hot paths this way would certain make sense.
Quoted 4 lines
And where GCC produces indefensibly crap code there GCC needs to be fixed. Crap code often increases size so the fix would increase the efficiency of -Os.
In some cases agreed, but common cases it's really: you asked for the smallest you got it, even if it's slow. It's not -Odwim.
One standard example here is a division by constant. The shortest way is using DIVI/IDIV if it's not 2^n and small enough, but it's really quite slow in hardware. If you spend a few more bytes you can do much better for a wide range of constants.
Most likely we would need a new -O flag to avoid such cases.
BTW I experimented with marking a few common cases like this (e.g. time unit conversion) hot, but gcc currently has trouble with __hot on inlines. So you would always need to mark the caller.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 9 lines · Andi Kleen
Quoted 7 lines
Quoted 3 lines
A much better to get smaller kernel images is to do more
__coldannotations for slow paths. Newer gcc will then simply only do -Os for these functions.That's an opt-in method and we cannot reach the kinds of 30% code size reductions that -Os can achieve. Most code in the kernel is not cache-hot, even on microbenchmarks.
Maybe, maybe not. But yes it can be approached from both ways.
You dont seem to have understood my point: there's a big difference between an opt-in and an opt-out model.
What you are arguing for is a 'bloaty code generator by default' model and that model sucks.
Trying to achieve reductions by opt-in marking functions as a 'please reduce it' __cold marker is a losing battle: most new kernel code is 'cold' and should be reduced, yet most new code does not (and will not) come with __cold markers.
The proper model is to assume that everything should be conservatively size-reduced (because, almost by definition, 90% of new kernel code should stay small and should stay out of the way), and where benchmarks+importance proves it we can allow bloatier code generator via __hot.
Important codepaths can get __hot annotations just as much as they are receiving 'inline' optimizations and other kinds of hand-tuning attention.
Quoted 2 lines
Personally I would prefer to simply write less bloated code to get code reductions. Simpler code is often faster too.
You are posing this as an if-else choice, while in reality both should be
done: the best result is to write simpler/faster code _and_ to have a compact-by-default code generator too ...
Quoted 8 lines
Quoted 3 lines
A much better model would be to actively mark hot codepaths with a
__hotattribute instead. Then the code size difference can be considered on a case by case basis.Yes that works too for those who still use -Os.
e.g. marking the scheduler and a few mm hot paths this way would certain make sense.
Possibly, but not without substantiating the rather vague statements you have made so far.
If you are sending such per function annotation patches then you need to come up with actual hard numbers as well. One convenient way to measure such things is a before/after "perf stat --repeat" run - as the noise estimations can be compared and we can see whether there's a provable effect. (And, of course, disassembly of GCC suckage is helpful as well.)
Thanks,
Ingo
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 2 lines
What you are arguing for is a 'bloaty code generator by default' model and that model sucks.
I am arguing for a "non sucky code by default" model.
It is widely known that "sucky code by default" sucks already, that is why the big distros made their choice.
Anyways luckily the default is all config options so we don't need to agree on this (and the best choice likely varies by workload too)
Quoted 2 lines
Possibly, but not without substantiating the rather vague statements you have made so far.
Yes, more data with recent builds is needed for concrete changes.
BTW afaik the "icache over everything" model was never really substantiated by all that much data either, just somehow it became dogma.
I must say I was a bit burned by doing annotations -- i added unlikely() originally and as far as I can see most unlikely()s are quite useless today because they do nothing the compiler doesn't do already so I would prefer to not repeat that.
So my personal preference is actually less annotations over more.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
Steven Rostedt
maintainerRe: [PATCH 03/13] jump label v9: x86 support
In reply to Andi KleenQuoted 14 lines · Andi Kleen
Quoted 4 lines · Ingo Molnar
Quoted 1 line · Peter Zijlstra
On Wed, 2010-06-09 at 17:39 -0400, Jason Baron wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?
Quoted 5 lines
Actually the big distros (RHEL, SLES) disable it all the time now. It costs you in some benchmarks. The code generated by -Os is often terrible.
Nearly everytime I investigate some bad asm code being generated by gcc it goes away when that flag is disabled.
I agree to the above statement. It was this option that first caused the crazy mcount prefix code that totally broke the function tracer on i386. With this option, some functions would send a copy of the return address to mcount and not an actual pointer to the return address on stack. This would confuse the function tracer and we ended up with a kernel panic.
-Os really needs to be fixed.
-- Steve
Re: [PATCH 03/13] jump label v9: x86 support
In reply to Ingo MolnarQuoted 8 lines · Ingo Molnar
Quoted 5 lines · Peter Zijlstra
Quoted 1 line · Jason Baron
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...
Quoted 8 lines
Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?Thanks,
Ingo
When I tested 'jump label' with CC_OPTIMIZE_FOR_SIZE, I saw a small performance drop , b/c there is less block re-ordering happening. There was a small gcc patch that Richard Henderson wrote to allow block re-ordering while still taking size into account. However, it did increase the text size somewhat from what current optimize for size does. This small patch combined with jump labels resulted in the expected performance gains. However, to keep the size to what it is now a much more involved gcc patch would be required.
Thus, until this additional gcc optimization is done, I've posted this as depends on !CC_OPTIMIZE_FOR_SIZE. If this gcc work is done, we can lift this restriction. As mentioned, a number of large distros are compiled !CC_OPTIMIZE_FOR_SIZE (rhel, sles), so we should still get a lot of benefit from this. Also, if people find these patches useful we might create some more impetus for the gcc work. So i see this as an iterative thing.
thanks,
-Jason
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 13 lines · * Jason Baron ([email protected]) wrote:
Quoted 12 lines · Ingo Molnar
Quoted 5 lines · Peter Zijlstra
Quoted 1 line · Jason Baron
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?
Quoted 7 lines
Quoted 4 lines
Thanks,
Ingo
When I tested 'jump label' with
CC_OPTIMIZE_FOR_SIZE, I saw a small performance drop , b/c there is less block re-ordering happening.
Is this a performance drop compared to a jump-label-less kernel or compared to -O2 kernel compiled with jump labels ? Or both ?
Mathieu
Signature · 4 lines
Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 19 lines · Mathieu Desnoyers
Quoted 18 lines · * Jason Baron ([email protected]) wrote:
Quoted 16 lines · Ingo Molnar
Quoted 5 lines · Peter Zijlstra
Quoted 1 line · Jason Baron
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?Thanks,
Ingo
Quoted 8 lines
Quoted 2 lines
When I tested 'jump label' with
CC_OPTIMIZE_FOR_SIZE, I saw a small performance drop , b/c there is less block re-ordering happening.Is this a performance drop compared to a jump-label-less kernel or compared to -O2 kernel compiled with jump labels ? Or both ?
Mathieu
Hi Mathieu,
So I'm quoting tbench benchmark here. The performance drop was jump label vs. all jump label patches backed out on -Os. If we move to -02, both the no jump label patches and the jump label patches applied are faster than all jump label patches backed out on -Os.
so:
jump labels -02 > no jump labels -02 > no jump labels -0s > jump lables -Os
thanks,
-Jason
H. Peter Anvin
reviewerRe: [PATCH 03/13] jump label v9: x86 support
In reply to Ingo MolnarQuoted 4 lines · Ingo Molnar
Quoted 1 line · Peter Zijlstra
On Wed, 2010-06-09 at 17:39 -0400, Jason Baron wrote:
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?
Actually the current reports from the gcc community is that gcc 4.5.0 + -Os produces a broken kernel even without asm goto:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44129
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Re: [PATCH 03/13] jump label v9: x86 support
Quoted 10 lines · H. Peter Anvin
Quoted 9 lines · Ingo Molnar
Quoted 5 lines · Peter Zijlstra
Quoted 1 line · Jason Baron
+ select
HAVE_ARCH_JUMP_LABELif !CC_OPTIMIZE_FOR_SIZEThat deserves a comment somewhere, it basically makes
OPTIMIZE_FOR_SIZEuseless...
Quoted 8 lines
Quoted 3 lines
Hm, we need more than a comment for that - distros enable
CC_OPTIMIZE_FOR_SIZEall the time, for the massive kernel image (and hotpath cache footprint) savings. Is this fixable?Actually the current reports from the gcc community is that gcc 4.5.0 + -Os produces a broken kernel even without asm goto:
Well, most distros havent switched to gcc 4.5 yet (even rawhide is still on 4.4 + backports) and i guess the lack of testing shows. New GCC versions have a long history of causing bugs in the kernel.
Thanks,
Ingo
Peter Zijlstra
maintainerRe: [PATCH 03/13] jump label v9: x86 support
In reply to Jason Baronarch/x86/include/asm/jump_label.h line 26
Quoted 10 lines · Jason Baron
arch/x86/include/asm/jump_label.h new file +10
-0+#define JUMP_LABEL (tag, label, cond) \ + do { \ + extern const char __jlstrtab_##tag[]; \ + asm goto("1:" \ + JUMP_LABEL_NOP \ + ".pushsection __jump_table, \"a\" \n\t"\ + _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \ + ".popsection \n\t" \ + : : "i" (__jlstrtab_##tag) : : label);\ + } while (0)
Would it make sense to have that macro at least evaluate cond in some way?
Peter Zijlstra
maintainerRe: [PATCH 03/13] jump label v9: x86 support
In reply to Jason BaronQuoted 3 lines · Jason Baron
add x86 support for jump label. I'm keeping this patch separate so its clear to arch maintainers what was required for x86 support this new feature. hopefully, it wouldn't be too painful for other arches.
Well patch 10 really rather spoils that, doesn't it?
PATCH 4/13 jump label v9: tracepoint support
In reply to Jason BaronMake use of the jump label infrastructure for tracepoints.
--- include/linux/tracepoint.h | 8 ++++++-- kernel/tracepoint.c | 8 ++++++++ 2 files changed, 14 insertions(+), 2 deletions(-)
include/linux/tracepoint.h +6 -2
@@ -16,6 +16,7 @@#include <linux/types.h> #include <linux/rcupdate.h> +#include <linux/jump_label.h> struct module ;struct tracepoint ;
@@ -144,7 +145,9 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin, extern struct tracepoint __tracepoint_##name; \ static inline void trace_##name(proto) \ { \ - if (unlikely(__tracepoint_##name.state)) \ + JUMP_LABEL(&__tracepoint_##name, do_trace, __tracepoint_##name.state);\ + return; \ +do_trace: \ __DO_TRACE(&__tracepoint_##name, \ TP_PROTO(data_proto), \ TP_ARGS(data_args)); \
@@ -171,7 +174,8 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin, __attribute__((section("__tracepoints_strings"))) = #name; \ struct tracepoint __tracepoint_##name \ __attribute__((section("__tracepoints"), aligned(32))) = \ - { __tpstrtab_##name, 0, reg, unreg, NULL } + { __tpstrtab_##name, 0, reg, unreg, NULL }; \ + DEFINE_JUMP_LABEL(name);#define DEFINE_TRACE (name) \ DEFINE_TRACE_FN(name, NULL, NULL);
kernel/tracepoint.c +8 -0
@@ -25,6 +25,7 @@#include <linux/err.h> #include <linux/slab.h> #include <linux/sched.h> +#include <linux/jump_label.h> extern struct tracepoint __start___tracepoints[];extern struct tracepoint __stop___tracepoints[];
@@ -263,6 +264,10 @@ static void set_tracepoint(struct tracepoint_entry **entry,* is used. */ rcu_assign_pointer (elem->funcs, (*entry)->funcs); +if (!elem->state && active) +enable_jump_label (elem->name); +if (elem->state && !active) +disable_jump_label (elem->name); elem->state = active; }
@@ -277,6 +282,9 @@ static void disable_tracepoint(struct tracepoint *elem)if (elem->unregfunc && elem->state) elem->unregfunc (); +if (elem->state) +disable_jump_label (elem->name); + elem->state =0 ;rcu_assign_pointer (elem->funcs,NULL ); }
Signature · 1 line
1.7.1
Peter Zijlstra
maintainerRe: [PATCH 04/13] jump label v9: tracepoint support
Review
Quoted 6 lines · Jason Baron
(quoted diff) +4
-0+ JUMP_LABEL(&__tracepoint_##name, do_trace, __tracepoint_##name.state);\ +#define JUMP_LABEL(tag, label, cond) \ + do { \ + extern const char __jlstrtab_##tag[]; \
So that results in:
extern const char __jlstrtab_&__tracepoint_FOO[]; ?
PATCH 5/13 jump label v9: add module support
In reply to Jason BaronAdd support for 'jump label' for modules.
--- arch/x86/kernel/ptrace.c | 1 + include/linux/jump_label.h | 3 +- include/linux/module.h | 5 +- kernel/jump_label.c | 136 ++++++++++++++++++++++++++++++++++++++++++++ kernel/module.c | 7 ++ 5 files changed, 150 insertions(+), 2 deletions(-)
arch/x86/kernel/ptrace.c +1 -0
@@ -21,6 +21,7 @@#include <linux/signal.h> #include <linux/perf_event.h> #include <linux/hw_breakpoint.h> +#include <linux/module.h> #include <asm/uaccess.h> #include <asm/pgtable.h>
include/linux/jump_label.h +2 -1
@@ -18,7 +18,8 @@ extern struct jump_entry __stop___jump_table[];#define DEFINE_JUMP_LABEL (name) \ const char __jlstrtab_##name[] \ - __used __attribute__((section("__jump_strings"))) = #name; + __used __attribute__((section("__jump_strings"))) = #name; \ + EXPORT_SYMBOL_GPL(__jlstrtab_##name);extern void arch_jump_label_transform (struct jump_entry *entry,enum jump_label_type type);
include/linux/module.h +4 -1
@@ -350,7 +350,10 @@ struct modulestruct tracepoint *tracepoints;unsigned int num_tracepoints;#endif - +#ifdef HAVE_JUMP_LABEL +struct jump_entry *jump_entries; +unsigned int num_jump_entries; +#endif #ifdef CONFIG_TRACING const char **trace_bprintk_fmt_start;unsigned int num_trace_bprintk_fmt;
kernel/jump_label.c +136 -0
152 lines not on this page · raw message
kernel/module.c +7 -0
@@ -55,6 +55,7 @@#include <linux/async.h> #include <linux/percpu.h> #include <linux/kmemleak.h> +#include <linux/jump_label.h> #define CREATE_TRACE_POINTS #include <trace/events/module.h>
@@ -2405,6 +2406,12 @@ static noinline struct module *load_module(void __user *umod,sizeof (*mod->tracepoints), &mod->num_tracepoints);#endif +#ifdef HAVE_JUMP_LABEL + mod->jump_entries =section_objs (hdr, sechdrs, secstrings, +"__jump_table" , +sizeof (*mod->jump_entries), + &mod->num_jump_entries); +#endif #ifdef CONFIG_EVENT_TRACING mod->trace_events = section_objs(hdr, sechdrs, secstrings, "_ftrace_events",
Signature · 1 line
1.7.1
PATCH 6/13 jump label v9: move ftrace_dyn_arch_init to common code
In reply to Jason BaronMove Steve's code for finding the best 5-byte no-op from ftrace.c to alternative.c. The idea is that other consumers (in this case jump label) want to make use of that code. I've created a global: 'char ideal_nop[5]', that is setup during setup_arch that can be used.
--- arch/x86/include/asm/alternative.h | 14 +++++++ arch/x86/include/asm/jump_label.h | 10 +--- arch/x86/kernel/alternative.c | 72 +++++++++++++++++++++++++++++++++++- arch/x86/kernel/ftrace.c | 70 +---------------------------------- arch/x86/kernel/jump_label.c | 19 +-------- arch/x86/kernel/module.c | 3 + arch/x86/kernel/setup.c | 3 + include/linux/jump_label.h | 9 ++++ kernel/jump_label.c | 33 ++++++++++++++++ kernel/trace/ftrace.c | 13 +------ 10 files changed, 140 insertions(+), 106 deletions(-)
arch/x86/include/asm/alternative.h +14 -0
@@ -4,6 +4,7 @@#include <linux/types.h> #include <linux/stddef.h> #include <linux/stringify.h> +#include <linux/jump_label.h> #include <asm/asm.h> /*
@@ -159,6 +160,8 @@ static inline void apply_paravirt(struct paravirt_patch_site *start,#define __parainstructions_end NULL#endif +extern void *text_poke_early (void *addr,const void *opcode,size_t len); +/* * Clear and restore the kernel write-protection flag on the local CPU. * Allows the kernel to edit read-only pages.
@@ -179,4 +182,15 @@ static inline void apply_paravirt(struct paravirt_patch_site *start,extern void *text_poke (void *addr,const void *opcode,size_t len);extern void *text_poke_smp (void *addr,const void *opcode,size_t len); +#if defined(CONFIG_DYNAMIC_FTRACE ) || defined(HAVE_JUMP_LABEL ) +#define IDEAL_NOP_SIZE_5 5 +extern unsigned char ideal_nop5[IDEAL_NOP_SIZE_5 ]; +extern int arch_init_ideal_nop5 (void ); +#else +static inline int arch_init_ideal_nop5 (void ) +{ +return 0 ; +} +#endif +#endif /* _ASM_X86_ALTERNATIVE_H */
arch/x86/include/asm/jump_label.h +3 -7
@@ -8,17 +8,13 @@#define JUMP_LABEL_NOP_SIZE 5 -#ifdef CONFIG_X86_64 -# define JUMP_LABEL_NOP P6_NOP5 -#else -# define JUMP_LABEL_NOP ".byte 0xe9 \n\t .long 0\n\t" -#endif +# define JUMP_LABEL_INITIAL_NOP ".byte 0xe9 \n\t .long 0\n\t" -#define JUMP_LABEL (tag, label, cond) \ +# define JUMP_LABEL (tag, label, cond) \ do { \ extern const char __jlstrtab_##tag[]; \ asm goto("1:" \ - JUMP_LABEL_NOP \ + JUMP_LABEL_INITIAL_NOP \ ".pushsection __jump_table, \"a\" \n\t"\ _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \ ".popsection \n\t" \
arch/x86/kernel/alternative.c +70 -2
87 lines not on this page · raw message
arch/x86/kernel/ftrace.c +1 -69
84 lines not on this page · raw message
arch/x86/kernel/jump_label.c +3 -16
@@ -30,19 +30,8 @@ void arch_jump_label_transform(struct jump_entry *entry, enum jump_label_type tyif (type ==JUMP_LABEL_ENABLE ) { code.jump =0xe9 ; code.offset = entry->target - (entry->code +JUMP_LABEL_NOP_SIZE ); - }else { -#ifdef CONFIG_X86_64 -/* opcode for P6_NOP5 */ - code.code[0 ] =0x0f ; - code.code[1 ] =0x1f ; - code.code[2 ] =0x44 ; - code.code[3 ] =0x00 ; - code.code[4 ] =0x00 ; -#else - code.jump =0xe9 ; - code.offset =0 ; -#endif - } + }else +memcpy (&code, ideal_nop5,JUMP_LABEL_NOP_SIZE );get_online_cpus ();mutex_lock (&text_mutex);text_poke_smp ((void *)entry->code, &code,JUMP_LABEL_NOP_SIZE );
@@ -50,11 +39,9 @@ void arch_jump_label_transform(struct jump_entry *entry, enum jump_label_type ty put_online_cpus(); } -static const u8 jump_label_nop[JUMP_LABEL_NOP_SIZE ] = {0x0f ,0x1f ,0x44 ,0x00 ,0x00 }; -const u8 *arch_get_jump_label_nop () { -return jump_label_nop; +return ideal_nop5; }#endif
arch/x86/kernel/module.c +3 -0
@@ -239,6 +239,9 @@ int module_finalize(const Elf_Ehdr *hdr,apply_paravirt (pseg, pseg + para->sh_size); } +/* make jump label nops */ +apply_jump_label_nops (me); +return module_bug_finalize (hdr, sechdrs, me); }
arch/x86/kernel/setup.c +3 -0
@@ -49,6 +49,7 @@#include <asm/pci-direct.h> #include <linux/init_ohci1394_dma.h> #include <linux/kvm_para.h> +#include <linux/jump_label.h> #include <linux/errno.h> #include <linux/kernel.h>
@@ -1063,6 +1064,8 @@ void __init setup_arch(char **cmdline_p) x86_init.oem.banner ();mcheck_init (); + +arch_init_ideal_nop5 (); }#ifdef CONFIG_X86_32
include/linux/jump_label.h +9 -0
@@ -11,6 +11,8 @@ enum jump_label_type { JUMP_LABEL_DISABLE }; +struct module ; +#ifdef HAVE_JUMP_LABEL extern struct jump_entry __start___jump_table[];
@@ -27,6 +29,8 @@ extern const u8 *arch_get_jump_label_nop(void);extern void jump_label_update (const char *name,enum jump_label_type type); +extern void apply_jump_label_nops (struct module *mod); +#define enable_jump_label (name) \ jump_label_update(name, JUMP_LABEL_ENABLE);
@@ -53,6 +57,11 @@ static inline int disable_jump_label(const char *name)return 0 ; } +static inline int apply_jump_label_nops (struct module *mod) +{ +return 0 ; +} +#endif #endif
kernel/jump_label.c +33 -0
@@ -184,10 +184,19 @@ void jump_label_update(const char *name, enum jump_label_type type)static int init_jump_label (void ) {int ret; +struct jump_entry *iter_start = __start___jump_table; +struct jump_entry *iter_stop = __stop___jump_table; +struct jump_entry *iter;mutex_lock (&jump_label_mutex); ret =build_jump_label_hashtable (__start___jump_table, __stop___jump_table); + iter = iter_start; +while (iter < iter_stop) { +text_poke_early ((void *)iter->code,arch_get_jump_label_nop (), +JUMP_LABEL_NOP_SIZE ); + iter++; + }mutex_unlock (&jump_label_mutex);return ret; }
@@ -298,6 +307,30 @@ static int jump_label_module_notify(struct notifier_block *self, unsigned long vreturn ret; } +/*** +* apply_jump_label_nops - patch module jump labels with arch_get_jump_label_nop() +* @mod: module to patch +* +* Allow for run-time selection of the optimal nops. Before the module +* loads patch these with arch_get_jump_label_nop(), which is specified by +* the arch specific jump label code. +*/ +void apply_jump_label_nops (struct module *mod) +{ +struct jump_entry *iter; + +/* if the module doesn't have jump label entries, just return */ +if (!mod->num_jump_entries) +return ; + + iter = mod->jump_entries; +while (iter < mod->jump_entries + mod->num_jump_entries) { +text_poke_early ((void *)iter->code,arch_get_jump_label_nop (), +JUMP_LABEL_NOP_SIZE ); + iter++; + } +} +struct notifier_block jump_label_module_nb = { .notifier_call = jump_label_module_notify, .priority =0 ,
kernel/trace/ftrace.c +1 -12
@@ -2749,20 +2749,9 @@ extern unsigned long __stop_mcount_loc[];void __init ftrace_init (void ) { -unsigned long count, addr, flags; +unsigned long count;int ret; -/* Keep the ftrace pointer to the stub */ - addr = (unsigned long )ftrace_stub; - -local_irq_save (flags); -ftrace_dyn_arch_init (&addr); -local_irq_restore (flags); - -/* ftrace_dyn_arch_init places the return code in addr */ -if (addr) - goto failed; - count = __stop_mcount_loc - __start_mcount_loc; ret =ftrace_dyn_table_alloc (count);
Signature · 1 line
1.7.1
Steven Rostedt
maintainerRe: [PATCH 06/13] jump label v9: move ftrace_dyn_arch_init to common code
Quoted 23 lines · Jason Baron
Move Steve's code for finding the best 5-byte no-op from
ftrace.ctoalternative.c. The idea is that other consumers (in this case jump label) want to make use of that code. I've created a global: 'char ideal_nop[5]', that is setup duringsetup_archthat can be used.Signed-off-by Jason Baron <[email protected]>kernel/trace/ftrace.c +1
-5@@ -2749,20 +2749,9 @@ extern unsigned long __stop_mcount_loc[];void __init ftrace_init (void ) { -unsigned long count, addr, flags; +unsigned long count;int ret; -/* Keep the ftrace pointer to the stub */ - addr = (unsigned long )ftrace_stub; - -local_irq_save (flags);
kernel/trace/ftrace.c extern unsigned long __stop_mcount_loc[]; · line 2757
Quoted 10 lines
kernel/trace/ftrace.c +0
-7-ftrace_dyn_arch_init (&addr); -local_irq_restore (flags); - -/* ftrace_dyn_arch_init places the return code in addr */ -if (addr) - goto failed; - count = __stop_mcount_loc - __start_mcount_loc; ret =ftrace_dyn_table_alloc (count);
Um, you just removed the ftrace arch specific setup call for dynamic ftrace. Although, I'm thinking this may not be needed since all archs currently have just a stub. With the exception of ARM which seems to call a "ftrace_mcount_set()" that git grep can not find.
Thus, if you remove this, then remove it from all archs.
-- Steve
PATCH 7/13 jump label v9: sort jump table at build-time
In reply to Jason BaronThe jump label table is more optimal accessed if the entries are continguous. Sorting the table accomplishes this. Do the sort at build-time. Adds a '-j' option to 'modpost' which replaces the vmlinux, with a sorted jump label section vmlinux. I've tested this on x86 with relocatable and it works fine there as well. Note that I have not sorted the jump label table in modules. The module tables tend to be smaller, and thus their is less value to this. The kernel continues to do the sort, just in case, but at least for the vmlinux, this is just a verfication that the jump label table has already been sorted.
--- Makefile | 6 ++- include/asm-generic/vmlinux.lds.h | 18 +++++++- scripts/mod/Makefile | 1 + scripts/mod/modpost.c | 83 +++++++++++++++++++++++++++++++++++- scripts/mod/modpost.h | 2 + 5 files changed, 103 insertions(+), 7 deletions(-)
Makefile +5 -1
@@ -152,7 +152,7 @@ obj := $(objtree) VPATH := $(srctree)$(if $(KBUILD_EXTMOD),:$(KBUILD_EXTMOD)) -export srctree objtree VPATH +export srctree objtree VPATH hdr-arch # SUBARCH tells the usermode build what the underlying arch is. That is set
@@ -850,6 +850,9 @@ define rule_vmlinux-modpost $(Q)echo 'cmd_$@ := $(cmd_vmlinux-modpost)' > $(dot-target).cmd endef +quiet_cmd_sort-jump-label = SORT $@ + cmd_sort-jump-label = $(srctree)/scripts/mod/modpost -j -s $@ + # vmlinux image - including updated kernel symbols vmlinux: $(vmlinux-lds) $(vmlinux-init) $(vmlinux-main) vmlinux.o $(kallsyms.o) FORCE ifdef CONFIG_HEADERS_CHECK
@@ -863,6 +866,7 @@ ifdef CONFIG_BUILD_DOCSRC endif $(call vmlinux-modpost) $(call if_changed_rule,vmlinux__) + $(call cmd,sort-jump-label) $(Q)rm -f .old_version # build vmlinux.o first to catch section mismatch errors early
include/asm-generic/vmlinux.lds.h +15 -3
@@ -168,7 +168,6 @@ TRACE_PRINTKS() \ FTRACE_EVENTS() \TRACE_SYSCALLS () \ -JUMP_TABLE () \/* * Data section helpers
@@ -207,7 +206,6 @@ *(__vermagic)/* Kernel version magic */ \ *(__markers_strings)/* Markers: strings */ \ *(__tracepoints_strings)/* Tracepoints: strings */ \ - *(__jump_strings)/* Jump: strings */ \ } \ \ .rodata1 : AT(ADDR(.rodata1) - LOAD_OFFSET) { \
@@ -216,6 +214,10 @@ \BUG_TABLE \ \ + JUMP_TABLE \ + \ + JUMP_STRINGS \ + \ /* PCI quirks */ \ .pci_fixup : AT(ADDR(.pci_fixup) - LOAD_OFFSET) { \ VMLINUX_SYMBOL(__start_pci_fixups_early) = .; \
@@ -559,11 +561,21 @@#define BUG_TABLE #endif -#define JUMP_TABLE () \ +#define JUMP_TABLE \ . = ALIGN(64); \ + __jump_table : AT(ADDR(__jump_table) - LOAD_OFFSET) { \ VMLINUX_SYMBOL(__start___jump_table) = .; \ *(__jump_table) \ VMLINUX_SYMBOL(__stop___jump_table) = .; \ + } + +#define JUMP_STRINGS \ + . = ALIGN(64); \ + __jump_strings : AT(ADDR(__jump_strings) - LOAD_OFFSET) { \ + VMLINUX_SYMBOL(__start___jump_strings) = .; \ + *(__jump_strings) \ + VMLINUX_SYMBOL(__stop___jump_strings) = .; \ + }#ifdef CONFIG_PM_TRACE #define TRACEDATA \
scripts/mod/Makefile +1 -0
@@ -1,4 +1,5 @@ hostprogs-y := modpost mk_elfconfig +HOST_EXTRACFLAGS += -iquote "$(srctree)/arch/$(hdr-arch)/include/asm" always := $(hostprogs-y) empty.o modpost-objs := modpost.o file2alias.o sumversion.o
scripts/mod/modpost.c +80 -3
135 lines not on this page · raw message
scripts/mod/modpost.h +2 -0
@@ -126,6 +126,8 @@ struct elf_info {Elf_Section export_gpl_sec;Elf_Section export_unused_gpl_sec;Elf_Section export_gpl_future_sec; +Elf_Section jump_sec; +Elf_Section jump_strings_sec;const char *strtab;char *modinfo;unsigned int modinfo_len;
Signature · 1 line
1.7.1
PATCH 8/13 jump label v9: initialize workqueue tracepoints *before* they are registered
In reply to Jason BaronInitialize the workqueue data structures *before* they are registered so that they are ready for callbacks.
--- kernel/trace/trace_workqueue.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
kernel/trace/trace_workqueue.c +5 -5
@@ -263,6 +263,11 @@ int __init trace_workqueue_early_init(void) {int ret, cpu; +for_each_possible_cpu (cpu) { +spin_lock_init (&workqueue_cpu_stat (cpu)->lock); +INIT_LIST_HEAD (&workqueue_cpu_stat (cpu)->list); + } + ret =register_trace_workqueue_insertion (probe_workqueue_insertion,NULL );if (ret) goto out;
@@ -279,11 +284,6 @@ int __init trace_workqueue_early_init(void)if (ret) goto no_creation; -for_each_possible_cpu (cpu) { -spin_lock_init (&workqueue_cpu_stat (cpu)->lock); -INIT_LIST_HEAD (&workqueue_cpu_stat (cpu)->list); - } -return 0 ; no_creation:
Signature · 1 line
1.7.1
PATCH 9/13 jump label v9: jump_label_text_reserved() to reserve our jump points
In reply to Jason BaronAdd a jump_label_text_reserved(void *start, void *end), so that other pieces of code that want to modify kernel text, can first verify that jump label has not reserved the instruction.
--- arch/x86/kernel/kprobes.c | 3 +- include/linux/jump_label.h | 6 +++ kernel/jump_label.c | 83 ++++++++++++++++++++++++++++++++++++++++++++ kernel/kprobes.c | 3 +- 4 files changed, 93 insertions(+), 2 deletions(-)
arch/x86/kernel/kprobes.c +2 -1
@@ -1201,7 +1201,8 @@ static int __kprobes copy_optimized_instructions(u8 *dest, u8 *src) }/* Check whether the address range is reserved */ if (ftrace_text_reserved (src, src + len -1 ) || -alternatives_text_reserved (src, src + len -1 )) +alternatives_text_reserved (src, src + len -1 ) || +jump_label_text_reserved (src, src + len -1 ))return -EBUSY ;return len;
include/linux/jump_label.h +6 -0
@@ -28,6 +28,7 @@ extern void arch_jump_label_transform(struct jump_entry *entry,extern const u8 *arch_get_jump_label_nop (void );extern void jump_label_update (const char *name,enum jump_label_type type); +extern int jump_label_text_reserved (void *start,void *end);extern void apply_jump_label_nops (struct module *mod);
@@ -62,6 +63,11 @@ static inline int apply_jump_label_nops(struct module *mod)return 0 ; } +static inline int jump_label_text_reserved (void *start,void *end) +{ +return 0 ; +} +#endif #endif
kernel/jump_label.c +83 -0
89 lines not on this page · raw message
kernel/kprobes.c +2 -1
@@ -1141,7 +1141,8 @@ int __kprobes register_kprobe(struct kprobe *p)preempt_disable ();if (!kernel_text_address ((unsigned long ) p->addr) ||in_kprobes_functions ((unsigned long ) p->addr) || -ftrace_text_reserved (p->addr, p->addr)) { +ftrace_text_reserved (p->addr, p->addr) || +jump_label_text_reserved (p->addr, p->addr)) {preempt_enable ();return -EINVAL ; }
Signature · 1 line
1.7.1
PATCH 10/13 jump label v9: convert jump label to use a key
In reply to Jason BaronConvert the jump label code to use a key to identify each related set of jump labels instead of a string. This saves space since we no longer need to store strings and their pointers, just the keys. In addition we can simplify the JUMP_LABEL macro to 2 arguments from 3. We use the address of the conditional variable as the key value. If jump labels are not enabled then we can use the conditional variable as usual.
--- arch/x86/include/asm/jump_label.h | 21 +++++++++++-------- include/linux/jump_label.h | 32 ++++++++---------------------- include/linux/tracepoint.h | 5 +-- kernel/jump_label.c | 38 ++++++++++++++++-------------------- kernel/tracepoint.c | 6 ++-- scripts/mod/modpost.c | 14 +----------- scripts/mod/modpost.h | 1 - 7 files changed, 45 insertions(+), 72 deletions(-)
arch/x86/include/asm/jump_label.h +12 -9
@@ -10,33 +10,36 @@# define JUMP_LABEL_INITIAL_NOP ".byte 0xe9 \n\t .long 0\n\t" -# define JUMP_LABEL (tag, label, cond) \ +# define JUMP_LABEL (key, label) \ do { \ - extern const char __jlstrtab_##tag[]; \ asm goto("1:" \ JUMP_LABEL_INITIAL_NOP \ ".pushsection __jump_table, \"a\" \n\t"\ _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \ ".popsection \n\t" \ - : : "i" (__jlstrtab_##tag) : : label);\ + : : "i" (key) : : label); \ } while (0)#endif /* __KERNEL__ */ #ifdef CONFIG_X86_64 +typedef __u64 jump_label_t ; +struct jump_entry { -__u64 code; -__u64 target; -__u64 name; +jump_label_t code; +jump_label_t target; +jump_label_t key; };#else +typedef __u32 jump_label_t ; +struct jump_entry { -__u32 code; -__u32 target; -__u32 name; +jump_label_t code; +jump_label_t target; +jump_label_t key; };#endif
include/linux/jump_label.h +9 -23
@@ -18,45 +18,31 @@ struct module;extern struct jump_entry __start___jump_table[];extern struct jump_entry __stop___jump_table[]; -#define DEFINE_JUMP_LABEL (name) \ - const char __jlstrtab_##name[] \ - __used __attribute__((section("__jump_strings"))) = #name; \ - EXPORT_SYMBOL_GPL(__jlstrtab_##name); -extern void arch_jump_label_transform (struct jump_entry *entry,enum jump_label_type type);extern const u8 *arch_get_jump_label_nop (void ); -extern void jump_label_update (const char *name,enum jump_label_type type); +extern void jump_label_update (jump_label_t key,enum jump_label_type type);extern int jump_label_text_reserved (void *start,void *end);extern void apply_jump_label_nops (struct module *mod); -#define enable_jump_label (name) \ - jump_label_update(name, JUMP_LABEL_ENABLE); +#define enable_jump_label (key) \ + jump_label_update(key, JUMP_LABEL_ENABLE); -#define disable_jump_label (name) \ - jump_label_update(name, JUMP_LABEL_DISABLE); +#define disable_jump_label (key) \ + jump_label_update(key, JUMP_LABEL_DISABLE);#else -#define DEFINE_JUMP_LABEL (name) - -#define JUMP_LABEL (tag, label, cond) \ +#define JUMP_LABEL (key, label) \ do { \ - if (unlikely(cond)) \ + if (unlikely(*key)) \ goto label; \ } while (0) -static inline int enable_jump_label (const char *name) -{ -return 0 ; -} - -static inline int disable_jump_label (const char *name) -{ -return 0 ; -} +#define enable_jump_label (key) +#define disable_jump_label (key)static inline int apply_jump_label_nops (struct module *mod) {
include/linux/tracepoint.h +2 -3
@@ -145,7 +145,7 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin, extern struct tracepoint __tracepoint_##name; \ static inline void trace_##name(proto) \ { \ - JUMP_LABEL(&__tracepoint_##name, do_trace, __tracepoint_##name.state);\ + JUMP_LABEL(&__tracepoint_##name.state, do_trace); \ return; \ do_trace: \ __DO_TRACE(&__tracepoint_##name, \
@@ -174,8 +174,7 @@ do_trace: \ __attribute__((section("__tracepoints_strings"))) = #name; \ struct tracepoint __tracepoint_##name \ __attribute__((section("__tracepoints"), aligned(32))) = \ - { __tpstrtab_##name, 0, reg, unreg, NULL }; \ - DEFINE_JUMP_LABEL(name); + { __tpstrtab_##name, 0, reg, unreg, NULL };#define DEFINE_TRACE (name) \ DEFINE_TRACE_FN(name, NULL, NULL);
kernel/jump_label.c +17 -21
@@ -27,7 +27,7 @@ struct jump_label_entry {int nr_entries;/* hang modules off here */ struct hlist_head modules; -const char *name; +unsigned long key; };struct jump_label_module_entry {
@@ -58,8 +58,7 @@ static void sort_jump_label_entries(struct jump_entry *start, struct jump_entry iter_next = start; iter_next++;for (; iter_next < stop; iter++, iter_next++) { -if (strcmp ((char *)iter->name, - (char *)iter_next->name) >0 ) { +if (iter->key > iter_next->key) {swap_jump_label_entries (iter, iter_next); swapped =1 ; }
@@ -67,29 +66,28 @@ static void sort_jump_label_entries(struct jump_entry *start, struct jump_entry }while (swapped ==1 ); } -static struct jump_label_entry *get_jump_label_entry (const char *name) +static struct jump_label_entry *get_jump_label_entry (jump_label_t key) {struct hlist_head *head;struct hlist_node *node;struct jump_label_entry *e; -u32 hash =jhash (name,strlen (name),0 ); +u32 hash =jhash ((void *)&key,sizeof (jump_label_t),0 ); head = &jump_label_table[hash & (JUMP_LABEL_TABLE_SIZE -1 )];hlist_for_each_entry (e, node, head, hlist) { -if (!strcmp ((char *)name, (char *)e->name)) +if (key == e->key)return e; }return NULL ; } -static struct jump_label_entry *add_jump_label_entry (const char *name,int nr_entries,struct jump_entry *table) +static struct jump_label_entry *add_jump_label_entry (jump_label_t key,int nr_entries,struct jump_entry *table) {struct hlist_head *head;struct jump_label_entry *e; -size_t name_len;u32 hash; - e =get_jump_label_entry (name); + e =get_jump_label_entry (key);if (e)return ERR_PTR (-EEXIST );
@@ -97,10 +95,9 @@ static struct jump_label_entry *add_jump_label_entry(const char *name, int nr_enif (!e)return ERR_PTR (-ENOMEM ); - name_len =strlen (name) +1 ; - hash =jhash (name, name_len-1 ,0 ); + hash =jhash ((void *)&key,sizeof (jump_label_t),0 ); head = &jump_label_table[hash & (JUMP_LABEL_TABLE_SIZE -1 )]; - e->name = name; + e->key = key; e->table = table; e->nr_entries = nr_entries;INIT_HLIST_HEAD (&(e->modules));
@@ -117,17 +114,16 @@ static int build_jump_label_hashtable(struct jump_entry *start, struct jump_entrsort_jump_label_entries (start, stop); iter = start;while (iter < stop) { - entry =get_jump_label_entry ((char *)iter->name); + entry =get_jump_label_entry (iter->key);if (!entry) { iter_begin = iter; count =0 ;while ((iter < stop) && - (strcmp ((char *)iter->name, - (char *)iter_begin->name) ==0 )) { + (iter->key == iter_begin->key)) { iter++; count++; } - entry =add_jump_label_entry ((char *)iter_begin->name, + entry =add_jump_label_entry (iter_begin->key, count, iter_begin);if (IS_ERR (entry))return PTR_ERR (entry);
@@ -148,7 +144,7 @@ static int build_jump_label_hashtable(struct jump_entry *start, struct jump_entr* */ -void jump_label_update (const char *name,enum jump_label_type type) +void jump_label_update (jump_label_t key,enum jump_label_type type) {struct jump_entry *iter;struct jump_label_entry *entry;
@@ -157,7 +153,7 @@ void jump_label_update(const char *name, enum jump_label_type type)int count;mutex_lock (&jump_label_mutex); - entry =get_jump_label_entry (name); + entry =get_jump_label_entry (key);if (entry) { count = entry->nr_entries; iter = entry->table;
@@ -316,16 +312,16 @@ static int add_jump_label_module(struct module *mod) mod->jump_entries + mod->num_jump_entries); iter = mod->jump_entries;while (iter < mod->jump_entries + mod->num_jump_entries) { - entry =get_jump_label_entry ((char *)iter->name); + entry =get_jump_label_entry (iter->key); iter_begin = iter; count =0 ;while ((iter < mod->jump_entries + mod->num_jump_entries) && - (strcmp ((char *)iter->name, (char *)iter_begin->name) ==0 )) { + (iter->key == iter_begin->key)) { iter++; count++; }if (!entry) { - entry =add_jump_label_entry ((char *)iter_begin->name,0 ,NULL ); + entry =add_jump_label_entry (iter_begin->key,0 ,NULL );if (IS_ERR (entry))return PTR_ERR (entry); }
kernel/tracepoint.c +3 -3
@@ -265,9 +265,9 @@ static void set_tracepoint(struct tracepoint_entry **entry,*/ rcu_assign_pointer (elem->funcs, (*entry)->funcs);if (!elem->state && active) -enable_jump_label (elem->name); +enable_jump_label ((jump_label_t)&elem->state);if (elem->state && !active) -disable_jump_label (elem->name); +disable_jump_label ((jump_label_t)&elem->state); elem->state = active; }
@@ -283,7 +283,7 @@ static void disable_tracepoint(struct tracepoint *elem) elem->unregfunc ();if (elem->state) -disable_jump_label (elem->name); +disable_jump_label ((jump_label_t)&elem->state); elem->state =0 ;rcu_assign_pointer (elem->funcs,NULL );
scripts/mod/modpost.c +2 -12
@@ -393,19 +393,14 @@ static void sort_jump_label_table(struct elf_info *info, Elf_Ehdr *hdr) {int swapped =0 ;struct jump_entry *iter, *iter_next; -char *name, *next_name;Elf_Shdr *sechdrs = info->sechdrs;unsigned long jump_table, jump_table_end; -unsigned long jump_strings, jump_strings_addr; -if ((info->jump_sec ==0 ) && (info->jump_strings_sec ==0 )) +if (info->jump_sec ==0 )return ; jump_table = (unsigned long )hdr + sechdrs[info->jump_sec].sh_offset; jump_table_end = jump_table + sechdrs[info->jump_sec].sh_size; - jump_strings = (unsigned long )hdr + - sechdrs[info->jump_strings_sec].sh_offset; - jump_strings_addr = sechdrs[info->jump_strings_sec].sh_addr; do { swapped = 0;
@@ -413,10 +408,7 @@ static void sort_jump_label_table(struct elf_info *info, Elf_Ehdr *hdr) iter_next++;for (; iter_next < (struct jump_entry *)jump_table_end; iter++, iter_next++) { - name = (char *)(jump_strings + (iter->name - jump_strings_addr)); - next_name = (char *)(jump_strings + - (iter_next->name - jump_strings_addr)); -if (strcmp (name, next_name) >0 ) { +if (iter->key > iter_next->key) {swap_jump_label_entries (iter, iter_next); swapped =1 ; }
@@ -528,8 +520,6 @@ static int parse_elf(struct elf_info *info, const char *filename) info->export_gpl_future_sec = i; else if (strcmp(secname, "__jump_table") == 0) info->jump_sec = i; - else if (strcmp(secname, "__jump_strings") == 0) - info->jump_strings_sec = i;if (sechdrs[i].sh_type !=SHT_SYMTAB )continue ;
scripts/mod/modpost.h +0 -1
@@ -127,7 +127,6 @@ struct elf_info {Elf_Section export_unused_gpl_sec;Elf_Section export_gpl_future_sec;Elf_Section jump_sec; -Elf_Section jump_strings_sec;const char *strtab;char *modinfo;unsigned int modinfo_len;
Signature · 1 line
1.7.1
Peter Zijlstra
maintainerRe: [PATCH 10/13] jump label v9: convert jump label to use a key
include/linux/jump_label.h struct module; · line 42
Quoted 7 lines · Jason Baron
include/linux/jump_label.h +2
-2-#define JUMP_LABEL (tag, label, cond) \ +#define JUMP_LABEL (key, label) \ do { \ - if (unlikely(cond)) \ + if (unlikely(*key)) \ goto label; \ } while (0)
s/key/cond_var/ or something like that?
Peter Zijlstra
maintainerRe: [PATCH 10/13] jump label v9: convert jump label to use a key
In reply to Jason Baroninclude/linux/jump_label.h struct module; · line 45
Quoted 11 lines · Jason Baron
include/linux/jump_label.h +2
-9-static inline int enable_jump_label (const char *name) -{ -return 0 ; -} - -static inline int disable_jump_label (const char *name) -{ -return 0 ; -} +#define enable_jump_label (key) +#define disable_jump_label (key)
I would expect enable_jump_label() to look something like:
#define enable_jump_label(cond_var) \
do { \
*(cond_var) = 1; \
} while (0)That way the HAVE_JUMP_LABEL and !HAVE_JUMP_LABEL code has similar effects.
Re: [PATCH 10/13] jump label v9: convert jump label to use a key
Quoted 15 lines · Peter Zijlstra
include/linux/jump_label.h struct module; · line 45
Quoted 11 lines · Jason Baron
include/linux/jump_label.h +2
-9-static inline int enable_jump_label (const char *name) -{ -return 0 ; -} - -static inline int disable_jump_label (const char *name) -{ -return 0 ; -} +#define enable_jump_label (key) +#define disable_jump_label (key)I would expect
enable_jump_label()to look something like:
Quoted 7 lines
#define
enable_jump_label(cond_var)\do { \ *(cond_var) = 1; \ } while (0)That way the
HAVE_JUMP_LABELand !HAVE_JUMP_LABEL code has similar effects.
right. I was going to clean that up in a followup. But you are right - I think it makes the code much clearer. will fix.
thanks,
-Jason
Peter Zijlstra
maintainerRe: [PATCH 10/13] jump label v9: convert jump label to use a key
Quoted 2 lines · Jason Baron
right. I was going to clean that up in a followup. But you are right - I think it makes the code much clearer. will fix.
Please fold this patch into the previous patches. That makes the series much more readable.
PATCH 11/13 jump label v9: convert dynamic debug to use jump labels.
In reply to Jason BaronConvert the 'dynamic debug' infrastructure to use jump labels.
--- include/linux/dynamic_debug.h | 42 ++++++++++++++------------- lib/dynamic_debug.c | 42 ++------------------------- scripts/Makefile.lib | 11 +------ scripts/basic/Makefile | 2 +- scripts/basic/hash.c | 64 ----------------------------------------- 5 files changed, 27 insertions(+), 134 deletions(-) delete mode 100644 scripts/basic/hash.c
include/linux/dynamic_debug.h +22 -20
@@ -1,6 +1,8 @@#ifndef _DYNAMIC_DEBUG_H#define _DYNAMIC_DEBUG_H +#include <linux/jump_label.h> +/* dynamic_printk_enabled, and dynamic_printk_enabled2 are bitmasks in which * bit n is set to 1 if any modname hashes into the bucket n, 0 otherwise. They * use independent hash functions, to reduce the chance of false positives.
@@ -22,9 +24,7 @@ struct _ddebug {const char *function;const char *filename;const char *format; -char primary_hash; -char secondary_hash; - unsigned int lineno:24; +unsigned int lineno;/* * The flags field controls the behaviour at the callsite. * The bits here are changed dynamically when the user
@@ -32,7 +32,7 @@ struct _ddebug {*/ #define _DPRINTK_FLAGS_PRINT (1<<0)/* printk() a message using the format */ #define _DPRINTK_FLAGS_DEFAULT 0 - unsigned int flags:8; +char flags; } __attribute__((aligned(8)));
@@ -42,33 +42,35 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,#if defined(CONFIG_DYNAMIC_DEBUG )extern int ddebug_remove_module (char *mod_name); -#define __dynamic_dbg_enabled (dd) ({ \ - int __ret = 0; \ - if (unlikely((dynamic_debug_enabled & (1LL << DEBUG_HASH)) && \ - (dynamic_debug_enabled2 & (1LL << DEBUG_HASH2)))) \ - if (unlikely(dd.flags)) \ - __ret = 1; \ - __ret; }) -#define dynamic_pr_debug (fmt, ...) do { \ + __label__ do_printk; \ + __label__ out; \ static struct _ddebug descriptor \ __used \ __attribute__((section("__verbose"), aligned(8))) = \ - { KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \ - DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \ - if (__dynamic_dbg_enabled(descriptor)) \ - printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ + { KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__, \ + _DPRINTK_FLAGS_DEFAULT }; \ + JUMP_LABEL(&descriptor.flags, do_printk); \ + goto out; \ +do_printk: \ + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ +out: ; \ } while (0)#define dynamic_dev_dbg (dev, fmt, ...) do { \ + __label__ do_printk; \ + __label__ out; \ static struct _ddebug descriptor \ __used \ __attribute__((section("__verbose"), aligned(8))) = \ - { KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \ - DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \ - if (__dynamic_dbg_enabled(descriptor)) \ - dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ + { KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__, \ + _DPRINTK_FLAGS_DEFAULT }; \ + JUMP_LABEL(&descriptor.flags, do_printk); \ + goto out; \ +do_printk: \ + dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ +out: ; \ } while (0)#else
lib/dynamic_debug.c +3 -39
65 lines not on this page · raw message
scripts/Makefile.lib +1 -10
@@ -101,14 +101,6 @@ basename_flags = -D"KBUILD_BASENAME=KBUILD_STR($(call name-fix,$(basetarget)))" modname_flags = $(if $(filter 1,$(words $(modname))),\ -D"KBUILD_MODNAME=KBUILD_STR($(call name-fix,$(modname)))") -#hash values -ifdef CONFIG_DYNAMIC_DEBUG -debug_flags = -D"DEBUG_HASH=$(shell ./scripts/basic/hash djb2 $(@D)$(modname))"\ - -D"DEBUG_HASH2=$(shell ./scripts/basic/hash r5 $(@D)$(modname))" -else -debug_flags = -endif - orig_c_flags = $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(KBUILD_SUBDIR_CCFLAGS) \ $(ccflags-y) $(CFLAGS_$(basetarget).o) _c_flags = $(filter-out $(CFLAGS_REMOVE_$(basetarget).o), $(orig_c_flags))
@@ -152,8 +144,7 @@ endif c_flags = -Wp,-MD,$(depfile) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) \ $(__c_flags) $(modkern_cflags) \ - -D"KBUILD_STR(s)=\#s" $(basename_flags) $(modname_flags) \ - $(debug_flags) + -D"KBUILD_STR(s)=\#s" $(basename_flags) $(modname_flags) a_flags = -Wp,-MD,$(depfile) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) \ $(__a_flags) $(modkern_aflags)
scripts/basic/Makefile +1 -1
@@ -9,7 +9,7 @@ # fixdep: Used to generate dependency information during build process # docproc: Used in Documentation/DocBook -hostprogs-y := fixdep docproc hash +hostprogs-y := fixdep docproc always := $(hostprogs-y) # fixdep is needed to compile other host programs
scripts/basic/hash.c deleted +0 -64
64 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH 12/13 jump label v9: sparc64 add jump_label support
In reply to Jason BaronAdd jump label support for sparc64.
--- arch/sparc/Kconfig | 1 + arch/sparc/include/asm/jump_label.h | 32 +++++++++++++++++++++++++++++ arch/sparc/kernel/Makefile | 2 + arch/sparc/kernel/jump_label.c | 38 +++++++++++++++++++++++++++++++++++ arch/sparc/kernel/module.c | 6 +++++ 5 files changed, 79 insertions(+), 0 deletions(-) create mode 100644 arch/sparc/include/asm/jump_label.h create mode 100644 arch/sparc/kernel/jump_label.c
arch/sparc/Kconfig +1 -0
@@ -29,6 +29,7 @@ config SPARC select PERF_USE_VMALLOC select HAVE_DMA_ATTRS select HAVE_DMA_API_DEBUG + select HAVE_ARCH_JUMP_LABEL if !CC_OPTIMIZE_FOR_SIZE config SPARC32 def_bool !64BIT
arch/sparc/include/asm/jump_label.h new file +32 -0
@@ -0,0 +1,32 @@ +#ifndef _ASM_SPARC_JUMP_LABEL_H +#define _ASM_SPARC_JUMP_LABEL_H + +#ifdef __KERNEL__ + +#include <linux/types.h> +#include <asm/jump_entry.h> + +#define JUMP_LABEL_NOP_SIZE 4 + +#define JUMP_LABEL (tag, label) \ + do { \ + asm goto("1:\n\t" \ + "nop\n\t" \ + "nop\n\t" \ + ".pushsection __jump_table, \"a\"\n\t"\ + ".xword 1b, %l[" #label "], %c0\n\t" \ + ".popsection \n\t" \ + : : "i" (tag) : : label);\ + } while (0) + +#endif /* __KERNEL__ */ + +typedef __u32 jump_label_t ; + +struct jump_entry { +jump_label_t code; +jump_label_t target; +jump_label_t name; +}; + +#endif
arch/sparc/kernel/Makefile +2 -0
@@ -119,3 +119,5 @@ obj-$(CONFIG_COMPAT) += $(audit--y) pc--$(CONFIG_PERF_EVENTS) := perf_event.o obj-$(CONFIG_SPARC64) += $(pc--y) + +obj-$(CONFIG_SPARC64) += jump_label.o
arch/sparc/kernel/jump_label.c new file +38 -0
@@ -0,0 +1,38 @@ +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/mutex.h> +#include <linux/cpu.h> + +#include <linux/jump_label.h> +#include <linux/memory.h> + +#ifdef HAVE_JUMP_LABEL + +void arch_jump_label_transform (struct jump_entry *entry,enum jump_label_type type) +{ +u32 val, *insn = (u32 *) entry->code; + + val = *insn; +if (type ==JUMP_LABEL_ENABLE ) { +s32 off = (s32 )entry->target - (s32 )entry->code; + val =0x40000000 | ((u32 ) off >>2 ); + }else { + val =0x01000000 ; + } + +get_online_cpus (); +mutex_lock (&text_mutex); + *insn = val; +flushi (insn); +mutex_unlock (&text_mutex); +put_online_cpus (); +} + +static const u8 jump_label_nop[JUMP_LABEL_NOP_SIZE ] = {0x01 ,0x00 ,0x00 ,0x00 }; + +const u8 *arch_get_jump_label_nop () +{ +return jump_label_nop; +} + +#endif
arch/sparc/kernel/module.c +6 -0
@@ -18,6 +18,9 @@#include <asm/spitfire.h> #ifdef CONFIG_SPARC64 + +#include <linux/jump_label.h> +static void *module_map (unsigned long size) {struct vm_struct *area;
@@ -227,6 +230,9 @@ int module_finalize(const Elf_Ehdr *hdr,const Elf_Shdr *sechdrs, struct module *me) { +/* make jump label nops */ +apply_jump_label_nops (me ); +/* Cheetah's I-cache is fully coherent. */ if (tlb_type == spitfire) {unsigned long va;
Signature · 1 line
1.7.1
PATCH 13/13 jump label v9: add docs
In reply to Jason BaronAdd jump label docs as: Documentation/jump-label.txt
--- Documentation/jump-label.txt | 151 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 151 insertions(+), 0 deletions(-) create mode 100644 Documentation/jump-label.txt
Documentation/jump-label.txt new file +151 -0
@@ -0,0 +1,151 @@ + Jump Label + ---------- + +By: Jason Baron <[email protected]> + + +1) motivation + + +Currently, tracepoints are implemented using a conditional. The conditional +check requires checking a global variable for each tracepoint. Although, +the overhead of this check is small, it increases under memory pressure. As we +increase the number of tracepoints in the kernel this may become more of an +issue. In addition, tracepoints are often dormant (disabled), and provide no +direct kernel functionality. Thus, it is highly desirable to reduce their +impact as much as possible. Although tracepoints are the original motivation +for this work, other kernel code paths should be able to make use of the jump +label optimization. + + +2) jump label description/usage + + +gcc (v4.5) adds a new 'asm goto' statement that allows branching to a label. +http://gcc.gnu.org/ml/gcc-patches/2009-07/msg01556.html + +Thus, this patch set introduces an architecture specific 'JUMP_LABEL()' macro as +follows (x86): + +# define JUMP_LABEL_INITIAL_NOP ".byte 0xe9 \n\t .long 0\n\t" + +# define JUMP_LABEL(key, label) \ + do { \ + asm goto("1:" \ + JUMP_LABEL_INITIAL_NOP \ + ".pushsection __jump_table, \"a\" \n\t"\ + _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \ + ".popsection \n\t" \ + : : "i" (key) : : label); \ + } while (0) + + +For architectures that have not yet introduced jump label support its simply: + +#define JUMP_LABEL(key, label) \ + if (unlikely(*key)) \ + goto label; + +which then can be used as: + + .... + JUMP_LABEL(trace_name, trace_label, jump_enabled); + printk("not doing tracing\n"); + return; +trace_label: + printk("doing tracing: %d\n", file); + .... + +The 'key' argument is thus a pointer to a conditional argument that can be used +if the optimization is not enabled. Otherwise, this address serves as a unique +key to identify the particular instance of the jump label. + +Thus, when tracing is disabled, we simply have a no-op followed by a jump around +the dormant (disabled) tracing code. The 'JUMP_LABEL()' macro, produces a +'jump_table' which has the following format: + +[instruction address] [jump target] [tracepoint key] + +Thus, to enable a tracepoint, we simply patch the 'instruction address' with +a jump to the 'jump target'. + +The call to enable a jump label is: enable_jump_label(key); to disable: +disable_jump_label(key); + + +3) architecture interface + + +There are a few functions and macros which arches must implement in order to +take advantage of this optimization. As previously mentioned, if there is no +architecture support we simply fall back to a traditional, load, test, and +jump sequence. + +* add "HAVE_ARCH_JUMP_LABEL" to arch/<arch>/Kconfig to indicate support + +* #define JUMP_LABEL_NOP_SIZE, arch/x86/include/asm/jump_label.h + +* #define "JUMP_LABEL(tag, label, cond)", arch/x86/include/asm/jump_label.h + +* add: void arch_jump_label_transform(struct jump_entry *entry, enum jump_label_type type) + and + const u8 *arch_get_jump_label_nop(void) + + see: arch/x86/kernel/jump_label.c + +* finally add a definition for "struct jump_entry". + see: arch/x86/include/asm/jump_label.h + + +4) Jump label analysis (x86) + + +I've tested the performance of using 'get_cycles()' calls around the +tracepoint call sites. For an Intel Core 2 Quad cpu (in cycles, averages): + + idle after tbench run + ---- ---------------- +old code 32 88 +new code 2 4 + + +The performance improvement can be reproduced reliably on both Intel and AMD +hardware. + +In terms of code analysis the current code for the disabled case is a 'cmpl' +followed by a 'je' around the tracepoint code. so: + +cmpl - 83 3d 0e 77 87 00 00 - 7 bytes +je - 74 3e - 2 bytes + +total of 9 instruction bytes. + +The new code is a 'nopl' followed by a 'jmp'. Thus: + +nopl - 0f 1f 44 00 00 - 5 bytes +jmp - eb 3e - 2 bytes + +total of 7 instruction bytes. + +So, the new code also accounts for 2 less bytes in the instruction cache per tracepoint. + +The optimization depends on !CC_OPTIMIZE_FOR_SIZE. When CC_OPTIMIZE_FOR_SIZE is +set, gcc does not always out of line the not taken label path in the same way +that the "if unlikely()" paths are made out of line. Thus, with +CC_OPTIMIZE_FOR_SIZE set, this optimization is not always optimal. This may be +solved in subsequent gcc versions, that allow us to move labels out of line, +while still optimizing for size. In the case of !CC_OPTIMIZE_FOR_SIZE this +optimization is seen on high level benchmarks such as tbench where I can get +~1-2% higher throughput. In addition we are within .5% of the performance of no +tracepoints compiled in at all. + + +5) Acknowledgments + + +Thanks to Roland McGrath and Richard Henderson for helping come up with the +initial 'asm goto' and jump label design. + +Thanks to Mathieu Desnoyers and H. Peter Anvin for calling attention to this +issue, and outlining the requirements of a solution. Mathieu also implemened a +solution in the form of the "Immediate Values" work.
Signature · 1 line
1.7.1
Peter Zijlstra
maintainerRe: [PATCH 13/13] jump label v9: add docs
Documentation/jump-label.txt line 140
Quoted 9 lines · Jason Baron
Documentation/jump-label.txt new file +9
-0+The optimization depends on !CC_OPTIMIZE_FOR_SIZE. When CC_OPTIMIZE_FOR_SIZE is +set, gcc does not always out of line the not taken label path in the same way +that the "if unlikely()" paths are made out of line. Thus, with +CC_OPTIMIZE_FOR_SIZE set, this optimization is not always optimal. This may be +solved in subsequent gcc versions, that allow us to move labels out of line, +while still optimizing for size. In the case of !CC_OPTIMIZE_FOR_SIZE this +optimization is seen on high level benchmarks such as tbench where I can get +~1-2% higher throughput. In addition we are within .5% of the performance of no +tracepoints compiled in at all.
But does it generate invalid code for whatever -f flag triggers this (-fno-reorder-blocks ?)
David Miller
maintainerJason, I'm really at wits end about this patch set. To say that trying to test our your patches is frustrating for me so far would be an understatement.
Nothing you ever post builds for me, not one patch set has built properly.
I can also tell that you're just blindly making changes to the sparc bits and not trying to build test them at all:
1) Even though you created the jump_label_t, and made it properly a u32 on sparc, you left the assembler using ".xword" to record the entries.
2) The sparc "struct jump_label" still calls it's third member "name", it needs to be "key" or else the build breaks.
3) Eventhough the sparc JUMP_LABEL macro was fixed to have two args, the first arg was left as "tag" instead of being renamed to "key" and that name change propaged into the asm in the macro expansion.
I took care of that locally to try and test this, but then I hit the current major problem which is that you're using things like text_poke_early() unconditionally, but that is an X86-only facility implemented by x86's alternative mechanism.
Also, kernel/jump_label.c only gets the ERR_PTR() definitions indirectly on the x86 platform, it needs to include linux/err.h directly to make sure those things are available on every platform.
You gave me the impression a few iterations ago that you were doing build testing on sparc64 using cross-compilers, or that you would start to do so. You're obviously not, could you please start doing so and let me know when you've at least build tested your jump-label patch series on sparc64 and at least one architecture that lacks jump-label support?
Thanks.
Quoted 30 lines · David Miller
Jason, I'm really at wits end about this patch set. To say that trying to test our your patches is frustrating for me so far would be an understatement.
Nothing you ever post builds for me, not one patch set has built properly.
I can also tell that you're just blindly making changes to the sparc bits and not trying to build test them at all:
1) Even though you created the
jump_label_t, and made it properly a u32 on sparc, you left the assembler using ".xword" to record the entries.2) The sparc "struct
jump_label" still calls it's third member "name", it needs to be "key" or else the build breaks.3) Eventhough the sparc
JUMP_LABELmacro was fixed to have two args, the first arg was left as "tag" instead of being renamed to "key" and that name change propaged into the asm in the macro expansion.I took care of that locally to try and test this, but then I hit the current major problem which is that you're using things like
text_poke_early()unconditionally, but that is an X86-only facility implemented by x86's alternative mechanism.Also,
kernel/jump_label.conly gets theERR_PTR()definitions indirectly on the x86 platform, it needs to includelinux/err.hdirectly to make sure those things are available on every platform.
Quoted 8 lines
You gave me the impression a few iterations ago that you were doing build testing on sparc64 using cross-compilers, or that you would start to do so. You're obviously not, could you please start doing so and let me know when you've at least build tested your jump-label patch series on sparc64 and at least one architecture that lacks jump-label support?
Thanks.
Hi David,
Yes, I've tried to help re-write the sparc bits to the current api. However, I did not test the sparc enabled jump-label bits, b/c I need an updated cross compiler to do so (that has jump label support). However, I certainly did build test the patches on powerpc, which lacks jump-label support, so I know it builds on at least one architecture that lacks jump-label support as you've mentioned. And I did this specifically, since you requested this testing.
I guess I was hoping we could work more collaboratively on the sparc bits. A couple lines of code have fixed the issues that you've brought up. Sorry, if i mislead you. I really just want to do what is best for the linux kernel, if that's going off and figuring out how to compile a new sparc enabled jump label compiler for sparc, I will do it. And I do agree, that leaving text_poke_early() is my mistake. However, maybe we can discuss this issue? For example, the reason I have it in the code is b/c x86 determines the best no-op at run-time. Are other architectures going to have to require this kind of functionality. Or like sparc, are we going to be able to generally hard-code the nops on non-x86 at compile-time?
thanks. And again I apologize for any wasted cycles that I've caused.
-Jason
Quoted 50 lines · * Jason Baron ([email protected]) wrote:
Quoted 30 lines · David Miller
Jason, I'm really at wits end about this patch set. To say that trying to test our your patches is frustrating for me so far would be an understatement.
Nothing you ever post builds for me, not one patch set has built properly.
I can also tell that you're just blindly making changes to the sparc bits and not trying to build test them at all:
1) Even though you created the
jump_label_t, and made it properly a u32 on sparc, you left the assembler using ".xword" to record the entries.2) The sparc "struct
jump_label" still calls it's third member "name", it needs to be "key" or else the build breaks.3) Eventhough the sparc
JUMP_LABELmacro was fixed to have two args, the first arg was left as "tag" instead of being renamed to "key" and that name change propaged into the asm in the macro expansion.I took care of that locally to try and test this, but then I hit the current major problem which is that you're using things like
text_poke_early()unconditionally, but that is an X86-only facility implemented by x86's alternative mechanism.Also,
kernel/jump_label.conly gets theERR_PTR()definitions indirectly on the x86 platform, it needs to includelinux/err.hdirectly to make sure those things are available on every platform.Quoted 8 lines
You gave me the impression a few iterations ago that you were doing build testing on sparc64 using cross-compilers, or that you would start to do so. You're obviously not, could you please start doing so and let me know when you've at least build tested your jump-label patch series on sparc64 and at least one architecture that lacks jump-label support?
Thanks.
Hi David,
Yes, I've tried to help re-write the sparc bits to the current api. However, I did not test the sparc enabled jump-label bits, b/c I need an updated cross compiler to do so (that has jump label support). However, I certainly did build test the patches on powerpc, which lacks jump-label support, so I know it builds on at least one architecture that lacks jump-label support as you've mentioned. And I did this specifically, since you requested this testing.
Quoted 5 lines
I guess I was hoping we could work more collaboratively on the sparc bits. A couple lines of code have fixed the issues that you've brought up. Sorry, if i mislead you. I really just want to do what is best for the linux kernel, if that's going off and figuring out how to compile a new sparc enabled jump label compiler for sparc, I will do it.
Hi Jason,
It makes me wonder if anyone had success building a gcc 4.5 Intel-to-sparc64 cross-compiler ? Usually, the crosstool-like suites are a few versions behind. I'm aware that this is not trivial, as cross-compilers have a tendency to refuse to get built in certain occasions (such as being a recent less tested version). I'd recommend you focus on this as a first step before resubmitting.
Quoted 7 lines
And I do agree,
that leavingtext_poke_early()is my mistake. However, maybe we can discuss this issue? For example, the reason I have it in the code is b/c x86 determines the best no-op at run-time. Are other architectures going to have to require this kind of functionality. Or like sparc, are we going to be able to generally hard-code the nops on non-x86 at compile-time?
You might want to create "dumb" text_poke() and text_poke_early() implementations on other architectures that wraps kernel text updates pretty simply. The implementation is trivial if the architecture does not write-protect the text pages, but becomes more evolved when it does.
Thanks,
Mathieu
Quoted 6 lines
thanks. And again I apologize for any wasted cycles that I've caused.
-Jason
Signature · 4 lines
Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com
Quoted 6 lines · Mathieu Desnoyers
It makes me wonder if anyone had success building a gcc 4.5 Intel-to-sparc64 cross-compiler ? Usually, the crosstool-like suites are a few versions behind. I'm aware that this is not trivial, as cross-compilers have a tendency to refuse to get built in certain occasions (such as being a recent less tested version). I'd recommend you focus on this as a first step before resubmitting.
I'm building (and uploading to kernel.org[1]) 4.4.4 and 4.5.0 toolchains. They're untested on anything otyher than the system I built them on.
Give it 24 hours and then try'em
Yours Tony
[1] http://kernel.org/pub/tools/crosstool/files/bin/${host_arch}/ [2]
[2] currently only i686 and x86_64, powerpc64 are comming.
Quoted 11 lines · * Tony Breeds ([email protected]) wrote:
Quoted 6 lines · Mathieu Desnoyers
It makes me wonder if anyone had success building a gcc 4.5 Intel-to-sparc64 cross-compiler ? Usually, the crosstool-like suites are a few versions behind. I'm aware that this is not trivial, as cross-compilers have a tendency to refuse to get built in certain occasions (such as being a recent less tested version). I'd recommend you focus on this as a first step before resubmitting.
I'm building (and uploading to kernel.org[1]) 4.4.4 and 4.5.0 toolchains. They're untested on anything otyher than the system I built them on.
Cool !!!
Thanks a lot Tony.
Mathieu
Quoted 6 lines
Give it 24 hours and then try'em
Yours Tony
[1] http://kernel.org/pub/tools/crosstool/files/bin/${host_arch}/ [2]
[2] currently only i686 andx86_64, powerpc64 are comming.
Signature · 4 lines
Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com
David Miller
maintainerFrom: Jason Baron <[email protected]>
Date: Tue, 15 Jun 2010 10:28:11 -0400
Quoted 4 lines
For example, the reason I have it in the code is b/c x86 determines the best no-op at run-time. Are other architectures going to have to require this kind of functionality. Or like sparc, are we going to be able to generally hard-code the nops on non-x86 at compile-time?
I think most architectures will use a constant nop sequence, in fact x86 is the only one I can think of that needs variable nop sequences.
Why not abstract this behind some asm/jump_label.h macro just like everything else? "jump_label_text_poke_early()" or similar. On sparc64 I would define this to:
#include <asm/system.h>
static inline void jump_label_text_poke_early(void *addr, const void *opcode, size_t len)
{
u32 new_insn = *(u32 *)opcode;
u32 *insn_p = (u32 *) addr;
*insn_p = new_insn;
flushi(insn_p);
}H. Peter Anvin
reviewerQuoted 11 lines · David Miller
From: Jason Baron <[email protected]>
Date: Tue, 15 Jun 2010 10:28:11 -0400Quoted 4 lines
For example, the reason I have it in the code is b/c x86 determines the best no-op at run-time. Are other architectures going to have to require this kind of functionality. Or like sparc, are we going to be able to generally hard-code the nops on non-x86 at compile-time?
I think most architectures will use a constant nop sequence, in fact x86 is the only one I can think of that needs variable nop sequences.
One could potentially see that for other variable-length-instructions architectures, e.g. S390, m68k or ARM/Thumb2 as well.
For fixed-length-instructions architectures, well, it shouldn't be an issue.
-hpa