Implement for mutex(9) and rwlock(9).
Details
- Reviewers
kib jtl - Group Reviewers
transport - Commits
- rG656991b0c629: locks: augment lock_class with lc_trylock method
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are also RM and lockmgr lock classes. Even if you do not need them, I do not see how is it fine to leave them from the proper implementation of the new lock class method.
This is what lockmgr has for normal lock/unlock:
static void lock_lockmgr(struct lock_object *lock, uintptr_t how) { panic("lockmgr locks do not support sleep interlocking"); } static uintptr_t unlock_lockmgr(struct lock_object *lock) { panic("lockmgr locks do not support sleep interlocking"); }
I really don't see a value into adding this extra kernel text. If somebody write a code that tries lc_lock (or the new lc_trylock) on the lockmgr class, they will get a panic. It will panic regardless of we added this extra test to the kernel or not. Note that it cannot panic in run time in principle, only at development time.
The rmlock(9) also doesn't have try-wlock operation, so the "proper implementation" would be again just panic.
Panics with the call of NULL pointer are confusing, usually the last frame (before the call) is lost. I believe this is why lockmgr has real methods with panics instead of NULLs.
I do think that consistency and debugging convenience are good to have, and cost of adding yet another method is negligible. It's body could become
KASSERT(0, ("XXX")); __unreachable();
and other methods could be converted to that instead of panic() call.
Would that produce a zero text for a non-INVARIANT kernel?
I know that call NULL functions are hard to debug, so for a code that has a potential of such call to happen at runtime, I would add an extra function. But this one can panic only when a developer tries to create something new. In that case the developer should quickly understand what's going on.