by Frederic Weisbecker

[permalink] [raw]

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 <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Jason Baron <[email protected]>
> Cc: Xiao Guangrong <[email protected]>

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.