lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 3 Oct 2008 20:32:51 -0400 (EDT) From: Steven Rostedt <rostedt@...dmis.org> To: Arjan van de Ven <arjan@...radead.org> cc: mingo@...e.hu, linux-kernel@...r.kernel.org Subject: Re: [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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists