Re: [PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

[Chunyan Zhang]

Hello Steve,

Please correct me if I'm missing something in my following reply.

On 15 August 2016 at 22:28, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Mon, 15 Aug 2016 19:50:01 +0800
> Chunyan Zhang <zhang.chunyan@xxxxxxxxxx> wrote:
>
>> +int register_trace_export(struct trace_export *export);
>> +int unregister_trace_export(struct trace_export *export);
>> +
>> +#endif       /* CONFIG_TRACING */
>> +
>> +#endif       /* _LINUX_TRACE_H */
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index dade4c9..0247ac2 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -40,6 +40,7 @@
>>  #include <linux/poll.h>
>>  #include <linux/nmi.h>
>>  #include <linux/fs.h>
>> +#include <linux/trace.h>
>>  #include <linux/sched/rt.h>
>>
>>  #include "trace.h"
>> @@ -2128,6 +2129,127 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
>>       ftrace_trace_userstack(buffer, flags, pc);
>>  }
>>
>> +static inline void
>> +trace_generic_commit(struct trace_array *tr,
>> +            struct ring_buffer_event *event)
>
> Why is this marked inline? It is never called directly here.
>
>> +{
>> +     struct trace_entry *entry;
>> +     struct trace_export *export = tr->export;
>> +     unsigned int size = 0;
>> +
>> +     entry = ring_buffer_event_data(event);
>> +
>> +     trace_entry_size(size, entry->type);
>> +     if (!size)
>> +             return;
>> +
>> +     if (export->write)
>> +             export->write((char *)entry, size);
>> +}
>> +
>> +static inline void
>
> Same here.
>
>> +trace_rb_commit(struct trace_array *tr,
>> +            struct ring_buffer_event *event)
>> +{
>> +     __buffer_unlock_commit(tr->trace_buffer.buffer, event);
>> +}
>> +
>> +static DEFINE_MUTEX(trace_export_lock);
>> +
>> +static struct trace_export trace_export_rb __read_mostly = {
>> +     .name           = "rb",
>> +     .commit = trace_rb_commit,
>
> Make sure equal signs are lined up.
>
>> +     .next           = NULL,
>> +};
>> +static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;
>
> trace_exports_list needs to be annotated as rcu, if you are going to
> dereference it via rcu_dereference. That was the kbuild warning you
> received.

Ok, will do.

But I guess 'ftrace_ops_list' in ftrace.c [1] also should be annotated as rcu?

[1] http://lxr.free-electrons.com/source/kernel/trace/ftrace.c#L460

>
>> +
>> +inline void
>> +trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
>> +{
>> +     struct trace_export *export;
>> +
>> +     preempt_disable_notrace();
>> +
>> +     for (export = rcu_dereference_raw_notrace(trace_exports_list);
>> +          export && export->commit;
>> +          export = rcu_dereference_raw_notrace(export->next)) {
>> +             tr->export = export;
>> +             export->commit(tr, event);
>> +     }
>> +
>> +     preempt_enable_notrace();
>> +}
>
> I'm not too happy about the added overhead to normal function tracing
> (it will be noticeable), but I can fix that later.

I think I will try to revise here in the next revision.

