This is modelled after the existing rwlock and sx probes. I also added
a lockmgr-disown probe.
Update lockstat(1) to report on the new probes. Fix a bug in the hold
events for sx locks.
Details
Details
- Reviewers
mjg gnn - Group Reviewers
manpages - Commits
- rS351361: Add lockmgr(9) probes to the lockstat DTrace provider.
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 25994 Build 24547: arc lint + arc unit
Event Timeline
Comment Actions
The timing is a little funny as I'm working in the area and was considering adding these probes myself. I think the patch is fine with nits below taken care of.
Note supposed fast path is now extra pessimized with presence of these probes (i.e. they get evaled after the lock is taken each time, for other routines lockstat enabled brings them to the slowpath first). I'll take care of this later.
| sys/kern/kern_lock.c | ||
|---|---|---|
| 672 | This can and should be prefixed with LOCKSTAT_PROFILE_ENABLED(lockmgr__block) check. Reasoning is that error is almost guaranteed to be 0 and since right now the lock always blocks on contention sleep_time will not be 0, which means there are 2 branches to evaluate in the common case. If you prefix it you can skip one branch for the common case. This can be further fixed up but should be fine enough for now. I have cleanups pending which will reduce the surrounding branchfest. | |
| 863 | see the previous comment | |
| 1059 | see the previous comment | |
Comment Actions
I considered avoiding the fast path when probes are enabled, but didn't because the fast path is not inlined and the lockmgr_note_*() routines were too convenient.
| sys/kern/kern_lock.c | ||
|---|---|---|
| 672 | I don't quite understand the claim about sleep_time. lockstat_nsecs() will return 0 unless some lockstat probes are enabled, so sleep_time should be 0 in the common case. | |
| 1059 | That would disable lock_profiling too. Are you suggesting: if (tid != LK_KERNPROC) {
if (LOCKSTAT_PROFILE_ENABLED(lockmgr__release))
LOCKSTAT_RECORD1(...);
lock_profile_release_lock(...);
}? | |
Comment Actions
I undestand, it's why I noted it's fine but will have to be taken care of later (which I'll do while refactoring this code).
With the other remarks in place I think it's fine.
| sys/kern/kern_lock.c | ||
|---|---|---|
| 672 | Correct, that's my bad. I had a patch which always returned the time and went on autopilot here. | |
| 1059 | Oh sorry, I forgot for /these/ probes both LOCK_PROFILING and lockstat are mixed in. Since tid vast majority of the time is not LK_KERNPROC, I wanted: if (LOCKSTAT_PROFILE_ENABLED(lockmgr__release)) {
if (tid != LK_KERNPROC) { ...
}
}to normally have only branch. However, the macro situation would require too much ugliness to be worth it as it is. | |