Re: [PATCH v2] perf lock: add "info" subcommand for dumping miscinformation

[Hitoshi Mitake]

On 05/01/10 03:49, Frederic Weisbecker wrote:
> On Sat, Apr 24, 2010 at 07:46:41PM +0900, Hitoshi Mitake wrote:
>> Hi Frederic,
>>
>> I added "info" subcommand to perf lock,
>> this can be used as dumping metadata like thread or address of lock 
instances.
>> "map" was removed because info should do the work of it.
>>
>> This will be useful not only for debugging but also for ordinary 
analyzing.
>>
>> I made this patch on perf/core of your tree, could you queue this?
>>
>> v2: adding example of usage
>> % sudo ./perf lock info -t
>>   | Thread ID: comm
>>   | 	 0: swapper
>>   |         1: init
>>   |        18: migration/5
>>   |        29: events/2
>>   |        32: events/5
>>   |        33: events/6
>> ...
>>
>> % sudo ./perf lock info -m
>> | Address of instance: name of class
>> |  0xffff8800b95adae0:&(&sighand->siglock)->rlock
>> |  0xffff8800bbb41ae0:&(&sighand->siglock)->rlock
>> |  0xffff8800bf165ae0:&(&sighand->siglock)->rlock
>> |  0xffff8800b9576a98:&p->cred_guard_mutex
>> |  0xffff8800bb890a08:&(&p->alloc_lock)->rlock
>> |  0xffff8800b9522a08:&(&p->alloc_lock)->rlock
>> |  0xffff8800bb8aaa08:&(&p->alloc_lock)->rlock
>> |  0xffff8800bba72a08:&(&p->alloc_lock)->rlock
>> |  0xffff8800bf18ea08:&(&p->alloc_lock)->rlock
>> |  0xffff8800b8a0d8a0:&(&ip->i_lock)->mr_lock
>> |  0xffff88009bf818a0:&(&ip->i_lock)->mr_lock
>> |  0xffff88004c66b8a0:&(&ip->i_lock)->mr_lock
>> |  0xffff8800bb6478a0:&(shost->host_lock)->rlock
>> Signed-off-by: Hitoshi Mitake<mitake@xxxxxxxxxxxxxxxxxxxxx>
>> Cc: Ingo Molnar<mingo@xxxxxxx>
>> Cc: Peter Zijlstra<a.p.zijlstra@xxxxxxxxx>
>> Cc: Paul Mackerras<paulus@xxxxxxxxx>
>> Cc: Arnaldo Carvalho de Melo<acme@xxxxxxxxxx>
>> Cc: Jens Axboe<jens.axboe@xxxxxxxxxx>
>> Cc: Jason Baron<jbaron@xxxxxxxxxx>
>> Cc: Xiao Guangrong<xiaoguangrong@xxxxxxxxxxxxxx>
>
>
> I've eventually not queued it because of some various
> problems, see below:

Thanks for your advice, Frederic!

>
>
>
>> ---
>>   tools/perf/builtin-lock.c |   68 
+++++++++++++++++++++++++++++++++++++++++++--
>>   1 files changed, 65 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
>> index ce27675..c54211e 100644
>> --- a/tools/perf/builtin-lock.c
>> +++ b/tools/perf/builtin-lock.c
>> @@ -778,18 +778,61 @@ static void print_result(void)
>>   	}
>>   }
>>
>> +static int			info_threads;
>> +static int			info_map;
>> +
>> +static void rec_dump_threads(struct rb_node *node)
>> +{
>> +	struct thread_stat *st;
>> +	struct thread *t;
>> +
>> +	if (!node)
>> +		return;
>> +
>> +	if (node->rb_left)
>> +		rec_dump_threads(node->rb_left);
>
>
> That only walks the left nodes of the rbtree, imagine the following
> rbtree, W are visited nodes, U are the unvisited:
>
>                     Root
>                   /      \
>                  W        U
>                /  \      / \
>               W    U    U   U
>
>
> Better iterate using rb_first() then rb_next() until it is NULL.

Ah, yeah, recursive tracking is too primitive.
I rewrote it with rb_first() and rb_next().

>
>
>
>> +
>> +	st = container_of(node, struct thread_stat, rb);
>> +	BUG_ON(!st);
>
>
> You won't ever have !st because container_of computes an address
> based on a struct type and a contained address inside this struct.
>
> struct thread_stat {
> 	struct list_head	hash_entry;
> 	struct rb_node		rb;
> 	[...]
> } ts;
>
> If ts->rb == 1000, ts == 1000 - 16 or something like this.
>
> What matters is the "if (!node)" check you did before.

Yeah... my BUG_ON() checking was really silly and meaningless :(
I'll remove it.

>
>
>
>> +	t = perf_session__findnew(session, st->tid);
>> +	BUG_ON(!t);
>> +
>> +	printf("%10d: %s\n", st->tid, t->comm);
>
>
>
> Please don't use printf anymore (I did the same mistakes lately),
> now that are using a TUI and we might use a GUI one day, we
> can't assume anymore we are dealing with a normal stdout.
>
> So better use pr_debug, pr_err, pr_warning, etc...

OK. I'll replace printf() with pr_info() or pr_debug().

>
>
>
>> +
>> +	if (node->rb_right)
>> +		rec_dump_threads(node->rb_right);
>> +}
>> +
>> +static void dump_threads(void)
>> +{
>> +	printf("%10s: comm\n", "Thread ID");
>
>
>
> Same here and below.
>
>
>
>> +	rec_dump_threads(thread_stats.rb_node);
>> +}
>> +
>>   static void dump_map(void)
>>   {
>>   	unsigned int i;
>>   	struct lock_stat *st;
>>
>> +	printf("Address of instance: name of class\n");
>>   	for (i = 0; i<  LOCKHASH_SIZE; i++) {
>>   		list_for_each_entry(st,&lockhash_table[i], hash_entry) {
>> -			printf("%p: %s\n", st->addr, st->name);
>> +			printf(" %p: %s\n", st->addr, st->name);
>>   		}
>>   	}
>>   }
>>
>> +static void dump_info(void)
>> +{
>> +	/* ugly... */
>> +	if (info_threads)
>> +		dump_threads();
>
>
> No it's not ugly, it's ok, we do this everywhere in perf tools :)
>

Thanks :) I'll resend v3 patch later.

       Hitoshi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/