[PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[walimis]

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 <walimisdev@gmail.com>
---
 kernel/trace/ftrace.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index beb21a5..b47718b 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

[PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[walimis]

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 <walimisdev@gmail.com>
---
 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

Re: [PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[Ingo Molnar]

* walimis <walimisdev@gmail.com> 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 <walimisdev@gmail.com>
> ---
>  kernel/trace/ftrace.c |   26 +++++++++++++++-----------
>  1 files changed, 15 insertions(+), 11 deletions(-)

looks sane - applied to tip/tracing/urgent. Steve, any objections?

	Ingo

Re: [PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[Steven Rostedt]

On Sun, 16 Nov 2008, Ingo Molnar wrote:
> * walimis <walimisdev@gmail.com> 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 <walimisdev@gmail.com>
> > ---
> >  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

Re: [PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[walimis]

On Sun, Nov 16, 2008 at 10:01:46AM -0500, Steven Rostedt wrote:
>
>On Sun, 16 Nov 2008, Ingo Molnar wrote:
>> * walimis <walimisdev@gmail.com> 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 <walimisdev@gmail.com>
>> > ---
>> >  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

Re: [PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[Steven Rostedt]

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 <walimisdev@gmail.com>

Acked-by: Steven Rostedt <srostedt@redhat.com>

-- Steve

Re: [PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[walimis]

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 <walimisdev@gmail.com>
>
>Acked-by: Steven Rostedt <srostedt@redhat.com>
>
>-- Steve

Re: [PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[Steven Rostedt]

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

Re: [PATCH 1/1] ftrace: fix wrong pos computing when read buffer has been fulfilled.

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> 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