Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq

[Steven Rostedt]

On Fri, 27 Jun 2014 17:18:04 +0200
Petr MlÃdek <pmladek@xxxxxxx> wrote:


> > 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.

True, but I'm trying to keep trace_seq more tracing specific.

> 
> > 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.

There's actually things I want to do with the seq_buf behavior that I
can't do with the trace_seq behavior. That's more about having a
dynamic way to create seq_buf buffers and resize them if need be.
Although, perhaps an all or nothing approach will help in that.


> 
> > 
> > > 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 actually disagree. overflow means that you wrote more than what was
there. In this case, it was cropped.

Full suggests that you can't add anymore because it wont allow you.

The difference is that you are looking at a implementation point of
view. I'm looking at a usage point of view. With trace_seq, there is no
space left, you can't add any more. seq_buf, you can add more but it
wont be saved because it was overflowed.

What I should do is change overflow from being a flag to the number of
characters that it overflowed by. That would be useful information, and
would make more sense. It lets the user know what wasn't written.

-- Steve



> 
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/