Page MenuHomeFreeBSD

locks: augment lock_class with lc_trylock method
ClosedPublic

Authored by glebius on Jun 26 2024, 3:56 PM.
Tags
None
Referenced Files
F102059115: D45745.diff
Thu, Nov 7, 2:50 AM
F102020560: D45745.diff
Wed, Nov 6, 3:28 PM
Unknown Object (File)
Thu, Oct 24, 5:21 PM
Unknown Object (File)
Thu, Oct 17, 12:53 AM
Unknown Object (File)
Tue, Oct 15, 4:03 AM
Unknown Object (File)
Sun, Oct 13, 1:02 PM
Unknown Object (File)
Sat, Oct 12, 11:45 PM
Unknown Object (File)
Sat, Oct 12, 7:44 AM
Subscribers

Details

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.Jun 26 2024, 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.