| Message ID | 20170814105231.14608-6-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | add support for relative references in special sections | expand |
On Mon, 14 Aug 2017 11:52:31 +0100 Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > To avoid the need for relocating absolute references to tracepoint > structures at boot time when running relocatable kernels (which may > take a disproportionate amount of space), add the option to emit > these tables as relative references instead. > > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ingo Molnar <mingo@redhat.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > include/linux/tracepoint.h | 19 +++++++++++++++---- > kernel/tracepoint.c | 19 ++++++++++++++----- > 2 files changed, 29 insertions(+), 9 deletions(-) > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > index a26ffbe09e71..d02bf1a695e8 100644 > --- a/include/linux/tracepoint.h > +++ b/include/linux/tracepoint.h > @@ -228,6 +228,19 @@ extern void syscall_unregfunc(void); > return static_key_false(&__tracepoint_##name.key); \ > } > > +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS > +#define __TRACEPOINT_ENTRY(name) \ > + asm(" .section \"__tracepoints_ptrs\", \"a\" \n" \ > + " .balign 4 \n" \ > + " .long " VMLINUX_SYMBOL_STR(__tracepoint_##name) " - .\n" \ > + " .previous \n") > +#else > +#define __TRACEPOINT_ENTRY(name) \ > + static struct tracepoint * const __tracepoint_ptr_##name __used \ > + __attribute__((section("__tracepoints_ptrs"))) = \ > + &__tracepoint_##name > +#endif > + > /* > * We have no guarantee that gcc and the linker won't up-align the tracepoint > * structures, so we create an array of pointers that will be used for iteration > @@ -237,11 +250,9 @@ extern void syscall_unregfunc(void); > static const char __tpstrtab_##name[] \ > __attribute__((section("__tracepoints_strings"))) = #name; \ > struct tracepoint __tracepoint_##name \ > - __attribute__((section("__tracepoints"))) = \ > + __attribute__((section("__tracepoints"), used)) = \ > { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\ > - static struct tracepoint * const __tracepoint_ptr_##name __used \ > - __attribute__((section("__tracepoints_ptrs"))) = \ > - &__tracepoint_##name; > + __TRACEPOINT_ENTRY(name); > > #define DEFINE_TRACE(name) \ > DEFINE_TRACE_FN(name, NULL, NULL); > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index 685c50ae6300..1c6689603764 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -28,8 +28,8 @@ > #include <linux/sched/task.h> > #include <linux/static_key.h> > > -extern struct tracepoint * const __start___tracepoints_ptrs[]; > -extern struct tracepoint * const __stop___tracepoints_ptrs[]; > +extern const unsigned char __start___tracepoints_ptrs[]; > +extern const unsigned char __stop___tracepoints_ptrs[]; Does this have to be converted to a char array? > > /* Set to 1 to enable tracepoint debug output */ > static const int tracepoint_debug; > @@ -503,17 +503,26 @@ static __init int init_tracepoints(void) > __initcall(init_tracepoints); > #endif /* CONFIG_MODULES */ > > -static void for_each_tracepoint_range(struct tracepoint * const *begin, > - struct tracepoint * const *end, > +static void for_each_tracepoint_range(const void *begin, const void *end, > void (*fct)(struct tracepoint *tp, void *priv), > void *priv) > { > +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS > + const signed int *iter; > + > + if (!begin) > + return; > + for (iter = begin; iter < (signed int *)end; iter++) { Isn't the typecasts here sufficient, even if the above end is defined as a tracepoint * const *? -- Steve > + fct((struct tracepoint *)((unsigned long)iter + *iter), priv); > + } > +#else > struct tracepoint * const *iter; > > if (!begin) > return; > - for (iter = begin; iter < end; iter++) > + for (iter = begin; iter < (struct tracepoint * const *)end; iter++) > fct(*iter, priv); > +#endif > } > > /**
On 14 August 2017 at 16:23, Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 14 Aug 2017 11:52:31 +0100 > Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> To avoid the need for relocating absolute references to tracepoint >> structures at boot time when running relocatable kernels (which may >> take a disproportionate amount of space), add the option to emit >> these tables as relative references instead. >> >> Cc: Steven Rostedt <rostedt@goodmis.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> include/linux/tracepoint.h | 19 +++++++++++++++---- >> kernel/tracepoint.c | 19 ++++++++++++++----- >> 2 files changed, 29 insertions(+), 9 deletions(-) >> >> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h >> index a26ffbe09e71..d02bf1a695e8 100644 >> --- a/include/linux/tracepoint.h >> +++ b/include/linux/tracepoint.h >> @@ -228,6 +228,19 @@ extern void syscall_unregfunc(void); >> return static_key_false(&__tracepoint_##name.key); \ >> } >> >> +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS >> +#define __TRACEPOINT_ENTRY(name) \ >> + asm(" .section \"__tracepoints_ptrs\", \"a\" \n" \ >> + " .balign 4 \n" \ >> + " .long " VMLINUX_SYMBOL_STR(__tracepoint_##name) " - .\n" \ >> + " .previous \n") >> +#else >> +#define __TRACEPOINT_ENTRY(name) \ >> + static struct tracepoint * const __tracepoint_ptr_##name __used \ >> + __attribute__((section("__tracepoints_ptrs"))) = \ >> + &__tracepoint_##name >> +#endif >> + >> /* >> * We have no guarantee that gcc and the linker won't up-align the tracepoint >> * structures, so we create an array of pointers that will be used for iteration >> @@ -237,11 +250,9 @@ extern void syscall_unregfunc(void); >> static const char __tpstrtab_##name[] \ >> __attribute__((section("__tracepoints_strings"))) = #name; \ >> struct tracepoint __tracepoint_##name \ >> - __attribute__((section("__tracepoints"))) = \ >> + __attribute__((section("__tracepoints"), used)) = \ >> { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\ >> - static struct tracepoint * const __tracepoint_ptr_##name __used \ >> - __attribute__((section("__tracepoints_ptrs"))) = \ >> - &__tracepoint_##name; >> + __TRACEPOINT_ENTRY(name); >> >> #define DEFINE_TRACE(name) \ >> DEFINE_TRACE_FN(name, NULL, NULL); >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c >> index 685c50ae6300..1c6689603764 100644 >> --- a/kernel/tracepoint.c >> +++ b/kernel/tracepoint.c >> @@ -28,8 +28,8 @@ >> #include <linux/sched/task.h> >> #include <linux/static_key.h> >> >> -extern struct tracepoint * const __start___tracepoints_ptrs[]; >> -extern struct tracepoint * const __stop___tracepoints_ptrs[]; >> +extern const unsigned char __start___tracepoints_ptrs[]; >> +extern const unsigned char __stop___tracepoints_ptrs[]; > > Does this have to be converted to a char array? > >> >> /* Set to 1 to enable tracepoint debug output */ >> static const int tracepoint_debug; >> @@ -503,17 +503,26 @@ static __init int init_tracepoints(void) >> __initcall(init_tracepoints); >> #endif /* CONFIG_MODULES */ >> >> -static void for_each_tracepoint_range(struct tracepoint * const *begin, >> - struct tracepoint * const *end, >> +static void for_each_tracepoint_range(const void *begin, const void *end, >> void (*fct)(struct tracepoint *tp, void *priv), >> void *priv) >> { >> +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS >> + const signed int *iter; >> + >> + if (!begin) >> + return; >> + for (iter = begin; iter < (signed int *)end; iter++) { > > Isn't the typecasts here sufficient, even if the above end is defined > as a tracepoint * const *? > As long as iter's type is of the right size (4 bytes even on 64-bit arches), then it does not really matter what type we use to define the start and and of the array. It could be confusing, though, if anyone tries to use those section markers elsewhere so perhaps I should move them into the function in that case? Your call.
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > -static void for_each_tracepoint_range(struct tracepoint * const *begin, > - struct tracepoint * const *end, > +static void for_each_tracepoint_range(const void *begin, const void *end, > void (*fct)(struct tracepoint *tp, void *priv), > void *priv) > { > +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS > + const signed int *iter; > + > + if (!begin) > + return; > + for (iter = begin; iter < (signed int *)end; iter++) { > + fct((struct tracepoint *)((unsigned long)iter + *iter), priv); > + } I think checkpatch is correct here to complain about the unnecessary curly braces here. Plus why the heavy use of 'signed int' here and elsewhere in the patches - why not 'int' ? Plus #2, the heavy use of type casts looks pretty ugly to begin with - is there no way to turn this into more natural code? Thanks, Ingo
On 18 August 2017 at 09:26, Ingo Molnar <mingo@kernel.org> wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> -static void for_each_tracepoint_range(struct tracepoint * const *begin, >> - struct tracepoint * const *end, >> +static void for_each_tracepoint_range(const void *begin, const void *end, >> void (*fct)(struct tracepoint *tp, void *priv), >> void *priv) >> { >> +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS >> + const signed int *iter; >> + >> + if (!begin) >> + return; >> + for (iter = begin; iter < (signed int *)end; iter++) { >> + fct((struct tracepoint *)((unsigned long)iter + *iter), priv); >> + } > > I think checkpatch is correct here to complain about the unnecessary curly braces > here. > Fair enough. I will clean up to the extent feasible. > Plus why the heavy use of 'signed int' here and elsewhere in the patches - why > not 'int' ? > Yes, just 'int' should be sufficient. Force of habit, I suppose, given that unqualified 'char' is ambiguous between architectures, but this does not apply to 'int' of course. > Plus #2, the heavy use of type casts looks pretty ugly to begin with - is there no > way to turn this into more natural code? > Actually, Steven requested to keep the tracepoint section markers as they are, and cast them to (int *) in the conditionally compiled PREL32 case. So I guess it is a matter of taste, but because the types are fundamentally incompatible when this code is in effect (and the size of the pointed-to type differs on 64-bit architectures), there is always some mangling required. The initcall patch is probably the cleanest in this regard, but involves typedefs, which are frowned upon as well.
On 18 August 2017 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 18 August 2017 at 09:26, Ingo Molnar <mingo@kernel.org> wrote: >> >> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >>> -static void for_each_tracepoint_range(struct tracepoint * const *begin, >>> - struct tracepoint * const *end, >>> +static void for_each_tracepoint_range(const void *begin, const void *end, >>> void (*fct)(struct tracepoint *tp, void *priv), >>> void *priv) >>> { >>> +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS >>> + const signed int *iter; >>> + >>> + if (!begin) >>> + return; >>> + for (iter = begin; iter < (signed int *)end; iter++) { >>> + fct((struct tracepoint *)((unsigned long)iter + *iter), priv); >>> + } >> >> I think checkpatch is correct here to complain about the unnecessary curly braces >> here. >> > > Fair enough. I will clean up to the extent feasible. > OK, in an honest attempt to at least remove as many of the checkpatch errors as I can, I am running into the paradoxical situation where I either get ERROR: space required after that ',' (ctx:VxO) #83: FILE: include/linux/init.h:232: +#define console_initcall(fn) ___define_initcall(fn,, .con_initcall) or ERROR: space prohibited before that ',' (ctx:WxW) #156: FILE: include/linux/init.h:232: +#define console_initcall(fn) ___define_initcall(fn, , .con_initcall) ^ which I think may be checkpatch trying to give me a hint that I have done enough work for the week, and should spend some time by the pool instead.
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 18 August 2017 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 18 August 2017 at 09:26, Ingo Molnar <mingo@kernel.org> wrote: > >> > >> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> > >>> -static void for_each_tracepoint_range(struct tracepoint * const *begin, > >>> - struct tracepoint * const *end, > >>> +static void for_each_tracepoint_range(const void *begin, const void *end, > >>> void (*fct)(struct tracepoint *tp, void *priv), > >>> void *priv) > >>> { > >>> +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS > >>> + const signed int *iter; > >>> + > >>> + if (!begin) > >>> + return; > >>> + for (iter = begin; iter < (signed int *)end; iter++) { > >>> + fct((struct tracepoint *)((unsigned long)iter + *iter), priv); > >>> + } > >> > >> I think checkpatch is correct here to complain about the unnecessary curly braces > >> here. > >> > > > > Fair enough. I will clean up to the extent feasible. > > > > OK, in an honest attempt to at least remove as many of the checkpatch > errors as I can, [...] Note that I actually agreed with your list of checkpatch bogosities - the one I commented on was the only thing that needed fixing, IMHO. Thanks, Ingo
On 18 August 2017 at 18:58, Ingo Molnar <mingo@kernel.org> wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> On 18 August 2017 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > On 18 August 2017 at 09:26, Ingo Molnar <mingo@kernel.org> wrote: >> >> >> >> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >> >> >>> -static void for_each_tracepoint_range(struct tracepoint * const *begin, >> >>> - struct tracepoint * const *end, >> >>> +static void for_each_tracepoint_range(const void *begin, const void *end, >> >>> void (*fct)(struct tracepoint *tp, void *priv), >> >>> void *priv) >> >>> { >> >>> +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS >> >>> + const signed int *iter; >> >>> + >> >>> + if (!begin) >> >>> + return; >> >>> + for (iter = begin; iter < (signed int *)end; iter++) { >> >>> + fct((struct tracepoint *)((unsigned long)iter + *iter), priv); >> >>> + } >> >> >> >> I think checkpatch is correct here to complain about the unnecessary curly braces >> >> here. >> >> >> > >> > Fair enough. I will clean up to the extent feasible. >> > >> >> OK, in an honest attempt to at least remove as many of the checkpatch >> errors as I can, [...] > > Note that I actually agreed with your list of checkpatch bogosities - the one I > commented on was the only thing that needed fixing, IMHO. > Ah ok. Well, I think the code has improved slightly in some ways as a result, so I will just back out the bogus changes for v3.
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h index a26ffbe09e71..d02bf1a695e8 100644 --- a/include/linux/tracepoint.h +++ b/include/linux/tracepoint.h @@ -228,6 +228,19 @@ extern void syscall_unregfunc(void); return static_key_false(&__tracepoint_##name.key); \ } +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS +#define __TRACEPOINT_ENTRY(name) \ + asm(" .section \"__tracepoints_ptrs\", \"a\" \n" \ + " .balign 4 \n" \ + " .long " VMLINUX_SYMBOL_STR(__tracepoint_##name) " - .\n" \ + " .previous \n") +#else +#define __TRACEPOINT_ENTRY(name) \ + static struct tracepoint * const __tracepoint_ptr_##name __used \ + __attribute__((section("__tracepoints_ptrs"))) = \ + &__tracepoint_##name +#endif + /* * We have no guarantee that gcc and the linker won't up-align the tracepoint * structures, so we create an array of pointers that will be used for iteration @@ -237,11 +250,9 @@ extern void syscall_unregfunc(void); static const char __tpstrtab_##name[] \ __attribute__((section("__tracepoints_strings"))) = #name; \ struct tracepoint __tracepoint_##name \ - __attribute__((section("__tracepoints"))) = \ + __attribute__((section("__tracepoints"), used)) = \ { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\ - static struct tracepoint * const __tracepoint_ptr_##name __used \ - __attribute__((section("__tracepoints_ptrs"))) = \ - &__tracepoint_##name; + __TRACEPOINT_ENTRY(name); #define DEFINE_TRACE(name) \ DEFINE_TRACE_FN(name, NULL, NULL); diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index 685c50ae6300..1c6689603764 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -28,8 +28,8 @@ #include <linux/sched/task.h> #include <linux/static_key.h> -extern struct tracepoint * const __start___tracepoints_ptrs[]; -extern struct tracepoint * const __stop___tracepoints_ptrs[]; +extern const unsigned char __start___tracepoints_ptrs[]; +extern const unsigned char __stop___tracepoints_ptrs[]; /* Set to 1 to enable tracepoint debug output */ static const int tracepoint_debug; @@ -503,17 +503,26 @@ static __init int init_tracepoints(void) __initcall(init_tracepoints); #endif /* CONFIG_MODULES */ -static void for_each_tracepoint_range(struct tracepoint * const *begin, - struct tracepoint * const *end, +static void for_each_tracepoint_range(const void *begin, const void *end, void (*fct)(struct tracepoint *tp, void *priv), void *priv) { +#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS + const signed int *iter; + + if (!begin) + return; + for (iter = begin; iter < (signed int *)end; iter++) { + fct((struct tracepoint *)((unsigned long)iter + *iter), priv); + } +#else struct tracepoint * const *iter; if (!begin) return; - for (iter = begin; iter < end; iter++) + for (iter = begin; iter < (struct tracepoint * const *)end; iter++) fct(*iter, priv); +#endif } /**
To avoid the need for relocating absolute references to tracepoint structures at boot time when running relocatable kernels (which may take a disproportionate amount of space), add the option to emit these tables as relative references instead. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@redhat.com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- include/linux/tracepoint.h | 19 +++++++++++++++---- kernel/tracepoint.c | 19 ++++++++++++++----- 2 files changed, 29 insertions(+), 9 deletions(-) -- 2.11.0