Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri 26-09-14 11:00:43, Steven Rostedt wrote:
> As people are asking for this patch series to be added, I'm going back
> through your comments. I never replied to this email (at least my email
> client says I did not).
It is great that you are on it again. I am looking forward to have the
proposed solution of the backtrace printing under NMI.
> On Fri, 27 Jun 2014 18:52:04 +0200
> Petr Mládek wrote:
>
> > On Fri 2014-06-27 11:39:09, Steven Rostedt wrote:
> > > On Fri, 27 Jun 2014 17:18:04 +0200
> > > Petr Mládek 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.
> >
> > It is true that most writing functions write as much as possible. On
> > the other hand, I do not think that refusing to write, if there is not
> > enough space, is specific to tracing. It might be useful also for others.
>
> Looking at what seq_file does, for example seq_printf(), it fills the
> buffer to the max size. If the printf is truncated, it sets the count
> to the size of the buffer. Then in traverse(), it detects that an
> overflow happened and stops reading, frees the buffer, increases the
> size of the buffer (by power of two), and then tries again with the
> bigger buffer.
>
> Really, it doesn't matter if seq_printf() didn't write anything or if
> it truncated, the result would be the same. Perhaps then I can keep
> seq_buf doing a all or nothing approach like trace_seq does. I think it
> was Andrew (akpm) that criticized this behavior.
I guess that you mean https://lkml.org/lkml/2014/6/20/26 and the part:
--- cut ---
> +
> + /* If we can't write it all, don't bother writing anything */
This is somewhat unusual behavior for a write()-style thing. Comment
should explain "why", not "what".
--- cut ---
>
> >
> >
> > > >
> > > > >
> > > > > > 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;
> > > > }
>
> Well, if it is overflowed, it can't write anymore ;-)
>
> But notice that the seq.len is still the same. Hmm, I need to zero that
> as well. As tools are allowed to print s->buffer with it. We don't want
> added data to it.
Your patch actually do
back s->seq.len = save_len;
which is reasonable. I probably simplified it by the dots (...) too much. :-)
> > > >
> > > > 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.
> >
> > The problem is that we do not know that it was cropped.
> >
> > The "overflow" flag is set when (s->len > (s->size - 1)). In most
> > cases it will be set when (s->len == s->size).
> >
> > For example, seq_buf_printf() calls vsnprintf(). It will never write
> > over the buffer. We do not know if the message was cropped or if we
> > were lucky and the message was exactly as long as the free space.
> >
>
> Again, I was doing this because of what was suggested before. I'll try
> to find the email. I may work to have seq_buf() be used for seq_file
> first, and then make trace_seq() use it. That might make even more
> sense.
OK, let's see what is going out of the integration with seq_file.
Also I am going to think about another solution. In fact, I think that
both names "overflow" and "full" are slightly misleading. As explained
above, "overflow" might be set event when there was no real overflow and
"full" is set even when there seems to be a space. I understand
how it is designed but I wonder if we might find a more clear
solution, maybe just a better name(s).
> > In each case, I do not want to block this patch. The generic "seq_buf"
> > API looks reasonable. The tracing code is your area and it is your
> > decision. You know much more about it than me and the extra complexity
> > might be needed.
>
> Yeah, we can always change it later. It's not an interface into
> userspace, thus it's
Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
As people are asking for this patch series to be added, I'm going back
through your comments. I never replied to this email (at least my email
client says I did not).
On Fri, 27 Jun 2014 18:52:04 +0200
Petr Mládek wrote:
> On Fri 2014-06-27 11:39:09, Steven Rostedt wrote:
> > On Fri, 27 Jun 2014 17:18:04 +0200
> > Petr Mládek 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.
>
> It is true that most writing functions write as much as possible. On
> the other hand, I do not think that refusing to write, if there is not
> enough space, is specific to tracing. It might be useful also for others.
Looking at what seq_file does, for example seq_printf(), it fills the
buffer to the max size. If the printf is truncated, it sets the count
to the size of the buffer. Then in traverse(), it detects that an
overflow happened and stops reading, frees the buffer, increases the
size of the buffer (by power of two), and then tries again with the
bigger buffer.
Really, it doesn't matter if seq_printf() didn't write anything or if
it truncated, the result would be the same. Perhaps then I can keep
seq_buf doing a all or nothing approach like trace_seq does. I think it
was Andrew (akpm) that criticized this behavior.
>
> In the worst case, we could add a flag into the "struct seq_buf" that
> might define the behavior. Well, I am not sure if we want to add this
> complexity when there are no users. As I said, the backtraces might
> live with trace_seq behavior.
>
Yeah, I don't want to add more complexity to this. No flags :-)
> >>
> > > > 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.
>
> Note that we should not allocate in NMI context, especially when there
> is a softlock. So, we might need to define the behavior anyway.
No, I wouldn't mean that seq_buf would automatically allocate, but a
wrapper to seq_buf could. Like what seq_file does. In fact, perhaps, we
can have seq_buf be part of seq_file and consolidate the behavior?
>
>
> > >
> > > >
> > > > > 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;
> > > }
Well, if it is overflowed, it can't write anymore ;-)
But notice that the seq.len is still the same. Hmm, I need to zero that
as well. As tools are allowed to print s->buffer with it. We don't want
added data to it.
> > >
> > > 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.
>
> The problem is that we do not know that it was cropped.
>
> The "overflow" flag is set when (s->len > (s->size - 1)). In most
> cases it will be set when (s->len ==
Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri 2014-06-27 11:39:09, Steven Rostedt wrote:
> On Fri, 27 Jun 2014 17:18:04 +0200
> Petr Mládek 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.
It is true that most writing functions write as much as possible. On
the other hand, I do not think that refusing to write, if there is not
enough space, is specific to tracing. It might be useful also for others.
In the worst case, we could add a flag into the "struct seq_buf" that
might define the behavior. Well, I am not sure if we want to add this
complexity when there are no users. As I said, the backtraces might
live with trace_seq behavior.
>>
> > > 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.
Note that we should not allocate in NMI context, especially when there
is a softlock. So, we might need to define the behavior anyway.
> >
> > >
> > > > 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.
The problem is that we do not know that it was cropped.
The "overflow" flag is set when (s->len > (s->size - 1)). In most
cases it will be set when (s->len == s->size).
For example, seq_buf_printf() calls vsnprintf(). It will never write
over the buffer. We do not know if the message was cropped or if we
were lucky and the message was exactly as long as the free space.
> 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.
I know that there is some difference but I am not sure if users are
really interested into such details. In both cases, they are unable
to write more data. From they point of view, the buffer is simply full.
Honestly, I think that the two flags and the two point of view cause
more confusion than win.
Note that I do not want to change the meaning for trace_buf. I want to
change the meaning for "seq_buf" that has no users yet.
> 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.
I am afraid that we do not have this information. See above for the
example with vsnprintf().
In each case, I do not want to block this patch. The generic "seq_buf"
API looks reasonable. The tracing code is your area and it is your
decision. You know much more about it than me and the extra complexity
might be neede
Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri, 27 Jun 2014 17:18:04 +0200
Petr Mládek 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 wrote:
>
> > On Thu 2014-06-26 17:49:03, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Red Hat)"
> > >
> > > 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
> > > ---
> > > 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 ev
Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri 2014-06-27 10:21:34, Steven Rostedt wrote:
> On Fri, 27 Jun 2014 15:45:38 +0200
> Petr Mládek wrote:
>
> > 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.
> >
> >
> > Best Regards,
> > Petr
> >
> > [...]
>
>
> BTW, you shouldn't sign off if you have more comments, I usually stop
> reading when I see someone's signature. Or at the very least, state
> that you have more comments below.
Shame on me. I was about to send the mail but then I decided to add
more comments into the code. :-)
> >
> > > +#define MAX_MEMHEX_BYTES 8U
> > > +#define HEX_CHARS(MAX_MEMHEX_BYTES*2 + 1)
> > > +
> > > +/**
> > > + * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex
> > > + * @s: seq_buf descriptor
> > > + * @mem: The raw memory to write its hex ASCII representation of
> > > + * @len: The length of the raw memory to copy (in bytes)
> > > + *
> > > + * This is similar to seq_buf_putmem() except instead of just copying the
> > > + * raw memory into the buffer it writes its ASCII representation of it
> > > + * in hex characters.
> > > + *
> > > + * Returns how much it wrote to the buffer.
> > > + */
> > > +int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
> > > +unsigned int len)
> > > +{
> > > + unsigned char hex[HEX_CHARS];
> > > + const unsigned char *data = mem;
> > > + unsigned int start_len;
> > > + int i, j;
> > > + int cnt = 0;
> > > +
> > > + while (len) {
> > > + start_len = min(len, HEX_CHARS - 1);
> >
> > I would do s/start_len/end_len/
> >
> > I know that it depends on the point of view. But iteration between "0"
> > and "end" is better understandable for me :-)
>
> Yeah, I didn't like the name of that variable. I guess end_len is
> better, but still not exactly what I want to call it. Unfortunately, I
> don't know what exactly I want to call it ;-)
Yup, the code is tricky :-) The names like break_len, wrap_len,
split_len, block_len come to my mind.
I would prefer wrap_len after all. But the choice it yours.
Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri, 27 Jun 2014 15:45:38 +0200
Petr Mládek wrote:
> 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.
>
>
> Best Regards,
> Petr
>
> [...]
BTW, you shouldn't sign off if you have more comments, I usually stop
reading when I see someone's signature. Or at the very least, state
that you have more comments below.
> > --- /dev/null
> > +++ b/kernel/trace/seq_buf.c
> > @@ -0,0 +1,348 @@
> > +/*
> > + * seq_buf.c
> > + *
> > + * Copyright (C) 2014 Red Hat Inc, Steven Rostedt
> > + *
> > + * The seq_buf is a handy tool that allows you to pass a descriptor around
> > + * to a buffer that other functions can write to. It is similar to the
> > + * seq_file functionality but has some differences.
> > + *
> > + * To use it, the seq_buf must be initialized with seq_buf_init().
> > + * This will set up the counters within the descriptor. You can call
> > + * seq_buf_init() more than once to reset the seq_buf to start
> > + * from scratch.
> > + *
> > + */
> > +#include
> > +#include
> > +#include
> > +
> > +/* How much buffer is left on the seq_buf? */
> > +#define SEQ_BUF_LEFT(s) (((s)->size - 1) - (s)->len)
> > +
> > +/* How much buffer is written? */
> > +#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
> > +
> > +static inline void seq_buf_check_len(struct seq_buf *s)
> > +{
> > + if (unlikely(s->len > (s->size - 1))) {
>
> I would create macro for this check. It is repeated many times. I mean
>
> #define SEQ_BUF_OVERFLOW(s) (s->len > (s->size - 1))
>
> if (unlikely(SEQ_BUF_OVERFLOW))
Yeah, I was hoping that the static inline would be enough, but then it
didn't fit into the functions at the end. I never got around to then
adding a macro.
Everything was open coded when I first created it.
>
> > + s->len = s->size - 1;
> > + s->overflow = 1;
> > + }
> > +}
> > +
>
> [...]
>
> > +#define MAX_MEMHEX_BYTES 8U
> > +#define HEX_CHARS (MAX_MEMHEX_BYTES*2 + 1)
> > +
> > +/**
> > + * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex
> > + * @s: seq_buf descriptor
> > + * @mem: The raw memory to write its hex ASCII representation of
> > + * @len: The length of the raw memory to copy (in bytes)
> > + *
> > + * This is similar to seq_buf_putmem() except instead of just copying the
> > + * raw memory into the buffer it writes its ASCII representation of it
> > + * in hex characters.
> > + *
> > + * Returns how much it wrote to the buffer.
> > + */
> > +int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
> > + unsigned int len)
> > +{
> > + unsigned char hex[HEX_CHARS];
> > + const unsigned char *data = mem;
> > + unsigned int start_len;
> > + int i, j;
> > + int cnt = 0;
> > +
> > + while (len) {
> > + start_len = min(len, HEX_CHARS - 1);
>
> I would do s/start_len/end_len/
>
> I know that it depends on the point of view. But iteration between "0"
> and "end" is better understandable for me :-)
Yeah, I didn't like the name of that variable. I guess end_len is
better, but still not exactly what I want to call it. Unfortunately, I
don't know what exactly I want to call it ;-)
>
> > +#ifdef __BIG_ENDIAN
> > + for (i = 0, j = 0; i < start_len; i++) {
> > +#else
> > + for (i = start_len-1, j = 0; i >= 0; i--) {
> > +#endif
> > + hex[j++] = hex_asc_hi(data[i]);
> > + hex[j++] = hex_asc_lo(data[i]);
> > + }
> > + if (WARN_ON_ONCE(j == 0 || j/2 > len))
> > + break;
> > +
> > + /* j increments twice per loop */
> > + len -= j / 2;
> > + hex[j++] = ' ';
> > +
> > + cnt += seq_buf_putmem(s, hex, j);
> > + }
> > + return cnt;
> > +}
> > +
> > +/**
> > + * seq_buf_path - copy a path into the sequence buffer
> > + * @s: seq_buf descriptor
> > + * @path: path to write into the sequence buffer.
> > + *
> > + * Write a path name into the sequence buffer.
> > + *
> > + * Returns the number of bytes written into the buffer.
> > + */
> > +int seq_buf_path(struct seq_buf *s, const struct path *path)
> > +{
> > + unsigned int len = SEQ_BUF_LEFT(s);
> > + unsigned char *p;
> > + unsigned int start = s->len;
> > +
> > + WARN_ON((int)len < 0);
> > + p = d_path(path, s->buffer + s->len, len);
> > + if (!IS_ERR(p)) {
> > + p = mangle_path(s->buffer + s->len, p, "\n");
> > + if (p) {
> > + s->len = p - s->buffer;
> > + WARN_ON(s->len > (s->size - 1));
>
> strange indentation
>
> > + return s->len - start;
> > + }
> > + } else {
> > + s->buffer[s->len++] = '?';
> > + WARN_ON(s->len > (s->size - 1));
>
> same here
Heh, I added those for debugging, and then decided to keep it. Never
went back to clean it up :-/
-- Steve
>
> > + return s->len - start;
> > +
Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Fri, 27 Jun 2014 15:45:38 +0200 Petr Mládek wrote: > On Thu 2014-06-26 17:49:03, Steven Rostedt wrote: > > From: "Steven Rostedt (Red Hat)" > > > > 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 > > --- > > 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. > > > 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. 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. > > > 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. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
On Thu 2014-06-26 17:49:03, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)"
>
> 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
> ---
> 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.
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.
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.
IMHO, the most important thing is that both functions return 0 on
failure.
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.
Best Regards,
Petr
[...]
> --- /dev/null
> +++ b/kernel/trace/seq_buf.c
> @@ -0,0 +1,348 @@
> +/*
> + * seq_buf.c
> + *
> + * Copyright (C) 2014 Red Hat Inc, Steven Rostedt
> + *
> + * The seq_buf is a handy tool that allows you to pass a descriptor around
> + * to a buffer that other functions can write to. It is similar to the
> + * seq_file functionality but has some differences.
> + *
> + * To use it, the seq_buf must be initialized with seq_buf_init().
> + * This will set up the counters within the descriptor. You can call
> + * seq_buf_init() more than once to reset the seq_buf to start
> + * from scratch.
> + *
> + */
> +#include
> +#include
> +#include
> +
> +/* How much buffer is left on the seq_buf? */
> +#define SEQ_BUF_LEFT(s) (((s)->size - 1) - (s)->len)
> +
> +/* How much buffer is written? */
> +#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
> +
> +static inline void seq_buf_check_len(struct seq_buf *s)
> +{
> + if (unlikely(s->len > (s->size - 1))) {
I would create macro for this check. It is repeated many times. I mean
#define SEQ_BUF_OVERFLOW(s) (s->len > (s->size - 1))
if (unlikely(SEQ_BUF_OVERFLOW))
> + s->len = s->size - 1;
> + s->overflow = 1;
> + }
> +}
> +
[...]
> +#define MAX_MEMHEX_BYTES 8U
> +#define HEX_CHARS(MAX_MEMHEX_BYTES*2 + 1)
> +
> +/**
> + * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex
> + * @s: seq_buf descriptor
> + * @mem: The raw memory to write its hex ASCII representation of
> + * @len: The length of the raw memory to copy (in bytes)
> + *
> + * This is similar to seq_buf_putmem() except instead of just copying the
> + * raw memory into the buffer it writes its ASCII representation of it
> + * in hex characters.
> + *
> + * Returns how much it wrote to the buffer.
> + */
> +int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
> +unsigned int len)
> +{
> + unsigned char hex[HEX_CHARS];
> + const unsigned char *data = mem;
> + unsigned int start_len;
> + int i, j;
> + int cnt = 0;
> +
> + while (len) {
> + start_len = min(len, HEX_CHARS - 1);
I would do s/start_len/end_len/
I know that it depends on the point of view. But iteration between "0"
and "end" i
[RFC][PATCH 2/5 v2] tracing: Create seq_buf layer in trace_seq
From: "Steven Rostedt (Red Hat)"
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
---
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
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
new file mode 100644
index ..23b3fa2bd0cc
--- /dev/null
+++ b/include/linux/seq_buf.h
@@ -0,0 +1,58 @@
+#ifndef _LINUX_SEQ_BUF_H
+#define _LINUX_SEQ_BUF_H
+
+#include
+
+#include
+
+/*
+ * Trace sequences are used to allow a function to call several other functions
+ * to create a string of data to use.
+ */
+
+/**
+ * seq_buf - seq buffer structure
+ * @buffer:pointer to the buffer
+ * @size: size of the buffer
+ * @len: the amount of data inside the buffer
+ * @readpos: The next position to read in the buffer.
+ * @overflow: Set if more bytes should have been written to buffer
+ */
+struct seq_buf {
+ unsigned char *buffer;
+ unsigned intsize;
+ unsigned intlen;
+ unsigned intreadpos;
+ unsigned intoverflow;
+};
+
+static inline void
+seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
+{
+ s->buffer = buf;
+ s->size = size;
+ s->len = 0;
+ s->readpos = 0;
+ s->overflow = 0;
+}
+
+extern __printf(2, 3)
+int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
+extern __printf(2, 0)
+int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args);
+extern int
+seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
+extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s);
+extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf,
+ int cnt);
+extern int seq_buf_puts(struct seq_buf *s, const char *str);
+extern int seq_buf_putc(struct seq_buf *s, unsigned char c);
+extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int
len);
+extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
+ unsigned int len);
+extern int seq_buf_path(struct seq_buf *s, const struct path *path);
+
+extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+ int nmaskbits);
+
+#endif /* _LINUX_SEQ_BUF_H */
diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
index ea6c9dea79e3..27c98fd76503 100644
--- a/include/linux/trace_seq.h
+++ b/include/linux/trace_seq.h
@@ -1,7 +1,7 @@
#ifndef _LINUX_TRACE_SEQ_H
#define _LINUX_TRACE_SEQ_H
-#include
+#include
#include
@@ -12,16 +12,14 @@
struct trace_seq {
unsigned char buffer[PAGE_SIZE];
- unsigned intlen;
- unsigned intreadpos;
+ struct seq_buf seq;
int full;
};
static inline void
trace_seq_init(struct trace_seq *s)
{
- s->len = 0;
- s->readpos = 0;
+ seq_buf_init(&s->seq, s->buffer, PAGE_SIZE);
s->full = 0;
}
@@ -37,7 +35,7 @@ trace_seq_init(struct trace_seq *s)
static inline unsigned char *
trace_seq_buffer_ptr(struct trace_seq *s)
{
- return s->buffer + s->len;
+ return s->buffer + s->seq.len;
}
/*
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index 67d6369ddf83..edc98c72a634 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_RING_BUFFER_BENCHMARK) += ring_buffer_benchmark.o
obj-$(CONFIG_TRACING) += trace.o
obj-$(CONFIG_TRACING) += trace_output.o
obj-$(CONFIG_TRACING) += trace_seq.o
+obj-$(CONFIG_TRACING) += seq_buf.o
obj-$(CONFIG_TRACING) += trace_stat.o
obj-$(CONFIG_TRACING) += trace_printk.o
obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
new file mode 100644
index ..b7e33c18540a
--- /dev/null
+++ b/kernel/trace/seq_buf.c
@@ -0,0 +1,348 @@
+/*
+ * seq_buf.c
+ *
+ * Copyright (C) 2014 Red Hat Inc, Steven Rostedt
+ *
+ * The seq_buf is a handy tool that allows you to pass a descriptor around
+ * to a buffer that other functions can write to. It is similar to the
+ * seq_file functionality but has some differences.
+ *
+ * To use it, the seq_buf must be initialized with seq_buf_init().
+ * This will set up the counters within the descriptor. You can call
+
