Re: [PATCH v2 2/2] tracing: gpio: add Kconfig option for enabling/disabling trace events

Re: [PATCH v2 2/2] tracing: gpio: add Kconfig option for enabling/disabling trace events

[Steven Rostedt]

On Thu, 16 Jul 2015 20:39:36 +0300
Tal Shorer <tal.shorer@gmail.com> wrote:

> Add a new options to gpio Kconfig, CONFIG_GPIO_TRACING, that is used
> for enabling/disabling compilation of gpio function trace events.
> 

If I can get acks from the gpio maintainers, I can take this in my
tree, as it depends on code that modifies the core tracing facility.

-- Steve

> Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
> ---
>  drivers/gpio/Kconfig        | 7 +++++++
>  include/trace/events/gpio.h | 4 ++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c1e2ca3..2829e8e 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -88,6 +88,13 @@ config GPIO_SYSFS
>  config GPIO_GENERIC
>  	tristate
>  
> +config GPIO_TRACING
> +	bool "gpio tracing"
> +	depends on TRACING
> +	help
> +	  Enable tracing for gpio subsystem
> +
> +
>  # put drivers in the right section, in alphabetical order
>  
>  config GPIO_DA9052
> diff --git a/include/trace/events/gpio.h b/include/trace/events/gpio.h
> index 927a8ad..09af636 100644
> --- a/include/trace/events/gpio.h
> +++ b/include/trace/events/gpio.h
> @@ -1,6 +1,10 @@
>  #undef TRACE_SYSTEM
>  #define TRACE_SYSTEM gpio
>  
> +#ifndef CONFIG_GPIO_TRACING
> +#define NOTRACE
> +#endif
> +
>  #if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ)
>  #define _TRACE_GPIO_H
>  

Re: [PATCH v2 2/2] tracing: gpio: add Kconfig option for enabling/disabling trace events

[Tal Shorer]

On second thought, I wonder if the config option shouldn't be in
kernel/trace/Kconfig
Imagine there's an option for every subsystem. Disabling all of them
but the one you actually care about is going to be a mess if they're
scattered all over the place.
Your thoughts?

On Fri, Jul 17, 2015 at 9:36 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 16 Jul 2015 20:39:36 +0300
> Tal Shorer <tal.shorer@gmail.com> wrote:
>
>> Add a new options to gpio Kconfig, CONFIG_GPIO_TRACING, that is used
>> for enabling/disabling compilation of gpio function trace events.
>>
>
> If I can get acks from the gpio maintainers, I can take this in my
> tree, as it depends on code that modifies the core tracing facility.
>
> -- Steve
>
>> Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
>> ---
>>  drivers/gpio/Kconfig        | 7 +++++++
>>  include/trace/events/gpio.h | 4 ++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index c1e2ca3..2829e8e 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -88,6 +88,13 @@ config GPIO_SYSFS
>>  config GPIO_GENERIC
>>       tristate
>>
>> +config GPIO_TRACING
>> +     bool "gpio tracing"
>> +     depends on TRACING
>> +     help
>> +       Enable tracing for gpio subsystem
>> +
>> +
>>  # put drivers in the right section, in alphabetical order
>>
>>  config GPIO_DA9052
>> diff --git a/include/trace/events/gpio.h b/include/trace/events/gpio.h
>> index 927a8ad..09af636 100644
>> --- a/include/trace/events/gpio.h
>> +++ b/include/trace/events/gpio.h
>> @@ -1,6 +1,10 @@
>>  #undef TRACE_SYSTEM
>>  #define TRACE_SYSTEM gpio
>>
>> +#ifndef CONFIG_GPIO_TRACING
>> +#define NOTRACE
>> +#endif
>> +
>>  #if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ)
>>  #define _TRACE_GPIO_H
>>
>

Re: [PATCH v2 2/2] tracing: gpio: add Kconfig option for enabling/disabling trace events

[Linus Walleij]

On Fri, Jul 17, 2015 at 8:36 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 16 Jul 2015 20:39:36 +0300
> Tal Shorer <tal.shorer@gmail.com> wrote:
>
>> Add a new options to gpio Kconfig, CONFIG_GPIO_TRACING, that is used
>> for enabling/disabling compilation of gpio function trace events.
>>
>
> If I can get acks from the gpio maintainers, I can take this in my
> tree, as it depends on code that modifies the core tracing facility.

What works for the tracing maintainer works for me.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Now you can decide if you want this or not... there are some
comments from Tal Shorer I see.

Yours,
Linus Walleij

Re: [Patch v3 2/2] tracing: gpio: add Kconfig option for enabling/disabling trace events

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

