Re: [PATCH 3/3] ftrace: Add debug_dump trace to dump binary data from kernel to userspace

From: Frédéric Weisbecker
Date: Fri Nov 14 2008 - 06:54:33 EST


2008/11/14 Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>:
> +struct trace_array *trace_get_global_trace(void)
> +{
> + return &global_trace;
> +}


I'm not sure this is necessary to export the global_trace.
It is transmitted through init callback of a tracer (struct trace_array *tr).
You have a ctx_trace that you make pointing to global_trace using tr.
And after that you never use it.
That would be better to use your ctx_trace instead of exporting such function.

> +int debug_dump(void *bin_data, int len)
> +{
> + struct ring_buffer_event *event;
> + struct trace_array *tr;
> + struct trace_array_cpu *data;
> + struct dump_entry *entry;
> + unsigned long flags, irq_flags;
> + int cpu, size, pc;
> +
> + if (!tracer_enabled)
> + return 0;
> + tr = trace_get_global_trace();
> + pc = preempt_count();
> + preempt_disable_notrace();
> + cpu = raw_smp_processor_id();
> + data = tr->data[cpu];
> +
> + local_irq_save(flags);
> + size = sizeof(*entry) + len;
> + event = ring_buffer_lock_reserve(tr->buffer, size, &irq_flags);
> + if (!event)
> + goto out;
> + entry = ring_buffer_event_data(event);
> + tracing_generic_entry_update(&entry->ent, flags, pc);
> + entry->ent.type = TRACE_BIN_DUMP;
> + entry->len = len;
> +
> + memcpy(&entry->buf, bin_data, len);
> + ring_buffer_unlock_commit(tr->buffer, event, irq_flags);
> +
> +out:
> + local_irq_restore(flags);
> + preempt_enable_notrace();
> +
> + return len;
> +}
> +EXPORT_SYMBOL_GPL(debug_dump);


I don't think this is necessary here to protect against preemption or
interrupts.
IE: it is necessary for function tracing to avoid recursion. But for
other usual cases
I think this it is not needed. If you thought about protecting the
ring buffer, it is already
able to protect itself :-)


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


As I said, this callback recently disappeared.


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


NOP_TRACER...copy-pasting is not always our friend... :-)


> +static enum print_line_t debug_dump_entry(struct trace_iterator *iter)
> +{
> + struct trace_seq *s = &iter->seq;
> + struct trace_entry *entry;
> +
> + entry = iter->ent;
> + switch (entry->type) {
> + case TRACE_BIN_DUMP: {
> + struct dump_entry *field;
> + trace_assign_type(field, entry);
> + trace_seq_putmem(s, field->buf, field->len);
> + break;


Don't forget the TRACE_TYPE_PRINT_LINE...
--
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/