2015-04-16 04:43:31

by Joonsoo Kim

Subject: [PATCH] tracing: fix incorrect enabling of trace events by boot cmdline

There is a problem that trace events are not properly enabled with
boot cmdline. Problem is that if we pass "trace_event=kmem:mm_page_alloc"
to boot cmdline, it enables all kmem trace events.

It is caused by parsing mechanism. When we parse cmdline, buffer
contents is modified due to tokenization. And, if we use this buffer
again, we will get wrong result.

Unfortunately, this buffer should be accessed three times
to set trace events properly in boot time. So, we need to handle
this situation.

There is already handling code for ",", but, we need another for
":". This patch add it.

Signed-off-by: Joonsoo Kim <[email protected]>
---
kernel/trace/trace_events.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index db54dda..ce5b194 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -565,6 +565,7 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
{
char *event = NULL, *sub = NULL, *match;
+ int ret;

/*
* The buf format can be <subsystem>:<event-name>
@@ -590,7 +591,11 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
event = NULL;
}

- return __ftrace_set_clr_event(tr, match, sub, event, set);
+ ret = __ftrace_set_clr_event(tr, match, sub, event, set);
+
+ /* Put back the colon to allow this to be called again */
+ if (buf)
+ *(buf - 1) = ':';
}

/**
--
1.9.1

2015-04-16 13:40:10

by Steven Rostedt

Subject: Re: [PATCH] tracing: fix incorrect enabling of trace events by boot cmdline

On Thu, 16 Apr 2015 13:44:44 +0900
Joonsoo Kim <[email protected]> wrote:

> There is a problem that trace events are not properly enabled with
> boot cmdline. Problem is that if we pass "trace_event=kmem:mm_page_alloc"
> to boot cmdline, it enables all kmem trace events.
>
> It is caused by parsing mechanism. When we parse cmdline, buffer
> contents is modified due to tokenization. And, if we use this buffer
> again, we will get wrong result.
>
> Unfortunately, this buffer should be accessed three times
> to set trace events properly in boot time. So, we need to handle
> this situation.
>
> There is already handling code for ",", but, we need another for
> ":". This patch add it.

Thanks, but your patch has a bug in it. I'll fix it up.

>
> Signed-off-by: Joonsoo Kim <[email protected]>
> ---
> kernel/trace/trace_events.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index db54dda..ce5b194 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -565,6 +565,7 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
> static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> {
> char *event = NULL, *sub = NULL, *match;
> + int ret;
>
> /*
> * The buf format can be <subsystem>:<event-name>
> @@ -590,7 +591,11 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> event = NULL;
> }
>
> - return __ftrace_set_clr_event(tr, match, sub, event, set);
> + ret = __ftrace_set_clr_event(tr, match, sub, event, set);
> +
> + /* Put back the colon to allow this to be called again */
> + if (buf)
> + *(buf - 1) = ':';

You forgot to add:

return ret;

-- Steve

> }
>
> /**

2015-04-17 05:09:31

by Joonsoo Kim

Subject: Re: [PATCH] tracing: fix incorrect enabling of trace events by boot cmdline

On Thu, Apr 16, 2015 at 09:39:52AM -0400, Steven Rostedt wrote:
> On Thu, 16 Apr 2015 13:44:44 +0900
> Joonsoo Kim <[email protected]> wrote:
>
> > There is a problem that trace events are not properly enabled with
> > boot cmdline. Problem is that if we pass "trace_event=kmem:mm_page_alloc"
> > to boot cmdline, it enables all kmem trace events.
> >
> > It is caused by parsing mechanism. When we parse cmdline, buffer
> > contents is modified due to tokenization. And, if we use this buffer
> > again, we will get wrong result.
> >
> > Unfortunately, this buffer should be accessed three times
> > to set trace events properly in boot time. So, we need to handle
> > this situation.
> >
> > There is already handling code for ",", but, we need another for
> > ":". This patch add it.
>
> Thanks, but your patch has a bug in it. I'll fix it up.
>
> >
> > Signed-off-by: Joonsoo Kim <[email protected]>
> > ---
> > kernel/trace/trace_events.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index db54dda..ce5b194 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -565,6 +565,7 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
> > static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> > {
> > char *event = NULL, *sub = NULL, *match;
> > + int ret;
> >
> > /*
> > * The buf format can be <subsystem>:<event-name>
> > @@ -590,7 +591,11 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> > event = NULL;
> > }
> >
> > - return __ftrace_set_clr_event(tr, match, sub, event, set);
> > + ret = __ftrace_set_clr_event(tr, match, sub, event, set);
> > +
> > + /* Put back the colon to allow this to be called again */
> > + if (buf)
> > + *(buf - 1) = ':';
>
> You forgot to add:
>
> return ret;
>

Ah... my bad... :)
thanks for fixing it.

Thanks.