[v2,6/6] kernel: tracepoints: add support for relative references

Message ID 20170818112624.24991-7-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • add support for relative references in special sections
Related show

Commit Message

Ard Biesheuvel Aug. 18, 2017, 11:26 a.m.
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 | 42 ++++++++++++++++++--
 kernel/tracepoint.c        |  7 +---
 2 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.11.0

Comments

Ard Biesheuvel Aug. 18, 2017, 11:44 a.m. | #1
On 18 August 2017 at 12:26, 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 | 42 ++++++++++++++++++--

>  kernel/tracepoint.c        |  7 +---

>  2 files changed, 40 insertions(+), 9 deletions(-)

>

> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h

> index a26ffbe09e71..68701821933a 100644

> --- a/include/linux/tracepoint.h

> +++ b/include/linux/tracepoint.h

> @@ -228,6 +228,42 @@ 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")

> +

> +struct tracepoint_entry_t {

> +       int tp_offset;

> +};

> +

> +static inline

> +struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)

> +{

> +       return (struct tracepoint *)((unsigned long)ent + ent->tp_offset);

> +}

> +#else

> +#define __TRACEPOINT_ENTRY(name)                                        \

> +       static struct tracepoint * const __tracepoint_ptr_##name __used  \

> +       __attribute__((section("__tracepoints_ptrs"))) =                 \

> +               &__tracepoint_##name

> +

> +struct tracepoint_entry_t {

> +       struct tracepoint *tp;

> +};

> +

> +static inline

> +struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)

> +{

> +       return ent->tp;

> +}

> +#endif

> +

> +extern struct tracepoint_entry_t const __start___tracepoints_ptrs[];

> +extern struct tracepoint_entry_t const __stop___tracepoints_ptrs[];

> +


It appears the stuff above needs to be move inside the double-include
guard (which oddly enough does not cover the entire file)

>  /*

>   * 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 +273,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..21bc41454fd6 100644

> --- a/kernel/tracepoint.c

> +++ b/kernel/tracepoint.c

> @@ -28,9 +28,6 @@

>  #include <linux/sched/task.h>

>  #include <linux/static_key.h>

>

> -extern struct tracepoint * const __start___tracepoints_ptrs[];

> -extern struct tracepoint * const __stop___tracepoints_ptrs[];

> -

>  /* Set to 1 to enable tracepoint debug output */

>  static const int tracepoint_debug;

>

> @@ -508,12 +505,12 @@ static void for_each_tracepoint_range(struct tracepoint * const *begin,

>                 void (*fct)(struct tracepoint *tp, void *priv),

>                 void *priv)

>  {

> -       struct tracepoint * const *iter;

> +       struct tracepoint_entry_t const *iter;

>

>         if (!begin)

>                 return;

>         for (iter = begin; iter < end; iter++)

> -               fct(*iter, priv);

> +               fct(tracepoint_from_entry(iter), priv);

>  }

>

>  /**

> --

> 2.11.0

>
Steven Rostedt Aug. 18, 2017, 1:43 p.m. | #2
On Fri, 18 Aug 2017 12:44:17 +0100
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 18 August 2017 at 12:26, 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 | 42 ++++++++++++++++++--

> >  kernel/tracepoint.c        |  7 +---

> >  2 files changed, 40 insertions(+), 9 deletions(-)

> >

> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h

> > index a26ffbe09e71..68701821933a 100644

> > --- a/include/linux/tracepoint.h

> > +++ b/include/linux/tracepoint.h

> > @@ -228,6 +228,42 @@ 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")

> > +

> > +struct tracepoint_entry_t {

> > +       int tp_offset;

> > +};

> > +

> > +static inline

> > +struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)

> > +{

> > +       return (struct tracepoint *)((unsigned long)ent + ent->tp_offset);

> > +}

> > +#else

> > +#define __TRACEPOINT_ENTRY(name)                                        \

> > +       static struct tracepoint * const __tracepoint_ptr_##name __used  \

> > +       __attribute__((section("__tracepoints_ptrs"))) =                 \

> > +               &__tracepoint_##name

> > +

> > +struct tracepoint_entry_t {

> > +       struct tracepoint *tp;

> > +};

> > +

> > +static inline

> > +struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)

> > +{

> > +       return ent->tp;

> > +}

> > +#endif

> > +

> > +extern struct tracepoint_entry_t const __start___tracepoints_ptrs[];

> > +extern struct tracepoint_entry_t const __stop___tracepoints_ptrs[];

> > +  

> 

> It appears the stuff above needs to be move inside the double-include

> guard (which oddly enough does not cover the entire file)


Why was this moved to the header file? To fulfill some checkpatch
warning?

-- Steve

> 

> >  /*

> >   * 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 +273,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..21bc41454fd6 100644

> > --- a/kernel/tracepoint.c

> > +++ b/kernel/tracepoint.c

> > @@ -28,9 +28,6 @@

> >  #include <linux/sched/task.h>

> >  #include <linux/static_key.h>

> >

> > -extern struct tracepoint * const __start___tracepoints_ptrs[];

> > -extern struct tracepoint * const __stop___tracepoints_ptrs[];

> > -

> >  /* Set to 1 to enable tracepoint debug output */

> >  static const int tracepoint_debug;

> >

> > @@ -508,12 +505,12 @@ static void for_each_tracepoint_range(struct tracepoint * const *begin,

> >                 void (*fct)(struct tracepoint *tp, void *priv),

> >                 void *priv)

> >  {

> > -       struct tracepoint * const *iter;

> > +       struct tracepoint_entry_t const *iter;

> >

> >         if (!begin)

> >                 return;

> >         for (iter = begin; iter < end; iter++)

> > -               fct(*iter, priv);

> > +               fct(tracepoint_from_entry(iter), priv);

> >  }

