lkml.org 
[lkml]   [2008]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ftrace: Add a C-state tracer to help power optimization

Hi Arjan,

Very nice! I just have a few comments below.

On Fri, 3 Oct 2008, Arjan van de Ven wrote:
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index a921ba5..1ef1ded 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -117,6 +117,11 @@ struct trace_boot {
> struct boot_trace initcall;
> };
>
> +struct trace_cstate {
> + struct trace_entry ent;
> + struct cstate_trace state_data;
> +};
> +

Can you please register this in the trace_assign_type macro.

IF_ASSIGN(var, ent, struct trace_cstate, TRACE_BOOT); \



> /*
> * trace_flag_type is an enumeration that holds different
> * states when a trace occurs. These are:
> diff --git a/kernel/trace/trace_cstate.c b/kernel/trace/trace_cstate.c
> new file mode 100644
> index 0000000..fcd4e6e
> --- /dev/null
> +++ b/kernel/trace/trace_cstate.c


> +static enum print_line_t cstate_print_line(struct trace_iterator *iter)
> +{
> + int ret;
> + struct trace_entry *entry = iter->ent;
> + struct trace_cstate *field = (struct trace_cstate *)entry;

And here, do not typecast. Use the trace_assign_type macro below.

> + struct cstate_trace *it = &field->state_data;
> + struct trace_seq *s = &iter->seq;
> + struct timespec stamp = ktime_to_timespec(it->stamp);

Also note that iter->ts holds a timestamp counter from bootup in nanosecs.
It currently uses the sched_clock to record, but may change later to
something a bit better.

> + struct timespec duration = ktime_to_timespec(
> + ktime_sub(it->end, it->stamp));


trace_assign_type(field, entry);

The above will do a typecheck to force correct types.

But, you may not need to do any of this. See below.

> +
> + if (entry->type == TRACE_BOOT) {
> + ret = trace_seq_printf(s, "[%5ld.%09ld] Going to C%i on cpu %i for %ld.%09ld\n",
> + stamp.tv_sec,
> + stamp.tv_nsec,
> + it->state, it->CPU,
> + duration.tv_sec,
> + duration.tv_nsec);
> + if (!ret)
> + return TRACE_TYPE_PARTIAL_LINE;
> + return TRACE_TYPE_HANDLED;
> + }
> + return TRACE_TYPE_UNHANDLED;
> +}
> +
> +struct tracer cstate_tracer __read_mostly =
> +{
> + .name = "cstate",
> + .init = cstate_trace_init,
> + .reset = stop_cstate_trace,
> + .ctrl_update = cstate_trace_ctrl_update,
> + .print_line = cstate_print_line,

Here you bypass the print_line altogether. You really don't even need
to make a trace_cstate. You do not use any of the data in the entry field
so you can simply just store your cstate_trace data directly into the
buffer. The entry is only used for ftrace formatted prints.


> +};
> +
> +static int init_cstate_trace(void)
> +{
> + return register_tracer(&cstate_tracer);
> +}
> +device_initcall(init_cstate_trace);
> +
> +void trace_cstate(struct cstate_trace *it, int level)
> +{
> + struct ring_buffer_event *event;
> + struct trace_cstate *entry;
> + struct trace_array_cpu *data;
> + unsigned long irq_flags;
> + struct trace_array *tr = cstate_trace;
> +
> + if (!trace_cstate_enabled)
> + return;
> +
> + it->state = level;
> + preempt_disable();
> + it->CPU = smp_processor_id();

You don't need to copy the smp_processor_id. Since all the buffers are
per_cpu, the cpu id is passed into your print_line via the iter
structure.

iter->cpu is the cpu that this entry was recorded on.


> + data = tr->data[smp_processor_id()];

You don't need the above 'data' structure, unless you want to use it
to prevent being reentrant. But you currently dont.

> +
> + event = ring_buffer_lock_reserve(tr->buffer, sizeof(*entry),
> + &irq_flags);
> + if (!event)
> + goto out;
> + entry = ring_buffer_event_data(event);
> + tracing_generic_entry_update(&entry->ent, 0);
> + entry->ent.type = TRACE_BOOT;

It doesn't look like you use any of the trace_entry structure. You can
just store your normal structure, since you bypass the output with you
print_line function.

struct cstate_trace *entry;

[...]

entry = ring_buffer_event_data(event);
*entry = *it;
ring_buffer_unlock_commit(tr->buffer, event, irq_flags);


-- Steve

> + entry->state_data = *it;
> + ring_buffer_unlock_commit(tr->buffer, event, irq_flags);
> +
> + trace_wake_up();
> +
> + out:
> + preempt_enable();
> +}
> --
> 1.5.5.1
>
>
> --
> Arjan van de Ven Intel Open Source Technology Centre
> For development, discussion and tips for power savings,
> visit http://www.lesswatts.org
>


\
 
 \ /
  Last update: 2008-10-04 02:35    [W:0.145 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site