Page MenuHomeFreeBSD

Reimplement the lockstat provider using SDT(9).
AbandonedPublic

Authored by markj on May 25 2015, 3:12 AM.

Details

Reviewers
gnn
Group Reviewers
DTrace
Summary

Prior to this change the lockstat provider created using a custom
provider. This is done in illumos because lockstat uses some special
hot-patching techniques for probe sites, but that doesn't apply in
FreeBSD, so there's no reason to use a separate provider implementation.

Reimplementing the lockstat probes in terms of SDT(9) provides the
following benefits:

  • less code
  • argument type info: currently the lockstat probes carry no type info
  • improved performance when we switch to hot-patching SDT probe sites (currently a WIP)

While here, I've fixed a few bugs and nits at the probe sites, mostly
based on the upstream documentation:
https://wikis.oracle.com/display/DTrace/lockstat+Provider

In particular, this change

  • removes some unnecessary #ifdef KDTRACE_HOOKS usages: the lockstat macros expand to nothing if KDTRACE_HOOKS is not defined.
  • removes some unnecessary "if (spin_cnt > sleep_cnt)" conditionals: this condition is always true since spin_cnt is incremented at least once per loop body, while sleep_cnt is incremented once every time the thread blocks on a turnstile or enter a sleepqueue.
  • removes some unneeded parentheses around probe arguments.
  • ensures that a "block" probe fires before the corresponding "acquire" probe, as explicitly stated in the docs.
  • adds extra probe arguments to the rwlock block probe as documented above. Specifically, we define constants, LOCKSTAT_READER and LOCKSTAT_WRITER which correspond to the RW_{READER,WRITER} constants in the docs; the values are chosen based on the lockstat(1) code. These are used as probe arguments to indicate whether a read or write lock is being taken. Other probe arguments specify whether a writer held the lock when the current thread blocked, and the number of readers at the time the current thread blocked.
Test Plan

Manual testing for sane rwlock argument values, lockstat(1).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

markj retitled this revision from to Reimplement the lockstat provider using SDT(9)..
markj edited the test plan for this revision. (Show Details)
markj updated this object.
markj edited edge metadata.
markj added a reviewer: DTrace.

Mark, these are rather significant changes. I wonder if there is some easy way to integrate them with what I've just submitted in D2727 ?
Or should I just wait unil you commit this change and then rebase my change?

In D2642#51638, @avg wrote:

Mark, these are rather significant changes. I wonder if there is some easy way to integrate them with what I've just submitted in D2727 ?
Or should I just wait unil you commit this change and then rebase my change?

Your change is quite a bit smaller and mostly orthogonal to mine, so I'm happy to rebase this onto your change once it's committed.

sys/kern/kern_mutex.c
534

adaptive-block and adaptive-spin should probably fire before adaptive-acquire if we are to follow the Solaris specification in a stricter fashion.
However, unlike Solaris we have to allow both adaptive-block and adaptive-spin for the same lock acquisition event.

sys/kern/kern_rwlock.c
559

Although Solaris does not have rw-spin, we should probably put it before rw-acquire.

sys/kern/kern_rwlock.c
559

it would probably make sense to pass excatly the same arguments to rw-spin as are passed to rw-block if possible (possibly modulo counts vs nsecs)

sys/kern/kern_rwlock.c
531

Perhaps it would make sense to set this vaue ("last") to rw->rw_lock before the outmost loop is entered, so that the value reflects the original state of the lock? It certailny would help with arguments to rw-spin if we decide to supply them.

Outside the scope of this change, but should we also fire -acquire probes for trylock operations if they are successful?
I think that we should.

sys/kern/kern_mutex.c
534

Will fix, thanks.

sys/kern/kern_rwlock.c
531

That makes sense to me.

559

Ok, I'll fix both of these issues.

I haven't reviewed the patch, but the hot patching was a nice idea. However, if it hasn't been implemented until now...

In D2642#53371, @rpaulo wrote:

I haven't reviewed the patch, but the hot patching was a nice idea. However, if it hasn't been implemented until now...

It depends on the lock entry points being written a certain way. See for example: https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/intel/ia32/ml/lock_prim.s#L1063

The plan is to implement hot-patching for SDT soon. This therefore won't be as specialized as illumos' lockstat implementation, but I think it'll be good enough.

I've put up D2993-2995 with similar changes, rebased onto avg's recent lockstat work.