ftrace: fix wrong pos computing when read buffer has been fulfilled.

PATCH In review walimis 3 participants 8 messages Nov 15, 2008 – Nov 18, 2008

Subsystems (2)

Tracing ftrace

lore

Share & tools

lore

walimis

author

2008-11-15 07:19 UTC #0 lore raw

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

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(-)

kernel/trace/ftrace.c +15 ~~-11~~

index beb21a5..3a73534 100644
@@ -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;

Signature · 1 line

1.6.0.3

Ingo Molnar

2008-11-16 07:34 UTC #1 lore raw

Quoted 29 lines · 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.

Quoted 6 lines

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

Steven Rostedt

maintainer

2008-11-16 15:01 UTC #2 lore raw

Quoted 31 lines · Ingo Molnar

Quoted 29 lines · 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.

Quoted 8 lines

Quoted 6 lines
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

walimis

author

2008-11-16 16:06 UTC #3 lore raw

Quoted 19 lines · Steven Rostedt

Quoted 17 lines · Ingo Molnar
Quoted 14 lines · walimis
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(-)

Quoted 6 lines

Quoted 1 line
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

Quoted 2 lines

-- Steve

Steven Rostedt

maintainer Acked-by on v1 1/1

2008-11-18 14:36 UTC #4 lore raw

In reply to walimis

Quoted 25 lines · 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.

Quoted 5 lines

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.

Quoted 2 lines

another little fix is to show correct allocating pages count.

Ack.

Quoted 2 lines

Signed-off-by walimis <[email protected]>

Acked-by Steven Rostedt <[email protected]>

-- Steve

walimis

author

2008-11-18 15:17 UTC #5 lore raw

Quoted 30 lines · Steven Rostedt

Quoted 27 lines · 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:

Quoted 8 lines

Quoted 3 lines
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

Quoted 12 lines

Quoted 3 lines
another little fix is to show correct allocating pages count.
Ack.
Quoted 2 lines
Signed-off-by walimis <[email protected]>
Acked-by Steven Rostedt <[email protected]>
-- Steve

Steven Rostedt

maintainer

2008-11-18 15:32 UTC #6 lore raw

Quoted 16 lines · walimis

Quoted 11 lines · Steven Rostedt
Quoted 6 lines
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:

Quoted 8 lines

	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

Ingo Molnar

2008-11-18 15:53 UTC #7 lore raw

Quoted 27 lines · Steven Rostedt

Quoted 16 lines · walimis
Quoted 11 lines · Steven Rostedt
Quoted 6 lines
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:
Quoted 8 lines
	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.

Quoted 1 line

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