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

[Frederic Weisbecker <fweisbec@gmail.com>]

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@dcl.info.waseda.ac.jp>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Jason Baron <jbaron@redhat.com>
> Cc: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>


I've eventually not queued it because of some various
problems, see below:



> ---
>  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.



> +
> +	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.



> +	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...



> +
> +	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.