[PATCH 07/13] tracing/kdb: remove redundant checking

[PATCH 07/13] tracing/kdb: remove redundant checking

[zhangwei(Jovi)]

trace_empty is checking in while-loop, so the previous checking
is totally redundant, and more worse, the first trace entry is losted.

so remove it.

Signed-off-by: zhangwei(Jovi) <jovi.zhangwei@huawei.com>
---
 kernel/trace/trace_kdb.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
index 3c5c5df..6489b2e 100644
--- a/kernel/trace/trace_kdb.c
+++ b/kernel/trace/trace_kdb.c
@@ -57,8 +57,7 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
 		ring_buffer_read_start(iter.buffer_iter[cpu_file]);
 		tracing_iter_reset(&iter, cpu_file);
 	}
-	if (!trace_empty(&iter))
-		trace_find_next_entry_inc(&iter);
+
 	while (!trace_empty(&iter)) {
 		if (!cnt)
 			kdb_printf("---------------------------------\n");
-- 
1.7.9.7

Re: [PATCH 07/13] tracing/kdb: remove redundant checking

[Steven Rostedt]

This is Jason's code.

Jason, please give an Ack or Nack.

Thanks,

-- Steve


On Mon, 2013-03-11 at 15:13 +0800, zhangwei(Jovi) wrote:
> trace_empty is checking in while-loop, so the previous checking
> is totally redundant, and more worse, the first trace entry is losted.
> 
> so remove it.
> 
> Signed-off-by: zhangwei(Jovi) <jovi.zhangwei@huawei.com>
> ---
>  kernel/trace/trace_kdb.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
> index 3c5c5df..6489b2e 100644
> --- a/kernel/trace/trace_kdb.c
> +++ b/kernel/trace/trace_kdb.c
> @@ -57,8 +57,7 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
>  		ring_buffer_read_start(iter.buffer_iter[cpu_file]);
>  		tracing_iter_reset(&iter, cpu_file);
>  	}
> -	if (!trace_empty(&iter))
> -		trace_find_next_entry_inc(&iter);
> +
>  	while (!trace_empty(&iter)) {
>  		if (!cnt)
>  			kdb_printf("---------------------------------\n");

Re: [PATCH 07/13] tracing/kdb: remove redundant checking

[Jason Wessel]

On 03/11/2013 09:09 AM, Steven Rostedt wrote:
> This is Jason's code.
>
> Jason, please give an Ack or Nack.
>
> Thanks,
>
> -- Steve
>
>
> On Mon, 2013-03-11 at 15:13 +0800, zhangwei(Jovi) wrote:
>> trace_empty is checking in while-loop, so the previous checking
>> is totally redundant, and more worse, the first trace entry is losted.
>>
>> so remove it.
>>
>> Signed-off-by: zhangwei(Jovi) <jovi.zhangwei@huawei.com>
>> ---
>>  kernel/trace/trace_kdb.c |    3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
>> index 3c5c5df..6489b2e 100644
>> --- a/kernel/trace/trace_kdb.c
>> +++ b/kernel/trace/trace_kdb.c
>> @@ -57,8 +57,7 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
>>  		ring_buffer_read_start(iter.buffer_iter[cpu_file]);
>>  		tracing_iter_reset(&iter, cpu_file);
>>  	}
>> -	if (!trace_empty(&iter))
>> -		trace_find_next_entry_inc(&iter);
>> +
>>  	while (!trace_empty(&iter)) {
>>  		if (!cnt)
>>  			kdb_printf("---------------------------------\n");
>


This was just a copy of part of the logic used for printing the information in a similar manner to what you get when you cat the trace buffer to obtain the human readable version.

May I ask how you tested it though?

As far as I know the patch I sent Stephen quite a while ago got lost and the mainline version of the "ftdump" doesn't actually work.

Example:
[0]kdb> ftdump
Dumping ftrace buffer:
3BUG: sleeping function called from invalid context at /space/jw/git/kgdb/linux-2.6-kgdb/mm/slub.c:925
3in_atomic(): 1, irqs_disabled(): 1, pid: 81, name: sh
Pid: 81, comm: sh Not tainted 3.9.0-rc1-WR4.3.0.0_standard+ #568
Call Trace:
 <#DB>  [<ffffffff81069ffe>] __might_sleep+0xde/0x100
 [<ffffffff8112611b>] kmem_cache_alloc_trace+0xdb/0x170
 [<ffffffff810c01bd>] ring_buffer_read_prepare+0x4d/0x70
 [<ffffffff810d4d28>] kdb_ftdump+0x1e8/0x410
 [<ffffffff810a9a89>] kdb_parse+0x209/0x690
 [<ffffffff810aad0c>] kdb_main_loop+0x67c/0x8c0
 [<ffffffff810ad4b3>] kdb_stub+0x1d3/0x420
 [<ffffffff8104ccfd>] ? __send_signal+0x1ad/0x3e0
 [<ffffffff810a33be>] kgdb_cpu_enter+0x27e/0x590
 [<ffffffff810a3981>] kgdb_handle_exception+0x161/0x1c0
 [<ffffffff81027cf1>] __kgdb_notify+0x31/0xe0
 [<ffffffff81027e10>] kgdb_ll_trap+0x40/0x50
 [<ffffffff81002e12>] do_int3+0x52/0x130
 [<ffffffff81674345>] int3+0x25/0x40
 [<ffffffff810a2be2>] ? sysrq_handle_dbg+0x32/0x60
 <<EOE>>  [<ffffffff813e1e69>] __handle_sysrq+0x129/0x190


I think we need to actually empirically prove the code change is right or wrong before merging it, as well as cleaning up the change log slightly.

I'll go hunt down the patch the fixes the oops first.

Cheers,
Jason.

Re: [PATCH 07/13] tracing/kdb: remove redundant checking

[Jovi Zhang]

On Mon, Mar 11, 2013 at 10:46 PM, Jason Wessel
<jason.wessel@windriver.com> wrote:
> On 03/11/2013 09:09 AM, Steven Rostedt wrote:
>> This is Jason's code.
>>
>> Jason, please give an Ack or Nack.
>>
>> Thanks,
>>
>> -- Steve
>>
>>
>> On Mon, 2013-03-11 at 15:13 +0800, zhangwei(Jovi) wrote:
>>> trace_empty is checking in while-loop, so the previous checking
>>> is totally redundant, and more worse, the first trace entry is losted.
>>>
>>> so remove it.
>>>
>>> Signed-off-by: zhangwei(Jovi) <jovi.zhangwei@huawei.com>
>>> ---
>>>  kernel/trace/trace_kdb.c |    3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
>>> index 3c5c5df..6489b2e 100644
>>> --- a/kernel/trace/trace_kdb.c
>>> +++ b/kernel/trace/trace_kdb.c
>>> @@ -57,8 +57,7 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file)
>>>              ring_buffer_read_start(iter.buffer_iter[cpu_file]);
>>>              tracing_iter_reset(&iter, cpu_file);
>>>      }
>>> -    if (!trace_empty(&iter))
>>> -            trace_find_next_entry_inc(&iter);
>>> +
>>>      while (!trace_empty(&iter)) {
>>>              if (!cnt)
>>>                      kdb_printf("---------------------------------\n");
>>
>
>
> This was just a copy of part of the logic used for printing the information in a similar manner to what you get when you cat the trace buffer to obtain the human readable version.
>
> May I ask how you tested it though?
No, just watch the code, and that part looks weird.

kdb_ftdump seems copied from ftrace_dump function, it have similar
functionality,
can we have some way to unify these two function into one common function?
this at least can save trace_iterator static memory ~8k+. (I'm not
sure this can work or not)

>
> As far as I know the patch I sent Stephen quite a while ago got lost and the mainline version of the "ftdump" doesn't actually work.
>
> Example:
> [0]kdb> ftdump
> Dumping ftrace buffer:
> 3BUG: sleeping function called from invalid context at /space/jw/git/kgdb/linux-2.6-kgdb/mm/slub.c:925
> 3in_atomic(): 1, irqs_disabled(): 1, pid: 81, name: sh
> Pid: 81, comm: sh Not tainted 3.9.0-rc1-WR4.3.0.0_standard+ #568
> Call Trace:
>  <#DB>  [<ffffffff81069ffe>] __might_sleep+0xde/0x100
>  [<ffffffff8112611b>] kmem_cache_alloc_trace+0xdb/0x170
>  [<ffffffff810c01bd>] ring_buffer_read_prepare+0x4d/0x70
>  [<ffffffff810d4d28>] kdb_ftdump+0x1e8/0x410
>  [<ffffffff810a9a89>] kdb_parse+0x209/0x690
>  [<ffffffff810aad0c>] kdb_main_loop+0x67c/0x8c0
>  [<ffffffff810ad4b3>] kdb_stub+0x1d3/0x420
>  [<ffffffff8104ccfd>] ? __send_signal+0x1ad/0x3e0
>  [<ffffffff810a33be>] kgdb_cpu_enter+0x27e/0x590
>  [<ffffffff810a3981>] kgdb_handle_exception+0x161/0x1c0
>  [<ffffffff81027cf1>] __kgdb_notify+0x31/0xe0
>  [<ffffffff81027e10>] kgdb_ll_trap+0x40/0x50
>  [<ffffffff81002e12>] do_int3+0x52/0x130
>  [<ffffffff81674345>] int3+0x25/0x40
>  [<ffffffff810a2be2>] ? sysrq_handle_dbg+0x32/0x60
>  <<EOE>>  [<ffffffff813e1e69>] __handle_sysrq+0x129/0x190
>
>
> I think we need to actually empirically prove the code change is right or wrong before merging it, as well as cleaning up the change log slightly.
>
> I'll go hunt down the patch the fixes the oops first.
>
> Cheers,
> Jason.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/