>
>> +
>> +static void
>> +add_trace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     export->next = *list;
>
> As export->next will most likely need to be marked __rcu as well, this
> may need an rcu_assign_pointer() too.
>
>> +     /*
>> +      * We are entering export into the list but another
>> +      * CPU might be walking that list. We need to make sure
>> +      * the export->next pointer is valid before another CPU sees
>> +      * the export pointer included into the list.
>> +      */
>> +     rcu_assign_pointer(*list, export);
>> +
>> +}
>> +
>> +static int
>> +rm_trace_export(struct trace_export **list, struct trace_export *export)
>> +{
>> +     struct trace_export **p;
>> +
>> +     for (p = list; *p != &trace_export_rb; p = &(*p)->next)
>> +             if (*p == export)
>> +                     break;
>> +
>> +     if (*p != export)
>> +             return -1;
>> +
>> +     *p = (*p)->next;
>
> I believe this requires an rcu_assign_pointer() as well.
>
>> +
>> +     return 0;
>> +}
>> +
>> +int register_trace_export(struct trace_export *export)
>> +{
>> +     if (!export->write) {
>> +             pr_warn("trace_export must have the write() call back.\n");
>
> Probably should be a "WARN_ON_ONCE()".

Yes, will revise.

>
>> +             return -1;
>> +     }
>> +
>> +     if (!export->name) {
>> +             pr_warn("trace_export must have a name.\n");
>> +             return -1;
>> +     }
>
> If name isn't used don't add it till it is. That is, this patch should
> not have "name" in the structure. It's confusing, because I don't see a
> purpose for it.

Ok will remove that.

>
>> +
>> +     mutex_lock(&trace_export_lock);
>> +
>> +     export->tr = trace_exports_list->tr;
>
> I don't see where tr is ever assigned.
>
>> +     export->commit = trace_generic_commit;
>
> Shouldn't the caller pass in the commit function too?

The trace_export::write() callback is for caller, commit function
mainly deal with traces, is it better to keep 'trace_generic_commit'
in trace.c, i.e don't expose it to callers?

>
>> +
>> +     add_trace_export(&trace_exports_list, export);
>> +
>> +     mutex_unlock(&trace_export_lock);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(register_trace_export);
>> +
>> +int unregister_trace_export(struct trace_export *export)
>> +{
>> +     int ret;
>> +
>> +     mutex_lock(&trace_export_lock);
>> +
>> +     ret = rm_trace_export(&trace_exports_list, export);
>> +
>> +     mutex_unlock(&trace_export_lock);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(unregister_trace_export);
>> +
>>  void
>>  trace_function(struct trace_array *tr,
>>              unsigned long ip, unsigned long parent_ip, unsigned long flags,
>> @@ -2147,7 +2269,7 @@ trace_function(struct trace_array *tr,
>>       entry->parent_ip                = parent_ip;
>>
>>       if (!call_filter_check_discard(call, entry, buffer, event))
>
> How do you handle the discard? If the entry doesn't match the filter,
> it will try to discard the event. I don't see how the "trace_exports"
> handle that.

I directly used the entries which had already been filtered.
Humm.. sorry, actually you lost me here.

>
>
>> -             __buffer_unlock_commit(buffer, event);
>> +             trace_exports(tr, event);
>>  }
>>
>>  #ifdef CONFIG_STACKTRACE
>> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
>> index f783df4..a40f07c 100644
>> --- a/kernel/trace/trace.h
>> +++ b/kernel/trace/trace.h
>> @@ -260,6 +260,7 @@ struct trace_array {
>>       /* function tracing enabled */
>>       int                     function_enabled;
>>  #endif
>> +     struct trace_export     *export;
>>  };
>>
>>  enum {
>> @@ -301,6 +302,13 @@ static inline struct trace_array *top_trace_array(void)
>>               break;                                  \
>>       }
>>
>> +#undef IF_SIZE
>> +#define IF_SIZE(size, var, etype, id)                \
>> +             if (var == id) {                \
>> +                     size = (sizeof(etype)); \
>> +                     break;                  \
>> +             }
>> +
>>  /* Will cause compile errors if type is not found. */
>>  extern void __ftrace_bad_type(void);
>>
>> @@ -339,6 +347,29 @@ extern void __ftrace_bad_type(void);
>>       } while (0)
>>
>>  /*
>> + * The trace_entry_size return the size of specific trace type
>> + *
>> + * IF_SIZE(size, var);
>> + *
>> + * Where "var" is just the given trace type.
>> + */
>> +#define trace_entry_size(size, var)                                  \
>> +     do {                                                            \
>> +             IF_SIZE(size, var, struct ftrace_entry, TRACE_FN);      \
>> +             IF_SIZE(size, var, struct stack_entry, TRACE_STACK);    \
>> +             IF_SIZE(size, var, struct userstack_entry,              \
>> +                     TRACE_USER_STACK);                              \
>> +             IF_SIZE(size, var, struct print_entry, TRACE_PRINT);    \
>> +             IF_SIZE(size, var, struct bprint_entry, TRACE_BPRINT);  \
>> +             IF_SIZE(size, var, struct bputs_entry, TRACE_BPUTS);    \
>> +             IF_SIZE(size, var, struct trace_branch, TRACE_BRANCH);  \
>> +             IF_SIZE(size, var, struct ftrace_graph_ent_entry,       \
>> +                     TRACE_GRAPH_ENT);                               \
>> +             IF_SIZE(size, var, struct ftrace_graph_ret_entry,       \
>> +                     TRACE_GRAPH_RET);                               \
>
> I really really dislike this. This is a big if statement that all
> needs to go down one by one. Very inefficient!

Ok, will change to other implementation.


Thanks for your comments,
Chunyan

>
> -- Steve
>
>> +     } while (0)
>> +
>> +/*
>>   * An option specific to a tracer. This is a boolean value.
>>   * The bit is the bit index that sets its value on the
>>   * flags value in struct tracer_flags.
>