Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1710821 > unrolled thread

[PATCH 5/5] kernel: tracepoints: add support for relative references

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 5/5] kernel: tracepoints: add support for relative references Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-08-14 13:00 +0200
    Re: [PATCH 5/5] kernel: tracepoints: add support for relative  references Steven Rostedt <rostedt@goodmis.org> - 2017-08-14 17:30 +0200
      Re: [PATCH 5/5] kernel: tracepoints: add support for relative references Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-08-14 17:30 +0200
    Re: [PATCH 5/5] kernel: tracepoints: add support for relative  references Ingo Molnar <mingo@kernel.org> - 2017-08-18 10:30 +0200
      Re: [PATCH 5/5] kernel: tracepoints: add support for relative references Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-08-18 10:40 +0200
        Re: [PATCH 5/5] kernel: tracepoints: add support for relative references Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-08-18 11:30 +0200
          Re: [PATCH 5/5] kernel: tracepoints: add support for relative  references Ingo Molnar <mingo@kernel.org> - 2017-08-18 20:00 +0200
            Re: [PATCH 5/5] kernel: tracepoints: add support for relative references Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-08-18 20:20 +0200

#1710821 — [PATCH 5/5] kernel: tracepoints: add support for relative references

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[];
 
 /* 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
 }
 
 /**
-- 
2.11.0

[toc] | [next] | [standalone]

#1711024 — Re: [PATCH 5/5] kernel: tracepoints: add support for relative references

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
>  }
>  
>  /**

[toc] | [prev] | [next] | [standalone]

#1711026

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.

[toc] | [prev] | [next] | [standalone]

#1714689 — Re: [PATCH 5/5] kernel: tracepoints: add support for relative references

* 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

[toc] | [prev] | [next] | [standalone]

#1714698

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.

[toc] | [prev] | [next] | [standalone]

#1714748

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.

[toc] | [prev] | [next] | [standalone]

#1715350 — Re: [PATCH 5/5] kernel: tracepoints: add support for relative references

* 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

[toc] | [prev] | [next] | [standalone]

#1715361

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web