linux-kernel - Re: [PATCH v3 -tip 2/4] tracing: replace static old

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:	Fri, 21 Dec 2012 19:55:56 +0900
From:	Hiraku Toyooka <hiraku.toyooka.gu@...achi.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	masami.hiramatsu.pt@...achi.com,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	yrl.pp-manager.tt@...achi.com
Subject: Re: [PATCH v3 -tip 2/4] tracing: replace static old_tracer with strcmp

Hi,

(12/21/2012 12:04 PM), Steven Rostedt wrote:
> On Wed, 2012-12-19 at 16:02 +0900, Hiraku Toyooka wrote:
>> Currently, read functions for trace buffer use static "old_tracer"
>> for detecting changes of current tracer. This is because we can
>> assume that these functions are used from only one file ("trace").
>>
>> But we are adding snapshot feature for ftrace, then those functions
>> are called from two files. So we remove all static "old_tracer", and
>> replace those with string comparison between current and previous
>> tracers.
>
> I reworded what you wrote:
>

Thank you very much.

> ---
> Currently the trace buffer read functions use a static variable
> "old_tracer" for detecting if the current tracer changes. This was
> suitable for a single trace file ("trace"), but to add a snapshot
> feature that will use the same function for its file, a check against
> a static variable is not sufficient.
>
> To use the output functions for two different files, instead of storing
> the current tracer in a static variable, as the trace iterator
> descriptor
> contains a pointer to the original current tracer's name, that pointer
> can now be used to check if the current tracer has changed between
> different reads of the trace file.
> ---
>
> And I also realized a more efficient approach.
>
>>
>> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@...achi.com>
>> Cc: Steven Rostedt <rostedt@...dmis.org>
>> Cc: Frederic Weisbecker <fweisbec@...il.com>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: linux-kernel@...r.kernel.org
>> ---
>>   kernel/trace/trace.c |   18 ++++++------------
>>   1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index a8ce008..8d05a44 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -1948,7 +1948,6 @@ void tracing_iter_reset(struct trace_iterator *iter, int cpu)
>>   static void *s_start(struct seq_file *m, loff_t *pos)
>>   {
>>   	struct trace_iterator *iter = m->private;
>> -	static struct tracer *old_tracer;
>>   	int cpu_file = iter->cpu_file;
>>   	void *p = NULL;
>>   	loff_t l = 0;
>> @@ -1956,10 +1955,9 @@ static void *s_start(struct seq_file *m, loff_t *pos)
>>
>>   	/* copy the tracer to avoid using a global lock all around */
>>   	mutex_lock(&trace_types_lock);
>> -	if (unlikely(old_tracer != current_trace && current_trace)) {
>> -		old_tracer = current_trace;
>> +	if (unlikely(current_trace &&
>> +		     strcmp(iter->trace->name, current_trace->name) != 0))
>>   		*iter->trace = *current_trace;
>
> As iter->trace is a copy of current_trace, it points to everything that
> the current_trace pointed to. As the current_trace->name is a pointer to
> a const char string that never changes, we don't need to do the strcmp()
> you can simply do a direct comparison:
>
> 	if (unlikely(current_trace && iter->trace->name != current_trace->name)) {
>

Yes. I'll update my patch to use this efficient way.

> I would add a comment about that too:
> 	/*
> 	 * iter->trace is a copy of current_trace, the pointer to the
> 	 * name may be used instead of a strcmp(), as iter->trace->name
> 	 * will point to the same string as current_trace->name.
> 	 */

OK. I'll include this comment in my patch.


Thanks,
Hiraku Toyooka

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