Page MenuHomeFreeBSD

some lockstat improvements
ClosedPublic

Authored by avg on Jun 3 2015, 5:01 PM.

Details

Reviewers
markj
Group Reviewers
DTrace
Commits
rS284297: several lockstat improvements
Summary
  1. lockstat: teach about freebsd sx and rw locks

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

  1. 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.

  1. 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.

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

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

avg retitled this revision from to some lockstat improvements.
avg updated this object.
avg edited the test plan for this revision. (Show Details)
avg added a reviewer: markj.
cddl/contrib/opensolaris/cmd/lockstat/lockstat.c
159

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

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
569–570

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

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

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

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
569–570

ditto :-)

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

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

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.

  • lockstat: make rw-block probes closer to upstream
  • lockstat: fire block / spin probes before acquire
  • lockstat: record lock acquisitions in successful "try lock" operations
avg updated this object.
cddl/contrib/opensolaris/cmd/lockstat/lockstat.c
174

This should be lockstat:::sx-spin?

199–200

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

sys/kern/kern_sx.c
986

Shouldn't this be LOCKSTAT_READER?

990–992

I don't think the parens are needed here.

avg updated this object.
  • 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
174

Yes!

199–200

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
986

Yes, copy-paste error.

990–992

absolutely

markj edited edge metadata.
markj added inline comments.
cddl/contrib/opensolaris/cmd/lockstat/lockstat.c
199–200

Sure, I can handle that.

This revision is now accepted and ready to land.Jun 10 2015, 1:28 PM
This revision was automatically updated to reflect the committed changes.
avg marked 6 inline comments as done.