[PATCH 0/5] tracing: seqfile fixes

[PATCH 0/5] tracing: seqfile fixes

[Li Zefan]

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

[PATCH 1/5] tracing/events: don't increment @pos in s_start()

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

[PATCH 2/5] tracing_bprintk: don't increment @pos in t_start()

[Li Zefan]

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 <laijs@cn.fujitsu.com>
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 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

[PATCH 3/5] trace_stat: don't increment @pos in stat_seq_start()

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

[PATCH 4/5] ftrace: don't increment @pos in g_start()

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

[PATCH 5/5] tracing: reset iterator in t_start()

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

[PATCH] ftrace: don't increment @pos in g_start()

[Liming Wang]

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 <lizf@cn.fujitsu.com>
Signed-off-by: Liming Wang <liming.wang@windriver.com>
---
 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

Re: [PATCH 2/5] tracing_bprintk: don't increment @pos in t_start()

[Wang Liming]

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 <laijs@cn.fujitsu.com>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  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 <walimisdev@gmail.com>

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 = {

Re: [PATCH] ftrace: don't increment @pos in g_start()

[Li Zefan]

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 <lizf@cn.fujitsu.com>
> Signed-off-by: Liming Wang <liming.wang@windriver.com>
> ---
>  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)

Re: [PATCH 2/5] tracing_bprintk: don't increment @pos in t_start()

[Li Zefan]

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 <laijs@cn.fujitsu.com>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  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.

Re: [PATCH] ftrace: don't increment @pos in g_start()

[Li Zefan]

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 <lizf@cn.fujitsu.com>
>> Signed-off-by: Liming Wang <liming.wang@windriver.com>
>> ---
>>  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)

Re: [PATCH 2/5] tracing_bprintk: don't increment @pos in t_start()

[Wang Liming]

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 <laijs@cn.fujitsu.com>
>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>>> ---
>>>  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
> 
> 

Re: [PATCH] ftrace: don't increment @pos in g_start()

[Wang Liming]

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 <lizf@cn.fujitsu.com>
>>> Signed-off-by: Liming Wang <liming.wang@windriver.com>
>>> ---
>>>  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)
> 

Re: [PATCH 2/5] tracing_bprintk: don't increment @pos in t_start()

[Li Zefan]

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 <laijs@cn.fujitsu.com>
>>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>>>> ---
>>>>  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?

Re: [PATCH] ftrace: don't increment @pos in g_start()

[Wang Liming]

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

> 

Re: [PATCH] ftrace: don't increment @pos in g_start()

[Li Zefan]

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.

Re: [PATCH 5/5] tracing: reset iterator in t_start()

[Wang Liming]

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 <lizf@cn.fujitsu.com>
> ---
>  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;
  }

Re: [PATCH 5/5] tracing: reset iterator in t_start()

[Li Zefan]

> 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;
>  }
> 
> 

Re: [PATCH 5/5] tracing: reset iterator in t_start()

[Wang Liming]

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;
>>  }
>>
>>
>