From: Wei Wang <[email protected]>
trace_printk will cause trace_printk_init_buffers executed in kernel
start, which will increase memory and also show bad warnings in
production kernel.
Signed-off-by: Wei Wang <[email protected]>
---
include/linux/kernel.h | 17 +++++++++++++++++
include/linux/trace_events.h | 4 ++++
2 files changed, 21 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 6a1eb0b0aad96..f1a003e5986a9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -677,6 +677,7 @@ do { \
* let gcc optimize the rest.
*/
+#if defined(DEBUG)
#define trace_printk(fmt, ...) \
do { \
char _______STR[] = __stringify((__VA_ARGS__)); \
@@ -685,7 +686,11 @@ do { \
else \
trace_puts(fmt); \
} while (0)
+#else
+#define trace_printk(fmt, ...)
+#endif
+#if defined(DEBUG)
#define do_trace_printk(fmt, args...) \
do { \
static const char *trace_printk_fmt __used \
@@ -699,6 +704,9 @@ do { \
else \
__trace_printk(_THIS_IP_, fmt, ##args); \
} while (0)
+#else
+#define do_trace_printk(fmt, args...)
+#endif
extern __printf(2, 3)
int __trace_bprintk(unsigned long ip, const char *fmt, ...);
@@ -731,6 +739,7 @@ int __trace_printk(unsigned long ip, const char *fmt, ...);
* (1 when __trace_bputs is used, strlen(str) when __trace_puts is used)
*/
+#if defined(DEBUG)
#define trace_puts(str) ({ \
static const char *trace_printk_fmt __used \
__attribute__((section("__trace_printk_fmt"))) = \
@@ -741,6 +750,10 @@ int __trace_printk(unsigned long ip, const char *fmt, ...);
else \
__trace_puts(_THIS_IP_, str, strlen(str)); \
})
+#else
+#define trace_puts(str)
+#endif
+
extern int __trace_bputs(unsigned long ip, const char *str);
extern int __trace_puts(unsigned long ip, const char *str, int size);
@@ -751,6 +764,7 @@ extern void trace_dump_stack(int skip);
* if we try to allocate the static variable to fmt if it is not a
* constant. Even with the outer if statement.
*/
+#if defined(DEBUG)
#define ftrace_vprintk(fmt, vargs) \
do { \
if (__builtin_constant_p(fmt)) { \
@@ -762,6 +776,9 @@ do { \
} else \
__ftrace_vprintk(_THIS_IP_, fmt, vargs); \
} while (0)
+#else
+#define ftrace_vprintk(fmt, vargs)
+#endif
extern __printf(2, 0) int
__ftrace_vbprintk(unsigned long ip, const char *fmt, va_list ap);
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 2bde3eff564cd..433208a1c6009 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -533,6 +533,7 @@ int trace_set_clr_event(const char *system, const char *event, int set);
* if we try to allocate the static variable to fmt if it is not a
* constant. Even with the outer if statement optimizing out.
*/
+#if defined(DEBUG)
#define event_trace_printk(ip, fmt, args...) \
do { \
__trace_printk_check_format(fmt, ##args); \
@@ -546,6 +547,9 @@ do { \
} else \
__trace_printk(ip, fmt, ##args); \
} while (0)
+#else
+#define event_trace_printk(ip, fmt, args...)
+#endif
#ifdef CONFIG_PERF_EVENTS
struct perf_event;
--
2.17.0.484.g0c8726318c-goog
On Tue, 24 Apr 2018 11:08:10 -0700
Wei Wang <[email protected]> wrote:
> From: Wei Wang <[email protected]>
>
> trace_printk will cause trace_printk_init_buffers executed in kernel
> start, which will increase memory and also show bad warnings in
> production kernel.
>
NAK!
Where is trace_printk() being used to trigger this?
trace_printk() is to be added while debugging, but one should not need
to enable DEBUG to use it. But it should be removed when finished.
-- Steve
> Signed-off-by: Wei Wang <[email protected]>
We have seen many cases vendor have shipped kernel/drivers with it, and
have to clean up that every year. This was brought up in an internal
discussion and Greg suggested have some feedback from upstream about what
should be taken to prevent this globally besides fixing individual drivers.
From him "I think this change makes sense at a high level, but there could
be non-obvious reasons why this isn't the way things are handled right now."
On Tue, Apr 24, 2018 at 11:51 AM Steven Rostedt <[email protected]> wrote:
> On Tue, 24 Apr 2018 11:08:10 -0700
> Wei Wang <[email protected]> wrote:
> > From: Wei Wang <[email protected]>
> >
> > trace_printk will cause trace_printk_init_buffers executed in kernel
> > start, which will increase memory and also show bad warnings in
> > production kernel.
> >
> NAK!
> Where is trace_printk() being used to trigger this?
> trace_printk() is to be added while debugging, but one should not need
> to enable DEBUG to use it. But it should be removed when finished.
Yeah, ideal case is it *should* be removed, but if this is not for
production use, does it make sense we limit the usage with DEBUG?
> -- Steve
> > Signed-off-by: Wei Wang <[email protected]>
On Tue, 24 Apr 2018 19:02:34 +0000
Wei Wang <[email protected]> wrote:
> We have seen many cases vendor have shipped kernel/drivers with it, and
> have to clean up that every year. This was brought up in an internal
> discussion and Greg suggested have some feedback from upstream about what
> should be taken to prevent this globally besides fixing individual drivers.
> From him "I think this change makes sense at a high level, but there could
> be non-obvious reasons why this isn't the way things are handled right now."
The thing is, trace_printk() should not be used except for development
and debugging. There should be no use cases in the kernel that us it,
unless it's part of something else that should never be used (I use it
for the ring buffer benchmark which itself will destabilize the
system and is why I use it - I want that warning for it too).
Any trace_printk() in a patch submitted to the kernel should simply be
stripped out. If someone wants a trace_printk() in their code, then
they should create a trace event, which is the proper way of retrieving
static data from the kernel. Using trace_printk() is just a lazy fast
way to create trace events.
Hmm, ideally I think check_patch needs to add a warning if
trace_printk() is used.
-- Steve
checkpatch.pl sounds good. One thing to add is we have many off tree
patches with abuse trace_printk. Also as you mentioned, given this is
really not for use in production and we have been cleaning this our on our
side for years, could we consider to enforce this in kernel?
Thanks!
Wei
On Tue, Apr 24, 2018 at 12:14 PM Steven Rostedt <[email protected]> wrote:
> On Tue, 24 Apr 2018 19:02:34 +0000
> Wei Wang <[email protected]> wrote:
> > We have seen many cases vendor have shipped kernel/drivers with it, and
> > have to clean up that every year. This was brought up in an internal
> > discussion and Greg suggested have some feedback from upstream about
what
> > should be taken to prevent this globally besides fixing individual
drivers.
> > From him "I think this change makes sense at a high level, but there
could
> > be non-obvious reasons why this isn't the way things are handled right
now."
> The thing is, trace_printk() should not be used except for development
> and debugging. There should be no use cases in the kernel that us it,
> unless it's part of something else that should never be used (I use it
> for the ring buffer benchmark which itself will destabilize the
> system and is why I use it - I want that warning for it too).
> Any trace_printk() in a patch submitted to the kernel should simply be
> stripped out. If someone wants a trace_printk() in their code, then
> they should create a trace event, which is the proper way of retrieving
> static data from the kernel. Using trace_printk() is just a lazy fast
> way to create trace events.
> Hmm, ideally I think check_patch needs to add a warning if
> trace_printk() is used.
> -- Steve
On Tue, 24 Apr 2018 19:20:03 +0000
Wei Wang <[email protected]> wrote:
> checkpatch.pl sounds good. One thing to add is we have many off tree
> patches with abuse trace_printk. Also as you mentioned, given this is
> really not for use in production and we have been cleaning this our on our
> side for years, could we consider to enforce this in kernel?
That nasty warning was suppose to be the enforcement. I would expect
nobody would ship a kernel where it produced such a message on boot (or
loading of a module). If they don't notice, then they are not testing
their code.
A lot of kernel developers use trace_printk() and I want to make it as
easy to use as possible. I don't want to add a config to enable it,
because that would be something that could be rather annoying.
Let's add it to checkpatch and see if that can draining the swamp of
abusers.
-- Steve
On Tue, Apr 24, 2018, 12:26 Steven Rostedt <[email protected]> wrote:
> On Tue, 24 Apr 2018 19:20:03 +0000
> Wei Wang <[email protected]> wrote:
> > checkpatch.pl sounds good. One thing to add is we have many off tree
> > patches with abuse trace_printk. Also as you mentioned, given this is
> > really not for use in production and we have been cleaning this our on
our
> > side for years, could we consider to enforce this in kernel?
> That nasty warning was suppose to be the enforcement. I would expect
> nobody would ship a kernel where it produced such a message on boot (or
> loading of a module). If they don't notice, then they are not testing
> their code.
> A lot of kernel developers use trace_printk() and I want to make it as
> easy to use as possible. I don't want to add a config to enable it,
> because that would be something that could be rather annoying.
The config is not something new and it is controlling pr_debug and
pr_devel, so might not be too annoying, IMHO. But I agree this is not a
problem from us but from abusers.
Thanks!
-Wei
> Let's add it to checkpatch and see if that can draining the swamp of
> abusers.
> -- Steve
On Tue, 24 Apr 2018 20:39:27 +0000
Wei Wang <[email protected]> wrote:
> The config is not something new and it is controlling pr_debug and
> pr_devel, so might not be too annoying, IMHO. But I agree this is not a
> problem from us but from abusers.
And is the reason I never use pr_debug() and pr_devel().
-- Steve
On Tue, 24 Apr 2018 16:45:05 -0400
Steven Rostedt <[email protected]> wrote:
> On Tue, 24 Apr 2018 20:39:27 +0000
> Wei Wang <[email protected]> wrote:
>
> > The config is not something new and it is controlling pr_debug and
> > pr_devel, so might not be too annoying, IMHO. But I agree this is not a
> > problem from us but from abusers.
>
> And is the reason I never use pr_debug() and pr_devel().
>
Which reminds me, I need to replace pr_debug() to pr_info() for this
line:
pr_debug("syscall %s metadata not mapped, disabling ftrace event\n",
In trace_syscalls.c (I didn't add that), as a different commit that
changed syscall names for x86 caused all syscall tracepoints to
disappear. And if this was pr_info() and not pr_debug() the one that
made the change would have noticed his change had impact someplace else.
-- Steve
I have seen case when LLVM appends postfix to most function names, causing
func such as tracing_mark_write becomes tracing_mark_write.XXX which messed
all post-processing.
Also I think this is a typo?
https://lkml.org/lkml/2018/4/24/1176
On Tue, Apr 24, 2018 at 1:48 PM Steven Rostedt <[email protected]> wrote:
> On Tue, 24 Apr 2018 16:45:05 -0400
> Steven Rostedt <[email protected]> wrote:
> > On Tue, 24 Apr 2018 20:39:27 +0000
> > Wei Wang <[email protected]> wrote:
> >
> > > The config is not something new and it is controlling pr_debug and
> > > pr_devel, so might not be too annoying, IMHO. But I agree this is not
a
> > > problem from us but from abusers.
> >
> > And is the reason I never use pr_debug() and pr_devel().
> >
> Which reminds me, I need to replace pr_debug() to pr_info() for this
> line:
> pr_debug("syscall %s metadata not mapped, disabling ftrace event\n",
> In trace_syscalls.c (I didn't add that), as a different commit that
> changed syscall names for x86 caused all syscall tracepoints to
> disappear. And if this was pr_info() and not pr_debug() the one that
> made the change would have noticed his change had impact someplace else.
> -- Steve
On 04/24/2018 01:45 PM, Steven Rostedt wrote:
> On Tue, 24 Apr 2018 20:39:27 +0000
> Wei Wang <[email protected]> wrote:
>
>> The config is not something new and it is controlling pr_debug and
>> pr_devel, so might not be too annoying, IMHO. But I agree this is not a
>> problem from us but from abusers.
>
> And is the reason I never use pr_debug() and pr_devel().
pr_debug() is good along with dynamic debug.
--
~Randy
On Tue, Apr 24, 2018 at 12:26 PM Steven Rostedt <[email protected]> wrote:
> A lot of kernel developers use trace_printk() and I want to make it as
> easy to use as possible. I don't want to add a config to enable it,
> because that would be something that could be rather annoying.
> Let's add it to checkpatch and see if that can draining the swamp of
> abusers.
Currently I see f2fs trace is using this when having CONFIG_F2FS_IO_TRACE,
so I am not sure how checkpatch would work. How about we add a BUILD_BUG
surrounded by a config which would let us flag abuse easily on build time?
> -- Steve
On Tue, Apr 24, 2018 at 07:02:34PM +0000, Wei Wang wrote:
> We have seen many cases vendor have shipped kernel/drivers with it, and
> have to clean up that every year. This was brought up in an internal
> discussion and Greg suggested have some feedback from upstream about what
> should be taken to prevent this globally besides fixing individual drivers.
> From him "I think this change makes sense at a high level, but there could
> be non-obvious reasons why this isn't the way things are handled right now."
I said that? Heh, I normally say things like "fix all the stupid
drivers", you must have caught me on a good day :)
Anyway, the drivers should be fixed, if they are doing foolish things
like this, the core kernel should not be "protecting them from
themselves", that way lies madness, just talk to the Windows kernel
developers about the hoops they jump through for this type of stuff...
thanks,
greg k-h
On Wed, 25 Apr 2018 04:53:33 +0000
Wei Wang <[email protected]> wrote:
> On Tue, Apr 24, 2018 at 12:26 PM Steven Rostedt <[email protected]> wrote:
> > A lot of kernel developers use trace_printk() and I want to make it as
> > easy to use as possible. I don't want to add a config to enable it,
> > because that would be something that could be rather annoying.
>
> > Let's add it to checkpatch and see if that can draining the swamp of
> > abusers.
>
>
> Currently I see f2fs trace is using this when having CONFIG_F2FS_IO_TRACE,
> so I am not sure how checkpatch would work. How about we add a BUILD_BUG
> surrounded by a config which would let us flag abuse easily on build time?
I don't want a config to have to be set for adding this. That would
really irritate myself, as I constantly take configs from others for
debugging purposes and then slam trace_printk() all over the place. It
would be annoying to have to remember to enable a config. And having a
config would also change the way the kernel gets built, and for
debugging the less variables the better.
But you are correct. I see lots of abusers with trace_printk(). I think
it's time for me to start removing them.
-- Steve