Page MenuHomeFreeBSD

Add lockstat probes for lockmgr(9).
ClosedPublic

Authored by markj on Aug 21 2019, 8:29 PM.

Details

Summary

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.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 25994
Build 24547: arc lint + arc unit

Event Timeline

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

In D21355#464584, @mjg wrote:

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.

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(...);
}

?

In D21355#464584, @mjg wrote:

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.

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.

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.

This revision is now accepted and ready to land.Aug 21 2019, 10:38 PM