> >

> >  /**

> > --

> > 2.11.0

> >
Ard Biesheuvel Aug. 18, 2017, 1:44 p.m. | #3
On 18 August 2017 at 14:43, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 18 Aug 2017 12:44:17 +0100

> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>

>> On 18 August 2017 at 12:26, 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 | 42 ++++++++++++++++++--

>> >  kernel/tracepoint.c        |  7 +---

>> >  2 files changed, 40 insertions(+), 9 deletions(-)

>> >

>> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h

>> > index a26ffbe09e71..68701821933a 100644

>> > --- a/include/linux/tracepoint.h

>> > +++ b/include/linux/tracepoint.h

>> > @@ -228,6 +228,42 @@ 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")

>> > +

>> > +struct tracepoint_entry_t {

>> > +       int tp_offset;

>> > +};

>> > +

>> > +static inline

>> > +struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)

>> > +{

>> > +       return (struct tracepoint *)((unsigned long)ent + ent->tp_offset);

>> > +}

>> > +#else

>> > +#define __TRACEPOINT_ENTRY(name)                                        \

>> > +       static struct tracepoint * const __tracepoint_ptr_##name __used  \

>> > +       __attribute__((section("__tracepoints_ptrs"))) =                 \

>> > +               &__tracepoint_##name

>> > +

>> > +struct tracepoint_entry_t {

>> > +       struct tracepoint *tp;

>> > +};

>> > +

>> > +static inline

>> > +struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)

>> > +{

>> > +       return ent->tp;

>> > +}

>> > +#endif

>> > +

>> > +extern struct tracepoint_entry_t const __start___tracepoints_ptrs[];

>> > +extern struct tracepoint_entry_t const __stop___tracepoints_ptrs[];

>> > +

>>

>> It appears the stuff above needs to be move inside the double-include

>> guard (which oddly enough does not cover the entire file)

>

> Why was this moved to the header file? To fulfill some checkpatch

> warning?

>


Yes.
Steven Rostedt Aug. 18, 2017, 1:52 p.m. | #4
On Fri, 18 Aug 2017 14:44:15 +0100
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >> It appears the stuff above needs to be move inside the double-include

> >> guard (which oddly enough does not cover the entire file)  

> >

> > Why was this moved to the header file? To fulfill some checkpatch

> > warning?

> >  

> 

> Yes.


My preference is to ignore that checkpatch warning. The section
variables are created by linker magic, and not normal "extern"
variables. They are only used in one location, and I like to keep them
where they are used, and not be something other places might think they
can be used. In other words, keep them by the C code, and out of
headers.

Tracepoints and linker/asm work always triggers a lot of bogus
checkpatch warnings. Which is unfortunate. :-/

Thanks!

-- Steve
Ard Biesheuvel Aug. 18, 2017, 1:54 p.m. | #5
On 18 August 2017 at 14:52, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 18 Aug 2017 14:44:15 +0100

> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>

>> >> It appears the stuff above needs to be move inside the double-include

>> >> guard (which oddly enough does not cover the entire file)

>> >

>> > Why was this moved to the header file? To fulfill some checkpatch

>> > warning?

>> >

>>

>> Yes.

>

> My preference is to ignore that checkpatch warning. The section

> variables are created by linker magic, and not normal "extern"

> variables. They are only used in one location, and I like to keep them

> where they are used, and not be something other places might think they

> can be used. In other words, keep them by the C code, and out of

> headers.

>

> Tracepoints and linker/asm work always triggers a lot of bogus

> checkpatch warnings. Which is unfortunate. :-/

>


Actually, I couldn't agree more. I will backpedal on the checkpatch
appeasement in v3 in general.

Thanks,
Ard.

Patch

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index a26ffbe09e71..68701821933a 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -228,6 +228,42 @@  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")
+
+struct tracepoint_entry_t {
+	int tp_offset;
+};
+
+static inline
+struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)
+{
+	return (struct tracepoint *)((unsigned long)ent + ent->tp_offset);
+}
+#else
+#define __TRACEPOINT_ENTRY(name)					 \
+	static struct tracepoint * const __tracepoint_ptr_##name __used	 \
+	__attribute__((section("__tracepoints_ptrs"))) =		 \
+		&__tracepoint_##name
+
+struct tracepoint_entry_t {
+	struct tracepoint *tp;
+};
+
+static inline
+struct tracepoint *tracepoint_from_entry(const struct tracepoint_entry_t *ent)
+{
+	return ent->tp;
+}
+#endif
+
+extern struct tracepoint_entry_t const __start___tracepoints_ptrs[];
+extern struct tracepoint_entry_t const __stop___tracepoints_ptrs[];
+
 /*
  * 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 +273,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..21bc41454fd6 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -28,9 +28,6 @@ 
 #include <linux/sched/task.h>
 #include <linux/static_key.h>
 
-extern struct tracepoint * const __start___tracepoints_ptrs[];
-extern struct tracepoint * const __stop___tracepoints_ptrs[];
-
 /* Set to 1 to enable tracepoint debug output */
 static const int tracepoint_debug;
 
@@ -508,12 +505,12 @@  static void for_each_tracepoint_range(struct tracepoint * const *begin,
 		void (*fct)(struct tracepoint *tp, void *priv),
 		void *priv)
 {
-	struct tracepoint * const *iter;
+	struct tracepoint_entry_t const *iter;
 
 	if (!begin)
 		return;
 	for (iter = begin; iter < end; iter++)
-		fct(*iter, priv);
+		fct(tracepoint_from_entry(iter), priv);
 }
 
 /**