linux-kernel - Re: [PATCH] ftrace: Add a C-state tracer to help power optimization

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.1.10.0810032003140.17863@gandalf.stny.rr.com>
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/