On Sat, Aug 01, 2015 at 03:27:58PM +0300, Tal Shorer wrote:
> Add a new options to trace Kconfig, CONFIG_TRACING_EVENTS_GPIO, that is
> used for enabling/disabling compilation of gpio function trace events.
> 
> Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  include/trace/events/gpio.h | 4 ++++
>  kernel/trace/Kconfig        | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/include/trace/events/gpio.h b/include/trace/events/gpio.h
> index 927a8ad..2da73b9 100644
> --- a/include/trace/events/gpio.h
> +++ b/include/trace/events/gpio.h
> @@ -1,6 +1,10 @@
>  #undef TRACE_SYSTEM
>  #define TRACE_SYSTEM gpio
>  
> +#ifndef CONFIG_TRACING_EVENTS_GPIO
> +#define NOTRACE
> +#endif
> +
>  #if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ)
>  #define _TRACE_GPIO_H
>  
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 3b9a48a..67a1731 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -635,6 +635,13 @@ config TRACE_ENUM_MAP_FILE
>  
>  	If unsure, say N
>  
> +config TRACING_EVENTS_GPIO
> +	bool "Trace gpio events"
> +	depends on GPIOLIB
> +	default y
> +	help
> +	  Enable tracing events for gpio subsystem
> +

It happened a few times to me since this patch was merged (in 4.4-rc1)
that I worked on a system with tracing enabled but
/sys/kernel/debug/tracing/events/gpio were missing because of
CONFIG_TRACING_EVENTS_GPIO=n. This is annoying and I wonder what is the
motivating purpose of allowing to disable gpio events. There is no other
event type that can be disabled with kconfig symbols. Theoretically the
overhead should be so small, that having these events enabled
unconditionally should be fine, shouldn't it?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Patch v3 2/2] tracing: gpio: add Kconfig option for enabling/disabling trace events

[Linus Walleij]

On Wed, Mar 27, 2019 at 11:12 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> It happened a few times to me since this patch was merged (in 4.4-rc1)
> that I worked on a system with tracing enabled but
> /sys/kernel/debug/tracing/events/gpio were missing because of
> CONFIG_TRACING_EVENTS_GPIO=n. This is annoying and I wonder what is the
> motivating purpose of allowing to disable gpio events. There is no other
> event type that can be disabled with kconfig symbols. Theoretically the
> overhead should be so small, that having these events enabled
> unconditionally should be fine, shouldn't it?

You have a good point. We should not do anything special.

I guess by the time this was merged I believed it was customary
for trancepoints to live behind a Kconfig option.

I guess it is not.

I happily merge  a patch removing this again if no case
can be made for it.

Yours,
Linus Walleij

Re: [PATCH 2/2] tracing: drop handling of NOTRACE symbol

[Uwe Kleine-König]

Hello Linus and Steven,

On Mon, Apr 08, 2019 at 04:29:15PM +0200, Linus Walleij wrote:
> On Mon, Apr 8, 2019 at 4:27 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Mon, 8 Apr 2019 15:42:55 +0200
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > > > That's fine for me, but I'm a bit surprised you did that without an ack
> > > > from the tracing people?
> > >
> > > oOOPS no I guess I shouldn't, haha I just didn't
> > > look close enough, I thought for some reason it only
> > > applied in the GPIO subsystem. I'll back this out.
> >
> > No need to back it out.
> >
> > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> OK applied to the GPIO tree.

I just found this patch in one of my older topic branches and it still
applies to current Linus (Torvald)'s master.

Given that the only user is gone since 5.2-rc1 (commit
12f2639038ef420fe796171ffb810b30d1ac0619)

For reference, here comes it again, slightly adapted to reality. (Use
git am --scissors to apply.)

Best regards
Uwe

---->8----
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Date: Fri, 5 Apr 2019 21:31:47 +0200
Subject: [PATCH] tracing: drop handling of NOTRACE symbol
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When a header defined the NOTRACE cpp symbol creation of tracepoints was
skipped. Since this feature was introduced in 4.4-rc1 it was only ever
used by the gpio events and this was dropped in v5.2-rc1. So
remove this now unused knob, also to not encourage a new usage.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/linux/tracepoint.h | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 1fb11daa5c53..7d03805503bd 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -136,18 +136,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 #define TP_ARGS(args...)	args
 #define TP_CONDITION(args...)	args
 
-/*
- * Individual subsystem my have a separate configuration to
- * enable their tracepoints. By default, this file will create
- * the tracepoints if CONFIG_TRACEPOINT is defined. If a subsystem
- * wants to be able to disable its tracepoints from being created
- * it can define NOTRACE before including the tracepoint headers.
- */
-#if defined(CONFIG_TRACEPOINTS) && !defined(NOTRACE)
-#define TRACEPOINTS_ENABLED
-#endif
-
-#ifdef TRACEPOINTS_ENABLED
+#ifdef CONFIG_TRACEPOINTS
 
 /*
  * it_func[0] is never NULL because there is at least one element in the array
@@ -295,7 +284,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 #define EXPORT_TRACEPOINT_SYMBOL(name)					\
 	EXPORT_SYMBOL(__tracepoint_##name)
 
-#else /* !TRACEPOINTS_ENABLED */
+#else /* !CONFIG_TRACEPOINTS */
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
 	static inline void trace_##name(proto)				\
 	{ }								\
