Re: [RFC PATCH 1/1] trace: Move trace event enable from fs_initcall to early_initcall

[Ezequiel Garcia <elezegarcia@gmail.com>]

Hi Steven,

On Thu, Aug 16, 2012 at 6:18 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 2012-08-15 at 12:18 -0300, Ezequiel Garcia wrote:
>> This patch splits trace event initialization in two stages:
>>  * ftrace enable
>>  * sysfs event entry creation
>>
>> This allows to capture trace events from an earlier point
>> by using 'trace_event' kernel parameter and is important
>> to trace boot-up allocations.
>>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
>> ---
>>  kernel/trace/trace_events.c |   71 ++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 53 insertions(+), 18 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 29111da..3055bc9 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -1450,6 +1450,49 @@ static __init int setup_trace_event(char *str)
>>  }
>>  __setup("trace_event=", setup_trace_event);
>>
>> +static __init int event_trace_enable(void)
>> +{
>> +     struct ftrace_event_call **iter, *call;
>> +     char *buf = bootup_event_buf;
>> +     char *token;
>> +     int ret;
>> +
>> +     for_each_event(iter, __start_ftrace_events, __stop_ftrace_events) {
>> +
>> +             call = *iter;
>> +
>> +             /* The linker may leave blanks */
>
> Actually, this shouldn't be true anymore. I know you copied the comment,
> but with the new pointer settings, there should be no blanks. We
> probably should turn this into a WARN_ON().
>
>
>> +             if (!call->name)
>> +                     continue;
>> +
>> +             if (!call->class->raw_init)
>> +                     continue;
>> +
>> +             ret = call->class->raw_init(call);
>> +             if (ret < 0) {
>> +                     if (ret != -ENOSYS)
>> +                             pr_warning("Could not initialize trace events/%s\n",
>> +                                             call->name);
>> +                     continue;
>> +             }
>> +             list_add(&call->list, &ftrace_events);
>
> Anyway, this code is duplicated now here and in
> __trace_add_event_call(). A helper function should be created to store
> this in both locations.
>
>> +     }
>> +
>> +     while (true) {
>> +             token = strsep(&buf, ",");
>> +
>> +             if (!token)
>> +                     break;
>> +             if (!*token)
>> +                     continue;
>> +
>> +             ret = ftrace_set_clr_event(token, 1);
>> +             if (ret)
>> +                     pr_warning("Failed to enable trace event: %s\n", token);
>> +     }
>> +     return 0;
>> +}
>> +
>>  static __init int event_trace_init(void)
>>  {
>>       struct ftrace_event_call **call;
>> @@ -1457,8 +1500,6 @@ static __init int event_trace_init(void)
>>       struct dentry *entry;
>>       struct dentry *d_events;
>>       int ret;
>> -     char *buf = bootup_event_buf;
>> -     char *token;
>>
>>       d_tracer = tracing_init_dentry();
>>       if (!d_tracer)
>> @@ -1497,24 +1538,17 @@ static __init int event_trace_init(void)
>>       if (trace_define_common_fields())
>>               pr_warning("tracing: Failed to allocate common fields");
>>
>> +     /*
>> +      * Early initialization already enabled ftrace event.
>> +      * Now it's only necessary to create the event directory.
>> +      */
>>       for_each_event(call, __start_ftrace_events, __stop_ftrace_events) {
>> -             __trace_add_event_call(*call, NULL, &ftrace_event_id_fops,
>> -                                    &ftrace_enable_fops,
>> -                                    &ftrace_event_filter_fops,
>> -                                    &ftrace_event_format_fops);
>> -     }
>> -
>> -     while (true) {
>> -             token = strsep(&buf, ",");
>> -
>> -             if (!token)
>> -                     break;
>> -             if (!*token)
>> -                     continue;
>>
>> -             ret = ftrace_set_clr_event(token, 1);
>> -             if (ret)
>> -                     pr_warning("Failed to enable trace event: %s\n", token);
>> +             event_create_dir(*call, d_events,
>> +                              &ftrace_event_id_fops,
>> +                              &ftrace_enable_fops,
>> +                              &ftrace_event_filter_fops,
>> +                              &ftrace_event_format_fops);
>
> This changes how events are currently. If we fail to create the
> directory, we still keep the event. This can become confusing as events
> and the directories will not match anymore. A failure should be checked
> for, and if it happens, the event should be removed.
>

Thanks for your comments, I'll send a v2 addressing them.

Regarding the 'complete solution': to be able to capture events from
the very beggining...
Have you thought about this?
Could you give me a hint on how could I implement it?

Thanks a lot,
Ezequiel.