some lockstat improvements

[avg]

Summary

1. lockstat: teach about freebsd sx and rw locks

Is it worthwhile preserving original solaris/illumos code under ifdef?

2. dtrace lockstat: use time as measure of spinning

Previously a count of spin loops was used. That was cheap to measure.
But as a result we compared apples or oranges when analyzing adaptive
locks.

3. make rw-block probes closer to upstream

So that lockstat(1) could correctly detect various rwlock event.
Apply the same treatment to sx probes.
Note that -acquire and -release event do not pass reader / writer argument at the moment.

4. fire lock acquisition probes in successful "try lock" operations of rwlock and sx

[markj]

cddl/contrib/opensolaris/cmd/lockstat/lockstat.c

153 ↗

(On Diff #5918)

I fixed this in D2642 by modifying the rwlock probes to pass the correct arguments as documented in Solaris' lockstat provider page: https://wikis.oracle.com/display/DTrace/lockstat+Provider

Do you have a preference for either approach?

sys/kern/kern_mutex.c

544 ↗

(On Diff #5918)

I think you can get rid of spin_cnt and sleep_cnt entirely here: spin_cnt > sleep_cnt is always true at this point since spin_cnt is incremented at least once per loop iteration, and sleep_cnt is incremented at most once per loop iteration.

sys/kern/kern_rwlock.c

564 ↗

(On Diff #5918)

Same as above - I believe spin_cnt and sleep_cnt can be removed.

[avg]

cddl/contrib/opensolaris/cmd/lockstat/lockstat.c

153 ↗

(On Diff #5918)

I like your approach better.
So, it seems that with your change we conform to the Solaris documentation except for rw-spin extension?

Ah, another thing is that we may report both the spinning and sleeping events for adaptive locks while on Solaris it's either-or.
I guess this results from different implementations of adaptive behavior.

BTW, it would probably make sense for rw-spin to provide exactly the same arguments as rw-block?

sys/kern/kern_mutex.c

544 ↗

(On Diff #5918)

Can't there be a case where spin_cnt == sleep_cnt ?
In that case we would fire the spin event unnecessarily.

sys/kern/kern_rwlock.c

564 ↗

(On Diff #5918)

ditto :-)

[markj]

cddl/contrib/opensolaris/cmd/lockstat/lockstat.c

153 ↗

(On Diff #5918)

Yup, I believe so. My plan is to next write a lockstat provider man page so that these differences can be documented (as was done with some other providers).

I think it makes sense to reuse rw-block's arguments; the one difference is that args[1] should be the time spent spinning rather than the time spent blocked.

sys/kern/kern_mutex.c

544 ↗

(On Diff #5918)

Oh, oops. :(
I somehow convinced myself that that was ok (i.e. firing the spin event with spin_cnt == sleep_cnt), since lockstat(1)'s event predicates would filter them out, but that's not true - in fact there is no predicate on the mtx spin event.

I made this same mistake in my lockstat code review, I'll fix it in the next upload.

[avg]

• lockstat: make rw-block probes closer to upstream
• lockstat: fire block / spin probes before acquire
• lockstat: record lock acquisitions in successful "try lock" operations

[markj]

cddl/contrib/opensolaris/cmd/lockstat/lockstat.c

175 ↗

(On Diff #6070)

This should be lockstat:::sx-spin?

199 ↗

(On Diff #6070)

These probes should also use arguments to determine whether a reader or writer is taking or releasing the lock.

sys/kern/kern_sx.c

987 ↗

(On Diff #6070)

Shouldn't this be LOCKSTAT_READER?

990 ↗

(On Diff #6070)

I don't think the parens are needed here.

[avg]

• lockstat: fixup a typo in SX shared spin probe specification
• _sx_slock_hard: fixup copy-paste error, LOCKSTAT_READER should be used
• remove redundant parentheses when reporting spin times for lockstat

cddl/contrib/opensolaris/cmd/lockstat/lockstat.c

175 ↗

(On Diff #6070)

Yes!

199 ↗

(On Diff #6070)

Yes, but I was too lazy to transplant that part of your change into this change.
I still can do that if you think that would be better. What do you think?

sys/kern/kern_sx.c

987 ↗

(On Diff #6070)

Yes, copy-paste error.

990 ↗

(On Diff #6070)

absolutely

[markj]

cddl/contrib/opensolaris/cmd/lockstat/lockstat.c

199–200 ↗

(On Diff #6073)

Sure, I can handle that.