@@ -327,7 +316,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
 #define EXPORT_TRACEPOINT_SYMBOL(name)
 
-#endif /* TRACEPOINTS_ENABLED */
+#endif /* CONFIG_TRACEPOINTS */
 
 #ifdef CONFIG_TRACING
 /**
-- 
2.20.1


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] tracing: drop handling of NOTRACE symbol

[Linus Walleij]

On Sat, Aug 24, 2019 at 4:04 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> Hello Linus and Steven,
(...)
> I just found this patch in one of my older topic branches and it still
> applies to current Linus (Torvald)'s master.
>
> Given that the only user is gone since 5.2-rc1 (commit
> 12f2639038ef420fe796171ffb810b30d1ac0619)
>
> For reference, here comes it again, slightly adapted to reality. (Use
> git am --scissors to apply.)

Sorry Uwe, it exploded last time for some reason that may have
been my fault, bad luck or something interim, or something that
has gone away since. But I had to pull it out and I guess I missed
to communicate about it.

I put it in a separate branch and push to the build servers so
we can see if something happens!

Yours,
Linus Walleij

Re: [PATCH 2/2] tracing: drop handling of NOTRACE symbol

[Linus Walleij]

On Fri, Sep 27, 2019 at 2:41 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> I put it in a separate branch and push to the build servers so
> we can see if something happens!

Oh there it is... buildserver says:

tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
 devel-drop-trace
branch HEAD: 40da9a24a778d94e034d160bfacf4931ae5922cd  tracing: drop
handling of NOTRACE symbol

Regressions in current branch:

kernel/trace/trace_events_filter.c:2105:14: error: field name not in
record or union initializer
kernel/trace/trace_events_filter.c:2115:44: error: field 'rec' has
incomplete type
kernel/trace/trace_events_filter.c:2121:2: note: in expansion of macro
'DATA_REC'
kernel/trace/trace_events_filter.c:2142:9: sparse: sparse: too many errors
kernel/trace/trace_events_filter.c:2222:29: error: passing argument 2
of 'create_filter' from incompatible pointer type
[-Werror=incompatible-pointer-types]
kernel/trace/trace_events_filter.c:2222:30: error:
'event_ftrace_test_filter' undeclared (first use in this function);
did you mean 'trace_ftrace_test_filter'?

Error ids grouped by kconfigs:

recent_errors
|-- arm-allmodconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|-- arm64-allmodconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|-- arm64-allyesconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|-- mips-allmodconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|-- s390-allmodconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|-- sh-allmodconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|-- sparc64-allmodconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|-- x86_64-allmodconfig
|   |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
|   |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
|   |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
|   |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
|   |-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC
|   `-- kernel-trace-trace_events_filter.c:sparse:sparse:too-many-errors
`-- x86_64-allyesconfig
    |-- kernel-trace-trace_events_filter.c:error:event_ftrace_test_filter-undeclared-(first-use-in-this-function)-did-you-mean-trace_ftrace_test_filter
    |-- kernel-trace-trace_events_filter.c:error:field-name-not-in-record-or-union-initializer
    |-- kernel-trace-trace_events_filter.c:error:field-rec-has-incomplete-type
    |-- kernel-trace-trace_events_filter.c:error:passing-argument-of-create_filter-from-incompatible-pointer-type
    `-- kernel-trace-trace_events_filter.c:note:in-expansion-of-macro-DATA_REC

I looked into this last time but couldn't wrap my head around it.

Yours,
Linus Walleij

Re: [PATCH 2/2] tracing: drop handling of NOTRACE symbol

[Steven Rostedt]

On Sat, 24 Aug 2019 16:04:26 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Hello Linus and Steven,
> 
> On Mon, Apr 08, 2019 at 04:29:15PM +0200, Linus Walleij wrote:
> > On Mon, Apr 8, 2019 at 4:27 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >   
> > > On Mon, 8 Apr 2019 15:42:55 +0200
> > > Linus Walleij <linus.walleij@linaro.org> wrote:
> > >  
> > > > > That's fine for me, but I'm a bit surprised you did that without an ack
> > > > > from the tracing people?  
> > > >
> > > > oOOPS no I guess I shouldn't, haha I just didn't
> > > > look close enough, I thought for some reason it only
> > > > applied in the GPIO subsystem. I'll back this out.  
> > >
> > > No need to back it out.
> > >
> > > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> > 
> > OK applied to the GPIO tree.  
> 
> I just found this patch in one of my older topic branches and it still
> applies to current Linus (Torvald)'s master.
> 
> Given that the only user is gone since 5.2-rc1 (commit
> 12f2639038ef420fe796171ffb810b30d1ac0619)
> 
> For reference, here comes it again, slightly adapted to reality. (Use
> git am --scissors to apply.)

I have some reservations against this patch. Mainly being that I'm
starting to think that we should be able to compile out individual
systems of trace events. The number of trace events continues to grow,
and there are many that I don't care about. This could be use to make
it easy to add configs that keep various trace event systems from being
compiled into the kernel.

-- Steve