Locking
Locking
Posted Sep 24, 2024 6:58 UTC (Tue) by roc (subscriber, #30627)In reply to: Locking by viro
Parent article: Resources for learning Rust for kernel development
As noted in this article, the kernel is already using scoped guards in C. E.g.:
https://github.com/torvalds/linux/blob/abf2050f51fdca0fd1...
so it's too late to require explicit "unlock".
Posted Sep 24, 2024 17:09 UTC (Tue)
by viro (subscriber, #7872)
[Link] (10 responses)
As those things go, guard() is pretty high on the "easily turned into a landmine" scale. scoped_guard() is saner, but it comes with its own set of headache sources - deeply nested structure can get confusing.
It's not about preserving some kind of purity; there's no such thing in the real world anyway. Bitrot _is_ a fact of life; there's no One True Tool/Style/Language that would prevent it. What matters is how brittle the thing is. Another fact of life is that trying to figure out a bug elsewhere will lead you through a lot of unfamiliar code (possibly - yours, but with detailed memories of the area swapped out of active memory; when there's a dozen of areas you need to get through, the latency of mental context switch can be very painful); you will need to make decisions about the next thing to look into, and do that based on the reasoning about unfamiliar code. It _can't_ be avoided; what matters is how hard will it be. And yes, that includes the need to modify something you've written ten years ago. It happens. And it's all about tradeoffs.
As for the __cleanup-based tools... in moderation it's fine, but blind use can lead to a lot of PITA. In particular, it comes with serious, er, adverse interactions with other tools. We'll see how bad a source of bitrot it will be; for now I'm very cautious about the cases where accidentally delaying the cleanup can cause problems, and locking is firmly in that class.
Posted Sep 24, 2024 19:44 UTC (Tue)
by Cyberax (✭ supporter ✭, #52523)
[Link] (6 responses)
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.
Posted Sep 24, 2024 20:02 UTC (Tue)
by daroc (editor, #160859)
[Link] (3 responses)
One also can drop the guard explicitly:
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.
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.
Posted Sep 24, 2024 20:21 UTC (Tue)
by intelfx (subscriber, #130118)
[Link]
So… true linear types (as discussed right in this comment section)? :-)
Posted Sep 25, 2024 17:54 UTC (Wed)
by NYKevin (subscriber, #129325)
[Link]
Posted Sep 24, 2024 20:49 UTC (Tue)
by viro (subscriber, #7872)
[Link] (1 responses)
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;
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.
Posted Sep 24, 2024 21:04 UTC (Tue)
by Cyberax (✭ supporter ✭, #52523)
[Link]
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.
Posted Sep 24, 2024 20:53 UTC (Tue)
by roc (subscriber, #30627)
[Link] (2 responses)
Posted Sep 28, 2024 23:12 UTC (Sat)
by viro (subscriber, #7872)
[Link] (1 responses)
A nice bit of MBAese, that... Opposition between "advantages" on one hand and "your concerns" on another is particularly charming - the former being implicitly objective and unchanging, while the latter - subjective and theoretical in the best case. Use of passive-with-reference-to-management syntax is also impressive. Well done.
Back in the real world, cleanup.h contains tools that are
The tools in question have nothing to do with Rust, so their relevance in this thread is, indeed, questionable. Who'd dragged them in, I wonder... <checks> https://lwn.net/Articles/991431/ is where that had happened, so it would be yourself, wouldn't it?
Posted Sep 29, 2024 7:22 UTC (Sun)
by Cyberax (✭ supporter ✭, #52523)
[Link]
Deferred cleanup has been used in C++ and Go for _decades_ and is very well understood. The main rule is: do NOT mix it with goto-based control. It can even be automated via a simple checkpatch.pl rule.
In that case, the of_node_put should also be rewritten into the guard-style cleanup.
Locking
Locking
Locking
drop(guard);
Forcing explicit destructuring
Forcing explicit destructuring
Locking
Locking
{
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.
Locking
Locking
Locking
1) potentially useful
2) experimental and not well-understood
3) demonstrably dangerous in incautious use - and that's demonstrably, not theoretically. Fresh example: https://lore.kernel.org/lkml/20240927-reset-guard-v1-1-29...
Locking