lkml.org 
[lkml]   [2014]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri 2014-06-27 10:19:07, Steven Rostedt wrote:
> On Fri, 27 Jun 2014 15:45:38 +0200
> Petr Mládek <pmladek@suse.cz> wrote:
>
> > On Thu 2014-06-26 17:49:03, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > >
> > > Create a seq_buf layer that trace_seq sits on. The seq_buf will not
> > > be limited to page size. This will allow other usages of seq_buf
> > > instead of a hard set PAGE_SIZE one that trace_seq has.
> > >
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > > ---
> > > include/linux/seq_buf.h | 58 ++++++
> > > include/linux/trace_seq.h | 10 +-
> > > kernel/trace/Makefile | 1 +
> > > kernel/trace/seq_buf.c | 348 +++++++++++++++++++++++++++++++++++
> > > kernel/trace/trace.c | 39 ++--
> > > kernel/trace/trace_events.c | 6 +-
> > > kernel/trace/trace_functions_graph.c | 6 +-
> > > kernel/trace/trace_seq.c | 184 +++++++++---------
> > > 8 files changed, 527 insertions(+), 125 deletions(-)
> > > create mode 100644 include/linux/seq_buf.h
> > > create mode 100644 kernel/trace/seq_buf.c
> >
> > There is a lot of similar code in the two layers. Do you have any
> > plans to transform the trace stuff to seq_buf and get rid of the
> > trace_seq layer in long term?
> >
> > If I get it correctly, the two layers are needed because:
> >
> > 1. seq_buf works with any buffer but
> > trace_seq with static buffer
> >
> > 2. seq_buf writes even part of the message until the buffer is full but
> > trace_buf writes only full messages or nothing
> >
> > 3. seq_buf returns the number of written characters but
> > trace_seq returns 1 on success and 0 on failure
> >
> > 4. seq_buf sets "overflow" flag when an operation failed but
> > trace_seq sets "full" when this happens
> >
> >
> > I wounder if we could get a compromise and use only one layer.
> >
> > ad 1st:
> >
> > I have checked many locations and it seems that trace_seq_init() is
> > called before the other functions as used. There is the WARN_ON()
> > in the generic seq_buf() functions, so they would not crash. If
> > there is some init missing, we could fix it later.
> >
> > But I haven't really tested it yet.
>
> Actually, there's a few hidden places that initialize a struct with
> kzalloc that contains a trace_seq. I started fixing them but there were
> more and more to find that I decided to give up and just add the check
> to see if it needed to be initialized at each call.
>
> Not that pretty, but not that critical to be worth crashing something
> we missed. Maybe in the future this can change, but not now.

I see.

> >
> > ad 2nd and 3rd:
> >
> > These are connected.
> >
> > On solution is to disable writing part of the text even in the
> > generic seq_buf functions. I think that it is perfectly fine.
> > If we do not print the entire line, we are in troubles anyway.
> > Note that we could not allocate another buffer in NMI, so
> > we will lose part of the message anyway.
>
> This patch uses seq_buf for the NMI code so it will fill to the end of
> the buffer and just truncate what can't fit.

I think that NMI code could live with the trace_seq behavior. The
lines are short. If we miss few characters it is not that big difference.

> trace_pipe depends on the trace_seq behavior.
>
> >
> > Another solution would be to live with incomplete lines in tracing.
> > I wonder if any of the functions tries to write the line again when the
> > write failed.
>
> This may break trace_pipe. Although there looks to be redundant
> behavior in that the pipe code also resets the seq.len on partial line,
> so maybe it's not an issue.
>
> >
> > IMHO, the most important thing is that both functions return 0 on
> > failure.
>
> Note, I'm not sure how tightly these two need to be. I'm actually
> trying to get trace_seq to be specific to tracing and nothing more.
> Have seq_buf be used for all other instances.

If the two layers make your life easier then they might make sense. I
just saw many similarities and wanted to help. IMHO, if anyone breaks
seq_buf, it will break trace_seq anyway. So, they are not really separated.

>
> > ad 4th:
> >
> > Both "full" and "overflow" flags seems to have the same meaning.
> > For example, trace_seq_printf() sets "full" on failure even
> > when s->seq.len != s->size.
>
> The difference is that the overflow flag is just used for info letting
> the user know that it did not fit. The full flag in trace_seq lets you
> know that you can not add anything else, even though the new stuff may
> fit.

I see. They have another meaning but they are set at the same time:

if (s->seq.overflow) {
...
s->full = 1;
return 0;
}

In fact, both names are slightly misleading. seq.overflow is set
when the buffer is full even when all characters were written.
s->full is set even when there is still some space :-)

I suggest to rename "overflow" to "full" and have it only in the
struct "seq_buf". Then it has the right meaning in "seq_buf" and is still
backward compatible with "trace_seq".


Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2014-06-27 17:41    [W:0.149 / U:0.396 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site