linux-kernel - Re: [PATCH tip 2/2] tracing: Introduce trace_buffer_{lock_reserve,unlock

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]

Date:	Thu, 5 Feb 2009 23:54:16 -0200
From:	Arnaldo Carvalho de Melo <acme@...hat.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Steven Rostedt <rostedt@...dmis.org>, Ingo Molnar <mingo@...e.hu>,
	Jens Axboe <jens.axboe@...cle.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH tip 2/2] tracing: Introduce
	trace_buffer_{lock_reserve,unlock_commit}

Em Thu, Feb 05, 2009 at 11:58:37PM +0100, Frederic Weisbecker escreveu:
> > +void trace_buffer_unlock_commit(struct trace_array *tr,
> > +				struct ring_buffer_event *event,
> > +				unsigned long flags, int pc)
> > +{
> > +	ring_buffer_unlock_commit(tr->buffer, event);
> > +
> > +	ftrace_trace_stack(tr, flags, 6, pc);
> > +	ftrace_trace_userstack(tr, flags, pc);
> > +	trace_wake_up();
> > +}
> 
> 
> I have mitigate feelings about this part. The name of this function could
> have some sense if _most_ of the tracers were using the stack traces. But that's
> not the case.
> 
> We have now this couple:
> 
> _ trace_buffer_lock_reserve() -> handles the ring-buffer reservation, the context info, and the type
> _ trace_buffer_unlock_commit() -> unlock, commit, wake and... stacktraces?
> 
> In my opinion, the latter doesn't follow the logic meaning of the first.
> And the result is a mixup of (trace_buffer | ring_buffer)(lock/unlock/reserve/commit).
> 
> You are sometimes using trace_buffer_lock_reserve followed by ring_buffer_unlock_commit.
> That looks a bit weird: we are using a high level function followed by its conclusion
> on the form of the low lovel function.
> 
> I think the primary role of this new couple should be to simplify the low level ring buffer
> bits as it does. But the stack things should stay separated.

Well, the whole reason for this cset was to provide a way to check for
things like stacktrace while reducing the number of explicit calls the
poor driver, oops, ftrace plugin writers had to keep in mind.

So it may well be the case for a better name, but frankly I think that
this is something better left _hidden_, a magic that the plugin writers
doesn't have to deal with.

But... if they feel lucky and smart, they can just call
ring_buffer_unlock_commit(tr->buffer, event) and do any other things in
a open coded way, as was done in other cases where
trace_buffer_lock_reserve was paired with ring_buffer_unlock_commit.

- Arnaldo
--
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/