Locking [LWN.net]

Locking

Posted Sep 24, 2024 19:44 UTC (Tue) by Cyberax (✭ supporter ✭, #52523)
In reply to: Locking by viro
Parent article: Resources for learning Rust for kernel development

> ... and it's a trouble waiting to happen. Look around and you'll find all kinds of bad ideas, most with exact same reason - hard to reason about the program state.

Why?

The reasoning here is simple: everything is locked after the scope guard is taken until the end of the scope. And you can't access the protected data accidentally without taking a lock. You also can't forget to clean up the lock in "goto cleanup". Early returns are also not a problem anymore.

It does reduce the flexibility a bit, but hand-over-hand locking is pretty rare.

to post comments

Locking

Posted Sep 24, 2024 20:02 UTC (Tue) by daroc (editor, #160859) [Link] (3 responses)

One also can drop the guard explicitly:

    drop(guard);

So I don't think it's less flexible, really. But, as in every discussion about programming languages, it's not really about what's possible so much as what the language makes easy or hard. I think it's a good point that Rust's locking is less explicit! That's certainly true, and it is a tradeoff. I (personally) think that the guarantees the compiler provides are worth it, but that doesn't mean we shouldn't acknowledge that less explicit locking does have downsides.

Forcing explicit destructuring

Posted Sep 24, 2024 20:19 UTC (Tue) by farnz (subscriber, #17727) [Link] (1 responses)

There is some thought being put into whether something like undroppable types would be a useful addition to the language. These would allow Rust for Linux to have a scope guard that must be explicitly destroyed (probably via a fn unlock(self) function), and where dropping them without calling that function is a compile-time error.

Forcing explicit destructuring

Posted Sep 24, 2024 20:21 UTC (Tue) by intelfx (subscriber, #130118) [Link]

> There is some thought being put into whether something like undroppable types would be a useful addition to the language

So… true linear types (as discussed right in this comment section)? :-)

Locking

Posted Sep 25, 2024 17:54 UTC (Wed) by NYKevin (subscriber, #129325) [Link]

One can argue that drop(guard) is just a funny way of spelling (the currently nonexistent) guard.unlock().

Locking

Posted Sep 24, 2024 20:49 UTC (Tue) by viro (subscriber, #7872) [Link] (1 responses)

... except that when you have several levels of nesting, finding the end of the right scope 80 lines down from where you've taken the sucker can be an interesting exercise. spin_unlock(...) is a lot more recognizable than one } in the series of 4.

BTW, do *NOT* mix that with goto-based cleanups without a lot of care. Any goto *into* that scope must come from that scope itself, which would be trivial if not for the fact that guard() can be not the first thing within the compound statement. In that case the scope extends from guard() to the end of compound statement. Now, it's very obvious that

if (condition) goto exit_something;
{
some_type var = foo();
....
exit_something:
something(var);
...
}
is a bug - no matter how liberal your interpretation of standard might be, on the path that takes this goto exit_something you have var used uninitialized. Anyone tempted to add such goto would see that this candidate failure exit is not suitable, no matter how similar it might be to what you want. And compiler will catch that if you try.

What's less obvious is that it applies to goto around the guard - there's no visible spin_unlock(...) in the end, but it is implicit and there's exact same kind of bug. Worse, gcc 12 and earlier does not even warn you - it goes ahead and produces broken code. clang does catch it properly, but neither gcc nor eyeball do.

So _if_ you use __cleanup-based cleanups, be very careful about labels in the scope - any goto from outside of scope to that will be trouble. With zero visible indication that such and such failure exit is *NOT* suitable to jump into from before the declaration that carries __cleanup on it. Gets especially nasty when the damn thing sits in the middle of function body, not nested at all. Do that and you've turned the goto-based cleanups you might have there into bugs.

It's not a theoretical concern - I've run into just that several time this cycle. Ended up with doing cross-builds on clang (and excluding the targets not supported by clang - thankfully, build coverage had not suffered in the places I needed to touch), but step into that while debugging something and forget about gcc missing such stuff and you are looking into a really fun debugging session...

In some cases it's a useful tool, but it's _really_ not something that could be used without (apriori non-obvious) care.

Locking

Posted Sep 24, 2024 21:04 UTC (Tue) by Cyberax (✭ supporter ✭, #52523) [Link]

> ... except that when you have several levels of nesting, finding the end of the right scope 80 lines down from where you've taken the sucker can be an interesting exercise. spin_unlock(...) is a lot more recognizable than one } in the series of 4.

Here's a thought: why does that matter? Is that because the scope is too large? Then it's probably a good idea to extract the locked code into its own scope. In my code, I also try to take the lock at the beginning of a scope, so the entire scope itself becomes a visual indicator of what's locked. The kernel code style with 8-space tabs makes this less practical in C, but Rust is different.

And if you want to re-lock the object, you need to look up for the locks that were just taken, instead of down to see the unlocks. It's a bit different perspective, but it's about the same level of mental overhead.

> What's less obvious is that it applies to goto around the guard - there's no visible spin_unlock(...) in the end, but it is implicit and there's exact same kind of bug.

Yes, the two styles are completely incompatible, and it's better not to mix them at all in one function (perhaps, a checkpatch.pl rule?). Good news is that Rust will not allow this kind of error.