Messages in this thread | | | Subject | Re: [PATCH 0/2][RFC] [RFC] tracing: separate out buffer from trace_seq | From | Steven Rostedt <> | Date | Wed, 09 Dec 2009 11:26:59 -0500 |
| |
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
| |