Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 21 Apr 2010 18:12:06 +0900 | From | Hitoshi Mitake <> | Subject | Re: [PATCH] perf lock: Fix state machine to recognize lock sequence |
| |
On 04/21/10 10:26, Frederic Weisbecker wrote: > On Fri, Apr 16, 2010 at 05:44:06PM +0900, Hitoshi Mitake wrote: >> Hi Ingo, >> >> I'm developing the model to recognize the correct sequence of lock events. >> Previous state machine of perf lock was really broken. >> This patch improves it a little. >> >> This patch prepares the array of state machine represents lock sequence for each threads. >> These state machines represent one of these sequence: >> >> 1) acquire -> acquired -> release >> 2) acquire -> contended -> acquired -> release >> 3) acquire (w/ try) -> release >> 4) acquire (w/ read) -> release >> >> The case of 4) is a little special. >> Double acquire of read lock is allowed, so state machine of sequence >> counts read lock number, and permit double acquire and release. >> >> But, things are not so simple. Something of my model is still wrong. >> I counted the number of lock instances with bad sequence, >> and ratio is like this (case of tracing whoami): bad:122, total:1956 > > > > I just gave your patch a try and it's worse: almost every sequences > were reported bad (it wasn't working either before your patch :) > > This is not the fault of your patch though. Actually your patch seems to > be a nice improvement.
Thanks for your review, Frederic!
> > In fact I just found two things: > > 1) We are working on tasks in pid basis. We should work on a task by using > its tid. > In fact we are processing the sequences of several threads in a process as > if we were dealing with a single task. > > If A and B are two threads belonging to a same process, and if we have: > > A: acquire lock 1, release lock 1 > B: acquire lock 2, release lock 2 > > ...then we are dealing with a random mess of sequences: > > AB: acquire lock 1, acquire lock 2, release lock 1, and any kind of random > things like this.
Ah, I missed tid. I'll fix this point.
> > 2) I can't get lock_acquired traces. Not sure why yet...
Really? It's mystery... I'll seek the cause.
> > >> >> There is another new bad thing. >> The size of array of state machine is equal to max depth lockdep defines. >> If perf lock record tries to record lock events of the programs with lots of >> system call like "perf bench sched messaging", the array will be exhausted :( > > > > Yeah, I suggest you use a list for that in fact. The max lockdep depth may > change in the future, or become variable, so we can't relay on that.
Yeah, I'll use list or hashtable.
> > But that's still a cool improvement. > > I'm queuing this patch.
Thanks! But I have to fix some points based on your advice. Should I send v2 patch or make fix on your tree?
Thanks, Hitoshi
| ![\](/images/icornerr.gif) |