Messages in this thread | | | Date | Fri, 27 Jun 2014 17:18:04 +0200 | From | Petr Mládek <> | Subject | Re: [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/
| |