Re: [PATCH 0/2][RFC] [RFC] tracing: separate out buffer from trace_seq

[Steven Rostedt <rostedt@goodmis.org>]

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