hi Steven,
Could you help me to review this patch?
Thanks.
Impact: make output of available_filter_functions complete
phenomenon:
The first value of dyn_ftrace_total_info is not equal with
`cat available_filter_functions | wc -l`, but they should be equal.
root cause:
When printing functions with seq_printf in t_show, if the read buffer
is just overflowed by current function record, then this function
won't be printed to user space through read buffer, it will
just be dropped. So we can't see this function printing.
So, every time the last function to fill the read buffer, if overflowed,
will be dropped.
This also applies to set_ftrace_filter if set_ftrace_filter has
more bytes than read buffer.
fix:
Through checking return value of seq_printf, if less than 0, we know
this function doesn't be printed. Then we decrease position to force
this function to be printed next time, in next read buffer.
another little fix is to show correct allocating pages count.
Signed-off-by: walimis <[email protected]>
---
kernel/trace/ftrace.c | 26 +++++++++++++++-----------
1 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index beb21a5..3a73534 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -700,7 +700,7 @@ static int __init ftrace_dyn_table_alloc(unsigned long num_to_init)
cnt = num_to_init / ENTRIES_PER_PAGE;
pr_info("ftrace: allocating %ld entries in %d pages\n",
- num_to_init, cnt);
+ num_to_init, cnt + 1);
for (i = 0; i < cnt; i++) {
pg->next = (void *)get_zeroed_page(GFP_KERNEL);
@@ -783,13 +783,11 @@ static void *t_start(struct seq_file *m, loff_t *pos)
void *p = NULL;
loff_t l = -1;
- if (*pos != iter->pos) {
- for (p = t_next(m, p, &l); p && l < *pos; p = t_next(m, p, &l))
- ;
- } else {
- l = *pos;
- p = t_next(m, p, &l);
- }
+ if (*pos > iter->pos)
+ *pos = iter->pos;
+
+ l = *pos;
+ p = t_next(m, p, &l);
return p;
}
@@ -800,15 +798,21 @@ static void t_stop(struct seq_file *m, void *p)
static int t_show(struct seq_file *m, void *v)
{
+ struct ftrace_iterator *iter = m->private;
struct dyn_ftrace *rec = v;
char str[KSYM_SYMBOL_LEN];
+ int ret = 0;
if (!rec)
return 0;
kallsyms_lookup(rec->ip, NULL, NULL, NULL, str);
- seq_printf(m, "%s\n", str);
+ ret = seq_printf(m, "%s\n", str);
+ if (ret < 0) {
+ iter->pos--;
+ iter->idx--;
+ }
return 0;
}
@@ -834,7 +838,7 @@ ftrace_avail_open(struct inode *inode, struct file *file)
return -ENOMEM;
iter->pg = ftrace_pages_start;
- iter->pos = -1;
+ iter->pos = 0;
ret = seq_open(file, &show_ftrace_seq_ops);
if (!ret) {
@@ -921,7 +925,7 @@ ftrace_regex_open(struct inode *inode, struct file *file, int enable)
if (file->f_mode & FMODE_READ) {
iter->pg = ftrace_pages_start;
- iter->pos = -1;
+ iter->pos = 0;
iter->flags = enable ? FTRACE_ITER_FILTER :
FTRACE_ITER_NOTRACE;
--
1.6.0.3
* walimis <[email protected]> wrote:
> hi Steven,
>
> Could you help me to review this patch?
>
> Thanks.
>
> Impact: make output of available_filter_functions complete
>
> phenomenon:
>
> The first value of dyn_ftrace_total_info is not equal with
> `cat available_filter_functions | wc -l`, but they should be equal.
>
> root cause:
>
> When printing functions with seq_printf in t_show, if the read
> buffer is just overflowed by current function record, then this
> function won't be printed to user space through read buffer, it will
> just be dropped. So we can't see this function printing. So, every
> time the last function to fill the read buffer, if overflowed, will
> be dropped. This also applies to set_ftrace_filter if
> set_ftrace_filter has more bytes than read buffer.
>
> fix:
>
> Through checking return value of seq_printf, if less than 0, we know
> this function doesn't be printed. Then we decrease position to force
> this function to be printed next time, in next read buffer.
>
> another little fix is to show correct allocating pages count.
>
> Signed-off-by: walimis <[email protected]>
> ---
> kernel/trace/ftrace.c | 26 +++++++++++++++-----------
> 1 files changed, 15 insertions(+), 11 deletions(-)
looks sane - applied to tip/tracing/urgent. Steve, any objections?
Ingo
On Sun, 16 Nov 2008, Ingo Molnar wrote:
> * walimis <[email protected]> wrote:
>
> > hi Steven,
> >
> > Could you help me to review this patch?
> >
> > Thanks.
> >
> > Impact: make output of available_filter_functions complete
> >
> > phenomenon:
> >
> > The first value of dyn_ftrace_total_info is not equal with
> > `cat available_filter_functions | wc -l`, but they should be equal.
> >
> > root cause:
> >
> > When printing functions with seq_printf in t_show, if the read
> > buffer is just overflowed by current function record, then this
> > function won't be printed to user space through read buffer, it will
> > just be dropped. So we can't see this function printing. So, every
> > time the last function to fill the read buffer, if overflowed, will
> > be dropped. This also applies to set_ftrace_filter if
> > set_ftrace_filter has more bytes than read buffer.
> >
> > fix:
> >
> > Through checking return value of seq_printf, if less than 0, we know
> > this function doesn't be printed. Then we decrease position to force
> > this function to be printed next time, in next read buffer.
> >
> > another little fix is to show correct allocating pages count.
> >
> > Signed-off-by: walimis <[email protected]>
> > ---
> > kernel/trace/ftrace.c | 26 +++++++++++++++-----------
> > 1 files changed, 15 insertions(+), 11 deletions(-)
>
> looks sane - applied to tip/tracing/urgent. Steve, any objections?
I'm actually still reviewing it. I was busy with the PPC work, so I did
not finish my review. Weekends are family time, so my activity now is
minimal. Pull it in, and if I find any problems with it, I'll just submit
a patch on top, or have walimis fix it ;-)
-- Steve
On Sun, Nov 16, 2008 at 10:01:46AM -0500, Steven Rostedt wrote:
>
>On Sun, 16 Nov 2008, Ingo Molnar wrote:
>> * walimis <[email protected]> wrote:
>>
>> > hi Steven,
>> >
>> > Could you help me to review this patch?
>> >
>> > Thanks.
>> >
>> > Impact: make output of available_filter_functions complete
>> >
>> > phenomenon:
>> >
>> > Signed-off-by: walimis <[email protected]>
>> > ---
>> > kernel/trace/ftrace.c | 26 +++++++++++++++-----------
>> > 1 files changed, 15 insertions(+), 11 deletions(-)
>>
>> looks sane - applied to tip/tracing/urgent. Steve, any objections?
>
>I'm actually still reviewing it. I was busy with the PPC work, so I did
>not finish my review. Weekends are family time, so my activity now is
>minimal. Pull it in, and if I find any problems with it, I'll just submit
>a patch on top, or have walimis fix it ;-)
OK, thanks for your review.
If I find any problem, I will submit to you ASAP.
walimis
>
>-- Steve
On Sat, 15 Nov 2008, walimis wrote:
>
> hi Steven,
>
> Could you help me to review this patch?
>
> Thanks.
>
> Impact: make output of available_filter_functions complete
>
> phenomenon:
>
> The first value of dyn_ftrace_total_info is not equal with
> `cat available_filter_functions | wc -l`, but they should be equal.
>
> root cause:
>
> When printing functions with seq_printf in t_show, if the read buffer
> is just overflowed by current function record, then this function
> won't be printed to user space through read buffer, it will
> just be dropped. So we can't see this function printing.
> So, every time the last function to fill the read buffer, if overflowed,
> will be dropped.
> This also applies to set_ftrace_filter if set_ftrace_filter has
> more bytes than read buffer.
>
> fix:
>
> Through checking return value of seq_printf, if less than 0, we know
> this function doesn't be printed. Then we decrease position to force
> this function to be printed next time, in next read buffer.
A lot of the code for seq files I cut and paste from other places that I
have written it. The seq files always confuse me, so I like to use stuff
that worked before. Some of the things that walimis fixed were not needed
for this file, and was overkill.
>
> another little fix is to show correct allocating pages count.
Ack.
>
> Signed-off-by: walimis <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
-- Steve
On Tue, Nov 18, 2008 at 09:36:07AM -0500, Steven Rostedt wrote:
>
>On Sat, 15 Nov 2008, walimis wrote:
>
>>
>> hi Steven,
>>
>> Could you help me to review this patch?
>>
>> Thanks.
>>
>> Impact: make output of available_filter_functions complete
>>
>> phenomenon:
>>
>> The first value of dyn_ftrace_total_info is not equal with
>> `cat available_filter_functions | wc -l`, but they should be equal.
>>
>> root cause:
>>
>> When printing functions with seq_printf in t_show, if the read buffer
>> is just overflowed by current function record, then this function
>> won't be printed to user space through read buffer, it will
>> just be dropped. So we can't see this function printing.
>> So, every time the last function to fill the read buffer, if overflowed,
>> will be dropped.
>> This also applies to set_ftrace_filter if set_ftrace_filter has
>> more bytes than read buffer.
>>
>> fix:
>>
>> Through checking return value of seq_printf, if less than 0, we know
>> this function doesn't be printed. Then we decrease position to force
>> this function to be printed next time, in next read buffer.
>
>A lot of the code for seq files I cut and paste from other places that I
>have written it. The seq files always confuse me, so I like to use stuff
>that worked before. Some of the things that walimis fixed were not needed
>for this file, and was overkill.
It also confuse me a lot. I also checked all places using seq files in
trace/ftrace code, to see whether there are similar problem.
- trace.c: in s_start() it force pos to decrease one such as:
l = *pos - 1;
so it works well.
- trace_stack.c: t_start, in almost all cases, can be only called
once, so we don't see the error. But I wonder whether it still works
well if output message exceeds the size of seq file buffer and call
t_start twice. If needed, I can test it.
- trace_branch.c : the same with above.
walimis
>
>>
>> another little fix is to show correct allocating pages count.
>
>Ack.
>
>>
>> Signed-off-by: walimis <[email protected]>
>
>Acked-by: Steven Rostedt <[email protected]>
>
>-- Steve
On Tue, 18 Nov 2008, walimis wrote:
> On Tue, Nov 18, 2008 at 09:36:07AM -0500, Steven Rostedt wrote:
> >>
> >> fix:
> >>
> >> Through checking return value of seq_printf, if less than 0, we know
> >> this function doesn't be printed. Then we decrease position to force
> >> this function to be printed next time, in next read buffer.
> >
> >A lot of the code for seq files I cut and paste from other places that I
> >have written it. The seq files always confuse me, so I like to use stuff
> >that worked before. Some of the things that walimis fixed were not needed
> >for this file, and was overkill.
> It also confuse me a lot. I also checked all places using seq files in
> trace/ftrace code, to see whether there are similar problem.
> - trace.c: in s_start() it force pos to decrease one such as:
> l = *pos - 1;
> so it works well.
> - trace_stack.c: t_start, in almost all cases, can be only called
> once, so we don't see the error. But I wonder whether it still works
> well if output message exceeds the size of seq file buffer and call
> t_start twice. If needed, I can test it.
> - trace_branch.c : the same with above.
>
If you can find a bug, then by all means, fix it ;-)
Thanks,
-- Steve
* Steven Rostedt <[email protected]> wrote:
>
> On Tue, 18 Nov 2008, walimis wrote:
> > On Tue, Nov 18, 2008 at 09:36:07AM -0500, Steven Rostedt wrote:
> > >>
> > >> fix:
> > >>
> > >> Through checking return value of seq_printf, if less than 0, we know
> > >> this function doesn't be printed. Then we decrease position to force
> > >> this function to be printed next time, in next read buffer.
> > >
> > >A lot of the code for seq files I cut and paste from other places that I
> > >have written it. The seq files always confuse me, so I like to use stuff
> > >that worked before. Some of the things that walimis fixed were not needed
> > >for this file, and was overkill.
>
> > It also confuse me a lot. I also checked all places using seq files in
> > trace/ftrace code, to see whether there are similar problem.
> > - trace.c: in s_start() it force pos to decrease one such as:
> > l = *pos - 1;
> > so it works well.
> > - trace_stack.c: t_start, in almost all cases, can be only called
> > once, so we don't see the error. But I wonder whether it still works
> > well if output message exceeds the size of seq file buffer and call
> > t_start twice. If needed, I can test it.
> > - trace_branch.c : the same with above.
> >
>
> If you can find a bug, then by all means, fix it ;-)
also, if you can think of a way to structure the code in a cleaner way
to make such mistakes less likely to occur, feel free to change that
too.
Ingo