Page MenuHomeFreeBSD

locks: augment lock_class with lc_trylock method
AcceptedPublic

Authored by glebius on Wed, Jun 26, 3:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 29, 12:22 AM
Unknown Object (File)
Fri, Jun 28, 10:56 PM
Unknown Object (File)
Thu, Jun 27, 3:41 PM
Subscribers

Details

Reviewers
kib
jtl
Group Reviewers
transport
Summary

Implement for mutex(9) and rwlock(9).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 58360
Build 55248: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Wed, Jun 26, 5:08 PM

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.

In D45745#1043581, @kib wrote:

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.

In D45745#1043856, @kib wrote:

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.