Re: [PATCH 2/3] ftrace: Add debug_print trace to print data from kernel to userspace

[Frédéric Weisbecker]

Hi Aneesh!

2008/11/14 Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>:
> The trace add a new interface debug_print() which can be used
> to dump data from kernel to user space using ftrace framework.


The actual "nop tracer" which is the default selected (if trace_boot
is not configured) let
the tracing engine able to receive and handle TRACE_PRINT events.
Even if nop tracer doesn't handle its output itself, the TRACE_PRINT
event output is relayed by
print_trace_fmt() in trace.c

Your output does almost the same but it is already implemented.



> +static void dp_trace_ctrl_update(struct trace_array *tr)
> +{
> +       /* When starting a new trace, reset the buffers */
> +       if (tr->ctrl)
> +               start_dp_trace(tr);
> +       else
> +               stop_dp_trace(tr);
> +}


BTW, ctrl_update() have been removed very recently.
Perhaps are you implementing this against the mainline? Its a better idea to
submit a new tracer against latest -tip tree.

> +
> +#ifdef CONFIG_NOP_TRACER
> +int
> +trace_selftest_startup_dp(struct tracer *trace, struct trace_array *tr)
> +{
> +       /* What could possibly go wrong? */
> +       return 0;
> +}
> +#endif


CONFIG_NOP_TRACER ?
Wouldn't it rather depend on CONFIG_FTRACE_SELFTEST? :-)

That would perhaps have been better to raise a ftrace_printk to make a test.
If you don't do anything in your selftest, then it is unnecessary to
implement one for
your tracer.


> +static enum print_line_t debug_print_line(struct trace_iterator *iter)
> +{
> +       struct trace_seq *s = &iter->seq;
> +       struct trace_entry *entry;
> +
> +       entry = iter->ent;
> +       switch (entry->type) {
> +       case TRACE_PRINT: {
> +               struct print_entry *field;
> +               trace_assign_type(field, entry);
> +
> +               trace_seq_printf(s, "%s", field->buf);
> +               if (entry->flags & TRACE_FLAG_CONT)
> +                       trace_seq_print_cont(s, iter);
> +               break;


You should test if trace_seq_printf successed to print the whole line.
If not, that's better to return TRACE_TYPE_PARTIAL_LINE, then the
output will be retried later.



> +       }
> +       default:
> +               printk(KERN_INFO "Unsupported type in debug_print\n");
> +               return TRACE_TYPE_UNHANDLED;
> +       }
> +
> +       return TRACE_TYPE_HANDLED;
> +}
> +
> +struct tracer dp_trace __read_mostly =
> +{
> +       .name           = "debug_print",
> +       .init           = dp_trace_init,
> +       .reset          = dp_trace_reset,
> +       .ctrl_update    = dp_trace_ctrl_update,
> +#ifdef CONFIG_FTRACE_SELFTEST
> +       .selftest       = trace_selftest_startup_dp,
> +#endif
> +       .print_line     = debug_print_line,
> +};
> +
> +__init static int init_dp_trace(void)
> +{
> +       return register_tracer(&dp_trace);
> +}
> +device_initcall(init_dp_trace);


Primarily, such a debug tracer is not a bad idea, IMHO.
And note that all of I just wrote in this answer in only my opinion.
Perhaps other people
would find other uses of this tracer that actual default output of the
tracing engine doesn't handle well, or trace.c is
would not be the right place for further enhancements that could
happen on debug entries if you need to.

But the actual simple output that you are submitting along this tracer
is already handled by the default output of the tracing internals.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/