lkml.org 
[lkml]   [2009]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 0/2][RFC] [RFC] tracing: separate out buffer from trace_seq
From
Date
On Wed, 2009-12-09 at 17:44 +0800, Lai Jiangshan wrote:
> Steven Rostedt wrote:

> > Steven Rostedt (2):
> > tracing: Change trace_seq to use separate buffer
> > tracing: Write directly into splice page for trace_pipe
>
> It's great that you wrote it and trace_pipe benefit from it.
> And it show separate buffer is required.
>
> But I didn't expect so much/big changes. I'm wondering that

Yeah, I first did a patch (and I still have it) that keeps the trace_seq
structure as is. But then I decided to bite the bullet and just convert
it to a print helper.

> we can use seq file directly for some files and
> kill all kmalloc(sizeof(struct trace_seq)).
>
> kernel/trace/trace.c:3720: s = kmalloc(sizeof(*s), GFP_KERNEL);
> We can use a short buffer + simple_read_from_buffer()
> Or seq file.

Heh, yeah that usage of trace_seq was not necessary. It looks like a
handler for sprintf only.


>
> kernel/trace/trace_events.c:539: s = kmalloc(sizeof(*s), GFP_KERNEL);
> We can use seq file after of my patchset(today) is applied.

Yeah, sure.

>
> kernel/trace/trace_events.c:580: s = kmalloc(sizeof(*s), GFP_KERNEL);
> we can use a shor buffer. "char buf[20]" + simple_read_from_buffer()
>
> kernel/trace/trace_events.c:604: s = kmalloc(sizeof(*s), GFP_KERNEL);
> We can use a short buffer + simple_read_from_buffer()
> Or seq file.
>
> kernel/trace/trace_events.c:660: s = kmalloc(sizeof(*s), GFP_KERNEL);
> We can use a short buffer + simple_read_from_buffer()
> Or seq file.

Yep to the above.

>
> kernel/trace/trace_events.c:715: s = kmalloc(sizeof(*s), GFP_KERNEL);
> We can use seq file.

Hmm, this I'm not so sure about (my line numbers are off, this is
"show_header" right?).

The reason that I wrote trace_seq is to let functions write to whatever
buffer they want to. Not just a seq_file. This function calls other
functions that write to this buffer, and in the future, this buffer may
not be from a normal read.


>
> kernel/trace/trace_ksym.c:230: s = kmalloc(sizeof(*s), GFP_KERNEL);
> We should use seq! It's very bad that we use trace_seq.

I agree that seq_file should be used here, but I would not say it was
"very bad" to use trace_seq.

>
> For function_stat_show(), it uses static trace_seq. but it can also
> use a short buffer + simple_read_from_buffer().

No it can't. It uses it to call trace_print_graph_duration which is used
for trace and trace_pipe (and others). This requires the trace_seq
structure.

>
> I will do this.(or you if you would like to)

The ones that I said above are fine, you can do. Also split the patches
up per section. That is, the trace_seq -> small buffer +
simple_read_from_buffer should be a separate patch than the conversion
to seq_file.

Note, one reason others have used trace_seq over seq_file is that the
seq_file is a confusing interface. I constantly get it wrong. Helper
functions should be simplistic and intuitive, not something to spend
time debugging when it is suppose to be helping.

I agree with most of the above conversions, but I don't want to convert
all of them just because they can be. seq_file can limit their usage.

>
> So could you write a simpler version of separate-buffer-trace_seq
> with supposition that all kmalloc(sizeof(struct trace_seq)) are removed.

Make the above changes (where I agreed), then I'll rewrite this patch on
top of yours.

OK?

Thanks!

-- Steve




\
 
 \ /
  Last update: 2009-12-09 17:29    [W:0.075 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site