Re: [PATCH] Add type of locks to lock trace events

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Jan 28, 2010 at 11:21:53AM +0100, Peter Zijlstra wrote:
> On Tue, 2010-01-26 at 14:56 +0900, Hitoshi Mitake wrote:
> > # Sorry, I wrote wrong Cc address. Previous mail was rejected by mailer-daemon.
> > # This is second time sending, if you already received this, please discard it...
> > 
> > There's no need to add any member to lockdep_map
> > for adding information of type of locks to lock trace events.
> > 
> > Example of perf trace:
> > |       init-0     [001]   335.078670: lock_acquired: 0xffff8800059d6bd8 &rq->lock kernel/lockdep.c:2973 (0 ns)
> > |       rb_consumer-424   [001]   335.078673: lock_acquire: 0xffff8800059d6bd8 1 &rq->lock kernel/lockdep.c:2973
> > |                                                                            # ^ &rq->lock is spin lock!
> > |       rb_consumer-424   [001]   335.078677: lock_acquire: 0xffff8800bba5e8e8 1 buffer->reader_lock_key kernel/trace/ring_
> > |       rb_consumer-424   [001]   335.078679: lock_acquired: 0xffff8800bba5e8e8 buffer->reader_lock_key kernel/trace/ring_b
> > |       rb_consumer-424   [001]   335.078684: lock_acquire: 0xffff8800059d12e8 1 &q->lock kernel/smp.c:83
> > 
> > Of course, as you told, type of lock dealing with is clear for human.
> > But it is not clear for programs like perf lock.
> > 
> > What I want to do is limiting types of lock focus on.
> > e.g. perf lock prof --type spin,rwlock
> > 
> > How do you think, Peter?
> 
> I still don't see the use for it, surely you're going to be familiar
> with the code if you're looking at lock statistics?



Comparing the avg/max time locks are acquired/waited between same
types of locks is interesting, but doing so between spinlocks and mutexes
is definetly pointless.

spinlocks will always be in the bottom of the report while their measures
should be taken in an utterly different order of magnitude.

Each lock nature should be reported separately. An rwlock won't have
the same behaviour pattern than a spinlock. It may spin less in average
but more in the maximum snapshot, and still that depends on a write_lock
or a read_lock operations.

That said I don't think this is the right approach. We only need this
information when the lock is initialized. That's the same for the
file and line things, especially since these add 1/5 more overhead.
And that's even the same for the name of the lock.

Actually we need a lock_init event that brings:

- lock addr
- lock name
- lock type
- file/line

This can be triggered on spinlock_init(), mutex_init(), etc..
for the dynamic cases and use a synthetize_lock_events()
from perf tools that could work toward events injection
through perf ioctl, for the static cases.
Or/and we could use a debugfs file for that, otherwise only
perf will be able to use the lock events. Or this can be
an event injection from ftrace.

And all other lock events should not be giving anymore the
name, type, or file/line. We should deduce it from the
lock_init events and the lock addr in each lock events.

I think this becomes critical, given the high overhead nature
of lock events and their impact in profiling them.