While testing syscall tracepoints proposed by Jason, I found some
entries were missing when reading available_events.
It turned out there's bug in seqfile handling. The bug is, it's
wrong to increment @pos in seq start().
I fixed similar bugs in some other places. (the last patch fixes
a different seqfile bug)
[PATCH 1/5] tracing/events: don't increment @pos in s_start()
[PATCH 2/5] tracing_bprintk: don't increment @pos in t_start()
[PATCH 3/5] trace_stat: don't increment @pos in stat_seq_start()
[PATCH 4/5] ftrace: don't increment @pos in g_start()
[PATCH 5/5] tracing: reset iterator in t_start()
---
kernel/trace/ftrace.c | 28 +++++++++++++---------------
kernel/trace/trace.c | 18 ++++--------------
kernel/trace/trace_events.c | 28 ++++++++++++++++++++++------
kernel/trace/trace_printk.c | 26 ++++++--------------------
kernel/trace/trace_stat.c | 6 +-----
5 files changed, 46 insertions(+), 60 deletions(-)
While testing syscall tracepoints posted by Jason, I found 3 entries
were missing when reading available_events. The output size of
available_events is < 4 pages, which means we lost 1 item per page.
The cause is, it's wrong to increment @pos in s_start().
Actually there's another bug here -- reading avaiable_events/set_events
can race with module unload:
# cat available_events |
s_start() |
s_stop() |
| # rmmod foo.ko
s_start() |
call = list_entry(m->private) |
@call might be freed and accessing it will lead to crash.
[ Impact fix missing entries when reading available_events/set_events ]
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_events.c | 28 ++++++++++++++++++++++------
1 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index aa341ff..4ab596e 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -300,10 +300,18 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
static void *t_start(struct seq_file *m, loff_t *pos)
{
+ struct ftrace_event_call *call = NULL;
+ loff_t l;
+
mutex_lock(&event_mutex);
- if (*pos == 0)
- m->private = ftrace_events.next;
- return t_next(m, NULL, pos);
+
+ m->private = ftrace_events.next;
+ for (l = 0; l <= *pos; ) {
+ call = t_next(m, NULL, &l);
+ if (!call)
+ break;
+ }
+ return call;
}
static void *
@@ -332,10 +340,18 @@ s_next(struct seq_file *m, void *v, loff_t *pos)
static void *s_start(struct seq_file *m, loff_t *pos)
{
+ struct ftrace_event_call *call = NULL;
+ loff_t l;
+
mutex_lock(&event_mutex);
- if (*pos == 0)
- m->private = ftrace_events.next;
- return s_next(m, NULL, pos);
+
+ m->private = ftrace_events.next;
+ for (l = 0; l <= *pos; ) {
+ call = s_next(m, NULL, &l);
+ if (!call)
+ break;
+ }
+ return call;
}
static int t_show(struct seq_file *m, void *v)
--
1.5.4.rc3
It's wrong to increment @pos in t_start(), otherwise we'll lose
some entries when reading printk_formats, if the output is large
than PAGE_SIZE.
[ Impact: fix missing entries when reading printk_formats ]
Reported-by: Lai Jiangshan <[email protected]>
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_printk.c | 26 ++++++--------------------
1 files changed, 6 insertions(+), 20 deletions(-)
diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index 9bece96..7b62781 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -155,25 +155,19 @@ int __ftrace_vprintk(unsigned long ip, const char *fmt, va_list ap)
EXPORT_SYMBOL_GPL(__ftrace_vprintk);
static void *
-t_next(struct seq_file *m, void *v, loff_t *pos)
+t_start(struct seq_file *m, loff_t *pos)
{
- const char **fmt = m->private;
- const char **next = fmt;
-
- (*pos)++;
+ const char **fmt = __start___trace_bprintk_fmt + *pos;
if ((unsigned long)fmt >= (unsigned long)__stop___trace_bprintk_fmt)
return NULL;
-
- next = fmt;
- m->private = ++next;
-
return fmt;
}
-static void *t_start(struct seq_file *m, loff_t *pos)
+static void *t_next(struct seq_file *m, void * v, loff_t *pos)
{
- return t_next(m, NULL, pos);
+ (*pos)++;
+ return t_start(m, pos);
}
static int t_show(struct seq_file *m, void *v)
@@ -224,15 +218,7 @@ static const struct seq_operations show_format_seq_ops = {
static int
ftrace_formats_open(struct inode *inode, struct file *file)
{
- int ret;
-
- ret = seq_open(file, &show_format_seq_ops);
- if (!ret) {
- struct seq_file *m = file->private_data;
-
- m->private = __start___trace_bprintk_fmt;
- }
- return ret;
+ return seq_open(file, &show_format_seq_ops);
}
static const struct file_operations ftrace_formats_fops = {
--
1.5.4.rc3
It's wrong to increment @pos in stat_seq_start(). It causes some
stat entries lost when reading stat file, if the output of the file
is large than PAGE_SIZE.
[ Impact: fix missing entries when reading a stat file ]
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_stat.c | 6 +-----
1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
index c006437..e66f5e4 100644
--- a/kernel/trace/trace_stat.c
+++ b/kernel/trace/trace_stat.c
@@ -199,17 +199,13 @@ static void *stat_seq_start(struct seq_file *s, loff_t *pos)
mutex_lock(&session->stat_mutex);
/* If we are in the beginning of the file, print the headers */
- if (!*pos && session->ts->stat_headers) {
- (*pos)++;
+ if (!*pos && session->ts->stat_headers)
return SEQ_START_TOKEN;
- }
node = rb_first(&session->stat_root);
for (i = 0; node && i < *pos; i++)
node = rb_next(node);
- (*pos)++;
-
return node;
}
--
1.5.4.rc3
It's wrong to increment @pos in g_start(). It causes some entries
lost when reading set_graph_function, if the output of the file
is large than PAGE_SIZE.
[ Impact: fix missing entries when reading set_graph_function ]
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/ftrace.c | 28 +++++++++++++---------------
1 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index ec18bb4..2c1c761 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2492,32 +2492,30 @@ int ftrace_graph_count;
unsigned long ftrace_graph_funcs[FTRACE_GRAPH_MAX_FUNCS] __read_mostly;
static void *
-g_next(struct seq_file *m, void *v, loff_t *pos)
+__g_next(struct seq_file *m, loff_t *pos)
{
unsigned long *array = m->private;
- int index = *pos;
- (*pos)++;
+ /* Nothing, tell g_show to print all functions are enabled */
+ if (!ftrace_graph_count && !*pos)
+ return (void *)1;
- if (index >= ftrace_graph_count)
+ if (*pos >= ftrace_graph_count)
return NULL;
+ return &array[*pos];
+}
- return &array[index];
+static void *
+g_next(struct seq_file *m, void *v, loff_t *pos)
+{
+ (*pos)++;
+ return __g_next(m, pos);
}
static void *g_start(struct seq_file *m, loff_t *pos)
{
- void *p = NULL;
-
mutex_lock(&graph_lock);
-
- /* Nothing, tell g_show to print all functions are enabled */
- if (!ftrace_graph_count && !*pos)
- return (void *)1;
-
- p = g_next(m, p, pos);
-
- return p;
+ return __g_next(m, pos);
}
static void g_stop(struct seq_file *m, void *p)
--
1.5.4.rc3
The iterator is m->private, but it's not reset to trace_types in
t_start(). If the output is larger than PAGE_SIZE and t_start()
is called the 2nd time, things will go wrong.
[ Impact: fix output of current_tracer when it's larger than PAGE_SIZE ]
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace.c | 18 ++++--------------
1 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 076fa6f..3bb3100 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2053,25 +2053,23 @@ static int tracing_open(struct inode *inode, struct file *file)
static void *
t_next(struct seq_file *m, void *v, loff_t *pos)
{
- struct tracer *t = m->private;
+ struct tracer *t = v;
(*pos)++;
if (t)
t = t->next;
- m->private = t;
-
return t;
}
static void *t_start(struct seq_file *m, loff_t *pos)
{
- struct tracer *t = m->private;
+ struct tracer *t;
loff_t l = 0;
mutex_lock(&trace_types_lock);
- for (; t && l < *pos; t = t_next(m, t, &l))
+ for (t = trace_types; t && l < *pos; t = t_next(m, t, &l))
;
return t;
@@ -2107,18 +2105,10 @@ static struct seq_operations show_traces_seq_ops = {
static int show_traces_open(struct inode *inode, struct file *file)
{
- int ret;
-
if (tracing_disabled)
return -ENODEV;
- ret = seq_open(file, &show_traces_seq_ops);
- if (!ret) {
- struct seq_file *m = file->private_data;
- m->private = trace_types;
- }
-
- return ret;
+ return seq_open(file, &show_traces_seq_ops);
}
static ssize_t
--
1.5.4.rc3
how about this one?
It's wrong to increment @pos in g_start(). It causes some entries
lost when reading set_graph_function, if the output of the file
is large than PAGE_SIZE.
[ Impact: fix missing entries when reading set_graph_function ]
Signed-off-by: Li Zefan <[email protected]>
Signed-off-by: Liming Wang <[email protected]>
---
kernel/trace/ftrace.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 134e580..1beaac6 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2495,29 +2495,25 @@ static void *
g_next(struct seq_file *m, void *v, loff_t *pos)
{
unsigned long *array = m->private;
- int index = *pos;
- (*pos)++;
+ if (v)
+ (*pos)++;
- if (index >= ftrace_graph_count)
+ if (*pos >= ftrace_graph_count)
return NULL;
- return &array[index];
+ return &array[*pos];
}
static void *g_start(struct seq_file *m, loff_t *pos)
{
- void *p = NULL;
-
mutex_lock(&graph_lock);
/* Nothing, tell g_show to print all functions are enabled */
if (!ftrace_graph_count && !*pos)
return (void *)1;
- p = g_next(m, p, pos);
-
- return p;
+ return g_next(m, NULL, pos);
}
static void g_stop(struct seq_file *m, void *p)
--
1.6.0.3
Liming Wang wrote:
> how about this one?
>
Yeah, this should work, and cleaner than my version.
> It's wrong to increment @pos in g_start(). It causes some entries
> lost when reading set_graph_function, if the output of the file
> is large than PAGE_SIZE.
>
> [ Impact: fix missing entries when reading set_graph_function ]
>
> Signed-off-by: Li Zefan <[email protected]>
> Signed-off-by: Liming Wang <[email protected]>
> ---
> kernel/trace/ftrace.c | 14 +++++---------
> 1 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 134e580..1beaac6 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2495,29 +2495,25 @@ static void *
> g_next(struct seq_file *m, void *v, loff_t *pos)
> {
> unsigned long *array = m->private;
> - int index = *pos;
>
> - (*pos)++;
> + if (v)
> + (*pos)++;
>
> - if (index >= ftrace_graph_count)
> + if (*pos >= ftrace_graph_count)
> return NULL;
>
> - return &array[index];
> + return &array[*pos];
> }
>
> static void *g_start(struct seq_file *m, loff_t *pos)
> {
> - void *p = NULL;
> -
> mutex_lock(&graph_lock);
>
> /* Nothing, tell g_show to print all functions are enabled */
> if (!ftrace_graph_count && !*pos)
> return (void *)1;
>
> - p = g_next(m, p, pos);
> -
> - return p;
> + return g_next(m, NULL, pos);
> }
>
> static void g_stop(struct seq_file *m, void *p)
Hi,
Li Zefan wrote:
> It's wrong to increment @pos in t_start(), otherwise we'll lose
> some entries when reading printk_formats, if the output is large
> than PAGE_SIZE.
>
> [ Impact: fix missing entries when reading printk_formats ]
>
> Reported-by: Lai Jiangshan <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/trace/trace_printk.c | 26 ++++++--------------------
> 1 files changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index 9bece96..7b62781 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -155,25 +155,19 @@ int __ftrace_vprintk(unsigned long ip, const char *fmt, va_list ap)
> EXPORT_SYMBOL_GPL(__ftrace_vprintk);
>
> static void *
> -t_next(struct seq_file *m, void *v, loff_t *pos)
> +t_start(struct seq_file *m, loff_t *pos)
> {
> - const char **fmt = m->private;
> - const char **next = fmt;
> -
> - (*pos)++;
> + const char **fmt = __start___trace_bprintk_fmt + *pos;
>
> if ((unsigned long)fmt >= (unsigned long)__stop___trace_bprintk_fmt)
> return NULL;
> -
> - next = fmt;
> - m->private = ++next;
> -
> return fmt;
> }
>
> -static void *t_start(struct seq_file *m, loff_t *pos)
> +static void *t_next(struct seq_file *m, void * v, loff_t *pos)
> {
> - return t_next(m, NULL, pos);
> + (*pos)++;
> + return t_start(m, pos);
> }
>
I prefer to .start to call .next, so I rewrite it to following:
@@ -157,17 +157,15 @@ EXPORT_SYMBOL_GPL(__ftrace_vprintk);
static void *
t_next(struct seq_file *m, void *v, loff_t *pos)
{
- const char **fmt = m->private;
- const char **next = fmt;
+ const char **fmt;
- (*pos)++;
+ if (v)
+ (*pos)++;
+ fmt = __start___trace_bprintk_fmt + *pos;
if ((unsigned long)fmt >= (unsigned long)__stop___trace_bprintk_fmt)
return NULL;
- next = fmt;
- m->private = ++next;
-
return fmt;
}
but I have not tested it, if you have no objection, then:
Signed-off-by: walimis <[email protected]>
Liming Wang
> static int t_show(struct seq_file *m, void *v)
> @@ -224,15 +218,7 @@ static const struct seq_operations show_format_seq_ops = {
> static int
> ftrace_formats_open(struct inode *inode, struct file *file)
> {
> - int ret;
> -
> - ret = seq_open(file, &show_format_seq_ops);
> - if (!ret) {
> - struct seq_file *m = file->private_data;
> -
> - m->private = __start___trace_bprintk_fmt;
> - }
> - return ret;
> + return seq_open(file, &show_format_seq_ops);
> }
>
> static const struct file_operations ftrace_formats_fops = {
Wang Liming wrote:
> Hi,
>
> Li Zefan wrote:
>> It's wrong to increment @pos in t_start(), otherwise we'll lose
>> some entries when reading printk_formats, if the output is large
>> than PAGE_SIZE.
>>
>> [ Impact: fix missing entries when reading printk_formats ]
>>
>> Reported-by: Lai Jiangshan <[email protected]>
>> Signed-off-by: Li Zefan <[email protected]>
>> ---
>> kernel/trace/trace_printk.c | 26 ++++++--------------------
>> 1 files changed, 6 insertions(+), 20 deletions(-)
>>
>> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
>> index 9bece96..7b62781 100644
>> --- a/kernel/trace/trace_printk.c
>> +++ b/kernel/trace/trace_printk.c
>> @@ -155,25 +155,19 @@ int __ftrace_vprintk(unsigned long ip, const
>> char *fmt, va_list ap)
>> EXPORT_SYMBOL_GPL(__ftrace_vprintk);
>>
>> static void *
>> -t_next(struct seq_file *m, void *v, loff_t *pos)
>> +t_start(struct seq_file *m, loff_t *pos)
>> {
>> - const char **fmt = m->private;
>> - const char **next = fmt;
>> -
>> - (*pos)++;
>> + const char **fmt = __start___trace_bprintk_fmt + *pos;
>>
>> if ((unsigned long)fmt >= (unsigned long)__stop___trace_bprintk_fmt)
>> return NULL;
>> -
>> - next = fmt;
>> - m->private = ++next;
>> -
>> return fmt;
>> }
>>
>> -static void *t_start(struct seq_file *m, loff_t *pos)
>> +static void *t_next(struct seq_file *m, void * v, loff_t *pos)
>> {
>> - return t_next(m, NULL, pos);
>> + (*pos)++;
>> + return t_start(m, pos);
>> }
>>
> I prefer to .start to call .next, so I rewrite it to following:
>
Thanks for the comment, but I don't think .next calls .start is bad,
and I'm not the only one doing this. Grep c_start() to see some of
them.
Li Zefan wrote:
> Liming Wang wrote:
>> how about this one?
>>
>
> Yeah, this should work, and cleaner than my version.
>
Hmmm, the patch is cleaner in diffstat but the resulted code
isn't..
After yours:
text data bss dec hex filename
14879 5480 4240 24599 6017 kernel/trace/ftrace.o
After mine:
text data bss dec hex filename
14873 5480 4240 24593 6011 kernel/trace/ftrace.o
>> It's wrong to increment @pos in g_start(). It causes some entries
>> lost when reading set_graph_function, if the output of the file
>> is large than PAGE_SIZE.
>>
>> [ Impact: fix missing entries when reading set_graph_function ]
>>
>> Signed-off-by: Li Zefan <[email protected]>
>> Signed-off-by: Liming Wang <[email protected]>
>> ---
>> kernel/trace/ftrace.c | 14 +++++---------
>> 1 files changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 134e580..1beaac6 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -2495,29 +2495,25 @@ static void *
>> g_next(struct seq_file *m, void *v, loff_t *pos)
>> {
>> unsigned long *array = m->private;
>> - int index = *pos;
>>
>> - (*pos)++;
>> + if (v)
>> + (*pos)++;
>>
>> - if (index >= ftrace_graph_count)
>> + if (*pos >= ftrace_graph_count)
>> return NULL;
>>
>> - return &array[index];
>> + return &array[*pos];
>> }
>>
>> static void *g_start(struct seq_file *m, loff_t *pos)
>> {
>> - void *p = NULL;
>> -
>> mutex_lock(&graph_lock);
>>
>> /* Nothing, tell g_show to print all functions are enabled */
>> if (!ftrace_graph_count && !*pos)
>> return (void *)1;
>>
>> - p = g_next(m, p, pos);
>> -
>> - return p;
>> + return g_next(m, NULL, pos);
>> }
>>
>> static void g_stop(struct seq_file *m, void *p)
Li Zefan wrote:
> Wang Liming wrote:
>> Hi,
>>
>> Li Zefan wrote:
>>> It's wrong to increment @pos in t_start(), otherwise we'll lose
>>> some entries when reading printk_formats, if the output is large
>>> than PAGE_SIZE.
>>>
>>> [ Impact: fix missing entries when reading printk_formats ]
>>>
>>> Reported-by: Lai Jiangshan <[email protected]>
>>> Signed-off-by: Li Zefan <[email protected]>
>>> ---
>>> kernel/trace/trace_printk.c | 26 ++++++--------------------
>>> 1 files changed, 6 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
>>> index 9bece96..7b62781 100644
>>> --- a/kernel/trace/trace_printk.c
>>> +++ b/kernel/trace/trace_printk.c
>>> @@ -155,25 +155,19 @@ int __ftrace_vprintk(unsigned long ip, const
>>> char *fmt, va_list ap)
>>> EXPORT_SYMBOL_GPL(__ftrace_vprintk);
>>>
>>> static void *
>>> -t_next(struct seq_file *m, void *v, loff_t *pos)
>>> +t_start(struct seq_file *m, loff_t *pos)
>>> {
>>> - const char **fmt = m->private;
>>> - const char **next = fmt;
>>> -
>>> - (*pos)++;
>>> + const char **fmt = __start___trace_bprintk_fmt + *pos;
>>>
>>> if ((unsigned long)fmt >= (unsigned long)__stop___trace_bprintk_fmt)
>>> return NULL;
>>> -
>>> - next = fmt;
>>> - m->private = ++next;
>>> -
>>> return fmt;
>>> }
>>>
>>> -static void *t_start(struct seq_file *m, loff_t *pos)
>>> +static void *t_next(struct seq_file *m, void * v, loff_t *pos)
>>> {
>>> - return t_next(m, NULL, pos);
>>> + (*pos)++;
>>> + return t_start(m, pos);
>>> }
>>>
>> I prefer to .start to call .next, so I rewrite it to following:
>>
>
> Thanks for the comment, but I don't think .next calls .start is bad,
> and I'm not the only one doing this. Grep c_start() to see some of
> them.
Yes, you are not the only one, but it's the only one in the tracing code. :)
I just think we should make the seq_* uniform so that we can understand them
more clearly.
Liming Wang
>
>
Li Zefan wrote:
> Li Zefan wrote:
>> Liming Wang wrote:
>>> how about this one?
>>>
>> Yeah, this should work, and cleaner than my version.
>>
>
> Hmmm, the patch is cleaner in diffstat but the resulted code
> isn't..
>
> After yours:
> text data bss dec hex filename
> 14879 5480 4240 24599 6017 kernel/trace/ftrace.o
>
> After mine:
> text data bss dec hex filename
> 14873 5480 4240 24593 6011 kernel/trace/ftrace.o
Hmmm, if you prefer to smaller target size, I don't care.
But in my system, I got the same size:
text data bss dec hex filename
14330 5019 104 19453 4bfd kernel/trace/ftrace.o
I use objdump to compute the actual size of all modified functions:
After mine:
func size
g_start 0x50
g_next 0x70
After yours:
func size
__g_next 0x70
g_next 0x20
g_start 0x30
I used Steve git tree and commit e482f8395f215e0ad6557b2722cd9b9b308035c4.
My gcc version is :
gcc version 4.2.4
I don't know where the difference.
Liming Wang
>
>
>>> It's wrong to increment @pos in g_start(). It causes some entries
>>> lost when reading set_graph_function, if the output of the file
>>> is large than PAGE_SIZE.
>>>
>>> [ Impact: fix missing entries when reading set_graph_function ]
>>>
>>> Signed-off-by: Li Zefan <[email protected]>
>>> Signed-off-by: Liming Wang <[email protected]>
>>> ---
>>> kernel/trace/ftrace.c | 14 +++++---------
>>> 1 files changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>>> index 134e580..1beaac6 100644
>>> --- a/kernel/trace/ftrace.c
>>> +++ b/kernel/trace/ftrace.c
>>> @@ -2495,29 +2495,25 @@ static void *
>>> g_next(struct seq_file *m, void *v, loff_t *pos)
>>> {
>>> unsigned long *array = m->private;
>>> - int index = *pos;
>>>
>>> - (*pos)++;
>>> + if (v)
>>> + (*pos)++;
>>>
>>> - if (index >= ftrace_graph_count)
>>> + if (*pos >= ftrace_graph_count)
>>> return NULL;
>>>
>>> - return &array[index];
>>> + return &array[*pos];
>>> }
>>>
>>> static void *g_start(struct seq_file *m, loff_t *pos)
>>> {
>>> - void *p = NULL;
>>> -
>>> mutex_lock(&graph_lock);
>>>
>>> /* Nothing, tell g_show to print all functions are enabled */
>>> if (!ftrace_graph_count && !*pos)
>>> return (void *)1;
>>>
>>> - p = g_next(m, p, pos);
>>> -
>>> - return p;
>>> + return g_next(m, NULL, pos);
>>> }
>>>
>>> static void g_stop(struct seq_file *m, void *p)
>
Wang Liming wrote:
> Li Zefan wrote:
>> Wang Liming wrote:
>>> Hi,
>>>
>>> Li Zefan wrote:
>>>> It's wrong to increment @pos in t_start(), otherwise we'll lose
>>>> some entries when reading printk_formats, if the output is large
>>>> than PAGE_SIZE.
>>>>
>>>> [ Impact: fix missing entries when reading printk_formats ]
>>>>
>>>> Reported-by: Lai Jiangshan <[email protected]>
>>>> Signed-off-by: Li Zefan <[email protected]>
>>>> ---
>>>> kernel/trace/trace_printk.c | 26 ++++++--------------------
>>>> 1 files changed, 6 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
>>>> index 9bece96..7b62781 100644
>>>> --- a/kernel/trace/trace_printk.c
>>>> +++ b/kernel/trace/trace_printk.c
>>>> @@ -155,25 +155,19 @@ int __ftrace_vprintk(unsigned long ip, const
>>>> char *fmt, va_list ap)
>>>> EXPORT_SYMBOL_GPL(__ftrace_vprintk);
>>>>
>>>> static void *
>>>> -t_next(struct seq_file *m, void *v, loff_t *pos)
>>>> +t_start(struct seq_file *m, loff_t *pos)
>>>> {
>>>> - const char **fmt = m->private;
>>>> - const char **next = fmt;
>>>> -
>>>> - (*pos)++;
>>>> + const char **fmt = __start___trace_bprintk_fmt + *pos;
>>>>
>>>> if ((unsigned long)fmt >= (unsigned
>>>> long)__stop___trace_bprintk_fmt)
>>>> return NULL;
>>>> -
>>>> - next = fmt;
>>>> - m->private = ++next;
>>>> -
>>>> return fmt;
>>>> }
>>>>
>>>> -static void *t_start(struct seq_file *m, loff_t *pos)
>>>> +static void *t_next(struct seq_file *m, void * v, loff_t *pos)
>>>> {
>>>> - return t_next(m, NULL, pos);
>>>> + (*pos)++;
>>>> + return t_start(m, pos);
>>>> }
>>>>
>>> I prefer to .start to call .next, so I rewrite it to following:
>>>
>>
>> Thanks for the comment, but I don't think .next calls .start is bad,
>> and I'm not the only one doing this. Grep c_start() to see some of
>> them.
> Yes, you are not the only one, but it's the only one in the tracing
> code. :)
> I just think we should make the seq_* uniform so that we can understand
> them more clearly.
>
I don't see how this make seq_* un-uniform..
And I don't want to add extra checking for this kind of uniform.
And if some next()s check (v == NULL) while others don't, do
you think it's uniform or not?
Wang Liming wrote:
> Li Zefan wrote:
>> Li Zefan wrote:
>>> Liming Wang wrote:
>>>> how about this one?
>>>>
>>> Yeah, this should work, and cleaner than my version.
>>>
>>
>> Hmmm, the patch is cleaner in diffstat but the resulted code
>> isn't..
>>
>> After yours:
>> text data bss dec hex filename
>> 14879 5480 4240 24599 6017 kernel/trace/ftrace.o
>>
>> After mine:
>> text data bss dec hex filename
>> 14873 5480 4240 24593 6011 kernel/trace/ftrace.o
> Hmmm, if you prefer to smaller target size, I don't care.
> But in my system, I got the same size:
>
> text data bss dec hex filename
> 14330 5019 104 19453 4bfd kernel/trace/ftrace.o
>
> I use objdump to compute the actual size of all modified functions:
>
> After mine:
> func size
> g_start 0x50
> g_next 0x70
>
> After yours:
> func size
> __g_next 0x70
> g_next 0x20
> g_start 0x30
>
> I used Steve git tree and commit e482f8395f215e0ad6557b2722cd9b9b308035c4.
> My gcc version is :
> gcc version 4.2.4
>
> I don't know where the difference.
>
Maybe because of different gcc versions:
# gcc --version
gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-33)
The point is, I don't see how the patch you posted is better than
mine. And it's fine for me to pick up yours if it's indeed better.
Li Zefan wrote:
> Wang Liming wrote:
>> Li Zefan wrote:
>>> Li Zefan wrote:
>>>> Liming Wang wrote:
>>>>> how about this one?
>>>>>
>>>> Yeah, this should work, and cleaner than my version.
>>>>
>>> Hmmm, the patch is cleaner in diffstat but the resulted code
>>> isn't..
>>>
>>> After yours:
>>> text data bss dec hex filename
>>> 14879 5480 4240 24599 6017 kernel/trace/ftrace.o
>>>
>>> After mine:
>>> text data bss dec hex filename
>>> 14873 5480 4240 24593 6011 kernel/trace/ftrace.o
>> Hmmm, if you prefer to smaller target size, I don't care.
>> But in my system, I got the same size:
>>
>> text data bss dec hex filename
>> 14330 5019 104 19453 4bfd kernel/trace/ftrace.o
>>
>> I use objdump to compute the actual size of all modified functions:
>>
>> After mine:
>> func size
>> g_start 0x50
>> g_next 0x70
>>
>> After yours:
>> func size
>> __g_next 0x70
>> g_next 0x20
>> g_start 0x30
>>
>> I used Steve git tree and commit e482f8395f215e0ad6557b2722cd9b9b308035c4.
>> My gcc version is :
>> gcc version 4.2.4
>>
>> I don't know where the difference.
>>
>
> Maybe because of different gcc versions:
>
> # gcc --version
> gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-33)
Maybe.
>
> The point is, I don't see how the patch you posted is better than
> mine. And it's fine for me to pick up yours if it's indeed better.
OK, it's fine to me to pick up yours. Nothing different.
Back to your patch:
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index ec18bb4..2c1c761 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2492,32 +2492,30 @@ int ftrace_graph_count;
> unsigned long ftrace_graph_funcs[FTRACE_GRAPH_MAX_FUNCS] __read_mostly;
>
> static void *
> -g_next(struct seq_file *m, void *v, loff_t *pos)
> +__g_next(struct seq_file *m, loff_t *pos)
> {
> unsigned long *array = m->private;
> - int index = *pos;
>
> - (*pos)++;
> + /* Nothing, tell g_show to print all functions are enabled */
> + if (!ftrace_graph_count && !*pos)
> + return (void *)1;
I think this checking should be put back to g_start, because it's only necessary
for g_start.
Liming Wang
>
Li Zefan wrote:
> The iterator is m->private, but it's not reset to trace_types in
> t_start(). If the output is larger than PAGE_SIZE and t_start()
> is called the 2nd time, things will go wrong.
>
> [ Impact: fix output of current_tracer when it's larger than PAGE_SIZE ]
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/trace/trace.c | 18 ++++--------------
> 1 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 076fa6f..3bb3100 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2053,25 +2053,23 @@ static int tracing_open(struct inode *inode, struct file *file)
> static void *
> t_next(struct seq_file *m, void *v, loff_t *pos)
> {
> - struct tracer *t = m->private;
> + struct tracer *t = v;
>
> (*pos)++;
>
> if (t)
> t = t->next;
>
> - m->private = t;
> -
> return t;
> }
>
> static void *t_start(struct seq_file *m, loff_t *pos)
> {
> - struct tracer *t = m->private;
> + struct tracer *t;
> loff_t l = 0;
>
> mutex_lock(&trace_types_lock);
> - for (; t && l < *pos; t = t_next(m, t, &l))
> + for (t = trace_types; t && l < *pos; t = t_next(m, t, &l))
> ;
>
> return t;
> @@ -2107,18 +2105,10 @@ static struct seq_operations show_traces_seq_ops = {
>
> static int show_traces_open(struct inode *inode, struct file *file)
> {
> - int ret;
> -
> if (tracing_disabled)
> return -ENODEV;
>
> - ret = seq_open(file, &show_traces_seq_ops);
> - if (!ret) {
> - struct seq_file *m = file->private_data;
> - m->private = trace_types;
> - }
> -
> - return ret;
> + return seq_open(file, &show_traces_seq_ops);
> }
>
> static ssize_t
Another version:
Since we have saved current (struct tracer *) in m->private in .next, in .start,
we don't need to call .next to find the one that should be printed in 2nd or nth
time.
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index cae34c6..02cdccc 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2055,8 +2055,6 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
{
struct tracer *t = m->private;
- (*pos)++;
-
if (t)
t = t->next;
@@ -2068,11 +2066,8 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
static void *t_start(struct seq_file *m, loff_t *pos)
{
struct tracer *t = m->private;
- loff_t l = 0;
mutex_lock(&trace_types_lock);
- for (; t && l < *pos; t = t_next(m, t, &l))
- ;
return t;
}
> Another version:
> Since we have saved current (struct tracer *) in m->private in .next, in
> .start, we don't need to call .next to find the one that should be
> printed in 2nd or nth time.
>
I don't like this for 2 reasons.
1. It's strange that @pos is not incremented in next().
2.
t_stop()
mutex_unlock()
unregister_tracer(t)
t_start()
mutex_lock()
t = m->private
...
t = t-next.
We access t->next though @t was unregistered. This is not
good, though it does no harm here.
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index cae34c6..02cdccc 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2055,8 +2055,6 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
> {
> struct tracer *t = m->private;
>
> - (*pos)++;
> -
> if (t)
> t = t->next;
>
> @@ -2068,11 +2066,8 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
> static void *t_start(struct seq_file *m, loff_t *pos)
> {
> struct tracer *t = m->private;
> - loff_t l = 0;
>
> mutex_lock(&trace_types_lock);
> - for (; t && l < *pos; t = t_next(m, t, &l))
> - ;
>
> return t;
> }
>
>
Li Zefan wrote:
>> Another version:
>> Since we have saved current (struct tracer *) in m->private in .next, in
>> .start, we don't need to call .next to find the one that should be
>> printed in 2nd or nth time.
>>
>
> I don't like this for 2 reasons.
>
> 1. It's strange that @pos is not incremented in next().
Yes, it's strang, but we know that @pos sometimes is not necessary, such in this
position.
>
> 2.
> t_stop()
> mutex_unlock()
> unregister_tracer(t)
> t_start()
> mutex_lock()
> t = m->private
> ...
> t = t-next.
>
> We access t->next though @t was unregistered. This is not
> good, though it does no harm here.
OK, it's a realy race problem if we call unregister_tracer.
btw: who realy calls this function? :)
Liming Wang
>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index cae34c6..02cdccc 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -2055,8 +2055,6 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
>> {
>> struct tracer *t = m->private;
>>
>> - (*pos)++;
>> -
>> if (t)
>> t = t->next;
>>
>> @@ -2068,11 +2066,8 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
>> static void *t_start(struct seq_file *m, loff_t *pos)
>> {
>> struct tracer *t = m->private;
>> - loff_t l = 0;
>>
>> mutex_lock(&trace_types_lock);
>> - for (; t && l < *pos; t = t_next(m, t, &l))
>> - ;
>>
>> return t;
>> }
>>
>>
>