Locking
Locking
Posted Sep 23, 2024 17:35 UTC (Mon) by tialaramex (subscriber, #21167)Parent article: Resources for learning Rust for kernel development
The way you'd do that is your unlock function takes the guard as a parameter. Since Rust has the destructive move semantic, the unlock doesn't need to actually "do" anything, it can return immediately - the guard was moved into the function and then not returned, it's gone - it was dropped, which gives effect to the programmer's intent.
Posted Sep 23, 2024 18:45 UTC (Mon)
by intelfx (subscriber, #130118)
[Link] (19 responses)
And then you will have half of the code using this no-op `.unlock()` and the other half of the code relying on the implicit Drop of the guard in the function scope itself. Sounds like a recipe for "disaster" levels of inconsistency.
You can, of course, mandate one way or the other via a code style document and/or linters. But that would add friction and strike another blow to the "it compiles, ergo it works" ideal.
Posted Sep 24, 2024 2:56 UTC (Tue)
by viro (subscriber, #7872)
[Link] (18 responses)
Sure, you don't lock to protect the code, or even the data - you lock to protect the invariants. But code is what you change, so unless one's answer to everything (including debugging) is "throw it all away and rewrite from scratch", you do need to be able to reason about the locking state at given spot in the code...
Posted Sep 24, 2024 6:58 UTC (Tue)
by roc (subscriber, #30627)
[Link] (11 responses)
As noted in this article, the kernel is already using scoped guards in C. E.g.:
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.
Posted Sep 24, 2024 11:55 UTC (Tue)
by daroc (editor, #160859)
[Link] (4 responses)
Keep an eye out for an article about that in the coming weeks.
Posted Sep 24, 2024 17:21 UTC (Tue)
by viro (subscriber, #7872)
[Link] (3 responses)
Posted Sep 24, 2024 17:31 UTC (Tue)
by daroc (editor, #160859)
[Link]
The parent-before-child order is actually fairly easy to enforce in Rust without using the technique from the talk (which was focused on imposing a global order). For the tree case, imagine a node looks like this:
By putting all the data that should only be accessed with a Mutex held inside an inner structure like that, we can enforce that the program can only have references to it that live as long as the lock is held. So if I had a reference to a Node, I couldn't access the children until I locked the Mutex. And then I could only hold on for references to them until I unlocked the Mutex, at which point the compiler would no longer let me use them.
Posted Sep 24, 2024 22:03 UTC (Tue)
by NYKevin (subscriber, #129325)
[Link] (1 responses)
The more difficult problem is preventing deadlock (which has never been an explicit goal of Rust's safety guarantees). Here's a simple way to at least prevent a lock from being recursively acquired:
* Invent a move-only wrapper (i.e. a type that is not Copy or Clone) for &Mutex. Each thread has this wrapper instead of the actual &Mutex, and the &Mutex is private so you can't directly get it out again.
But this is unideal for at least two reasons:
1. The whole point of Mutexes is to allow shared access to mutable data. A move-only wrapper is just undoing all of that hard work. Yes, it's per-thread, but that's still painful.
So I thought some more, and came up with a more elaborate idea, which works as follows:
* Each distinct lock in the lock hierarchy gets its own &Mutex wrapper (but those wrappers are Clone and Copy, so you can do all the same things you can do with &Mutex now). You can maybe use macros to make the wrappers less tedious to write.
I *think* this comprehensively prohibits any lock inversions from happening, statically, with zero overhead, provided that all mutexes in the system participate and your hierarchy really is acyclic. I also think you can probably use dyn as an "escape hatch" to check lock ordering at runtime instead of compile time, if it turns out that it's too hard to prove that some code is sound.
But I have not actually tried to implement this, so there might be some problem with it that I cannot see. For example, I'm not sure that Rust's trait solver is smart enough to materialize the whole DAG of an arbitrary lock hierarchy, so you might need to use macros for the hierarchy traits as well as the wrappers. Or you might tell everyone to manually write out the individual hierarchy traits they need, and check for cycles with some sort of linter or static analysis tool.
Posted Sep 24, 2024 22:34 UTC (Tue)
by NYKevin (subscriber, #129325)
[Link]
https://docs.rs/ordered-locks/latest/ordered_locks/
So yes, this is a thing you can do. Whether it is entirely feasible for very large and complicated lock hierarchies is still unclear to me.
Posted Sep 24, 2024 14:01 UTC (Tue)
by ehiggs (subscriber, #90713)
[Link]
> How would the compiler verify that a call you've added in the area under mutex will not lead to the same mutex being grabbed?
In some cases, tools can find this. For example in this Playground link I lock a mutex twice. The compiler will build and run the executable but deadlocks. If we use Tools->Miri, the issue is found. But I think this is found by running the program in an interpreter rather than detecting it at compile time.
https://play.rust-lang.org/?version=stable&mode=debug...
For more elaborate issues, the language will make the types messier and messier to work with as you venture further out of bounds.
For example if you have a `my_lock = std::sync::Mutex<MyData>` then getting access to `MyData` involves calling `my_lock.lock()` which gives a `MutexGuard<MyData>`. You can't pass a `MutexGuard<MyData>` to a function expecting `&MyData` so you need to try to get the reference to `&*MyData` - a reference to a dereference. Already, this is looking a bit weird and hopefully helps a reviewer. But you also have the MyData reference and pass that around.
While I C you might think 'how can I get access to the pointer and make sure it's locked', in the Rust case if you can find access to the reference then you know it's locked - otherwise it would have still been behind the Mutex wrapper.
Posted Sep 23, 2024 19:02 UTC (Mon)
by atnot (subscriber, #124910)
[Link] (6 responses)
And as a bonus, people can have their lock functions in a useful way too if they really want.
[1] for the unitiated: affine: at most once (Rust has this), linear: exactly once. I don't know why the math people use those words.
Posted Sep 23, 2024 19:51 UTC (Mon)
by NYKevin (subscriber, #129325)
[Link] (3 responses)
You can steal the API from std::thread::scope(), but I'm not sure how applicable that is in all cases. The basic idea is as follows:
* The caller passes in a closure, which takes some opaque scope object by reference.
This is obviously not as flexible as true linear types would be, but it is better than nothing.
Posted Oct 2, 2024 10:45 UTC (Wed)
by taladar (subscriber, #68407)
[Link] (2 responses)
Posted Oct 2, 2024 12:56 UTC (Wed)
by excors (subscriber, #95769)
[Link] (1 responses)
For example, mutexes could be designed like:
let mut m = ScopedMutex::new(42);
where with_lock guarantees it will always lock and unlock correctly around the accesses (unless the closure never returns).
The issue is, it would be nice to get similar guarantees *without* the closure, because closures create a potentially-inconvenient nested code structure and can introduce some extra lifetime challenges. Usually that's fine, but sometimes it's rather annoying. Better to have an API like:
let mut m = Mutex::new(42);
Currently the forget() is allowed and the mutex will never get unlocked, and some unrelated code may deadlock later (which is much harder to debug). It also means the Mutex might be dropped while it is still locked, so Mutex has to know how to clean up from that unusual state. Prohibiting the forget() (and any other code with similarly forgetful behaviour) will require new Rust language features.
(I think that's an easy trap to fall into when designing APIs: you design a MutexGuard whose lifetime is less than Mutex's lifetime, so you know the MutexGuard cannot be dropped after the Mutex is dropped (which is true, and a very helpful guarantee), and intuitively that means the MutexGuard will be dropped before the Mutex is dropped. And that's almost always true, but it's not guaranteed - the MutexGuard might have been forgotten instead of dropped - so you have to either handle that correctly (which can be difficult), or switch to a closure-based API.)
Posted Oct 2, 2024 13:44 UTC (Wed)
by farnz (subscriber, #17727)
[Link]
Note when doing that sort of analysis that you also have to take into account std::mem::swap and friends. An exclusive reference to a type I cannot construct works fine for what you describe, but once you allow a user-chosen type into the closure parameters, you have to consider what happens if I swap it out for another one I have lying around. I think that for the case of a single layer of locking, it's fine, but it becomes more exciting once you consider the case of nested locks, and my brain isn't up to looking for obscure corner cases in that situation.
Posted Sep 23, 2024 20:45 UTC (Mon)
by aszs (subscriber, #50252)
[Link] (1 responses)
The terms "linear types" and "affine types" comes from substructural logic theory which in turn borrows those terms from linear and affine equations in pure math theory.
The connection is this:
linear equations are of the form 𝑓(𝑥)=𝑎𝑥 and affine are 𝑓(𝑥)=𝑎𝑥+𝑏, where 𝑎 and 𝑏 are arbitrary constants.
Linear: f(x) = a*x, (x^1 as opposed to x^2...) "use x once"
perfectly obvious!
Posted Sep 24, 2024 6:53 UTC (Tue)
by vasvir (subscriber, #92389)
[Link]
Posted Oct 5, 2024 14:37 UTC (Sat)
by slanterns (guest, #173849)
[Link]
https://github.com/rust-lang/rust/issues/81872#issuecomme...
It was once added to the standard library as an experiment, and later withdrawn since the libs-api team thought it's better to just use `drop`.
Locking
>
> The way you'd do that is your unlock function takes the guard as a parameter. Since Rust has the destructive move semantic, the unlock doesn't need to actually "do" anything, it can return immediately - the guard was moved into the function and then not returned, it's gone - it was dropped, which gives effect to the programmer's intent.
Locking
Locking
https://github.com/torvalds/linux/blob/abf2050f51fdca0fd1...
so it's too late to require explicit "unlock".
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
Locking
Locking
Locking
struct Node {
some_unprotected_data: usize,
protected_data: Mutex<NodeInner>,
}
struct NodeInner {
more_protected_data: usize,
children: Vec<Node>,
}
Locking
* The wrapper has methods to lock the underlying Mutex as usual, but those methods borrow the wrapper mutably to ensure they cannot be called more than once.
* The wrapper is also !Send (otherwise, one thread could end up owning two of them and break the rules).
2. It does not solve the broader lock hierarchy problem.
* Each wrapper impls a generic trait that indicates its relationship with other locks in the hierarchy. For example, impl LockAfter<A> for B{} would mean "you may lock B after you have locked A."
* There's an empty struct (actually it has a PhantomData, but it's "empty" in the sense that it takes up zero bytes and gets constant-folded out of existence) which can only be constructed by these wrappers, or by some private initialization function that is callable from whatever "management" or runtime code is directly responsible for spawning threads. The struct has a generic type parameter which can either be one of the wrapper types, or it can be (). It also has a lifetime parameter, which it may not outlive (as constrained with PhantomData) To avoid having to write "the struct" over and over again, let's just say this struct is called LockHierarchy<'a, T>.
* LockHierarchy is not Clone. You have to pass it by value everywhere (but at runtime, it vanishes, so you can pass it around as much as you want without incurring any overhead).
* LockHierarchy is also !Send, so that no thread can ever own more than one.
* When a thread is created, the caller gives it a LockHierarchy<'static, ()> instance (created using the private initialization function). This construction must be done in the callee thread's startup code and not on the main thread, because it is !Send and can't be transferred, but this is not really much of a problem since the thread-spawning code can simply arrange for the thread to construct this object before it calls into the application code.
* Each wrapper takes a &'b mut LockHierarchy<'a, T> argument to lock(), and returns both a lock guard and a new LockHierarchy<'b, U> instance, where U is the type of the wrapper, T is trait-constrained to come before U in the lock hierarchy (or T = () is allowed unconditionally), 'b is the lifetime parameter of the lock guard, and 'a must outlive 'b. The latter constraint does not need to be spelled out, since it is implied by the validity of the type &'b mut LockHierarchy<'a, T>.
* At any given time, each thread should have exactly one LockHierarchy object (which is not borrowed mutably), that object is required in order to take any locks, and the type of the LockHierarchy object determines which locks you are still allowed to acquire. Each LockHierarchy's lifetime parameter stops it from outliving its associated lock guard.
* A thread can e.g. std::mem::forget() the LockHierarchy, or drop it, but oh well, that just means you can't lock anything. It doesn't cause unsafety or other problems, because LockHierarchy is just a trivial opaque object with no internal machinery.
Locking
https://docs.rs/lock_ordering/latest/lock_ordering/
Locking
Locking
[2] https://blog.ffwll.ch/2022/07/locking-engineering.html
Locking
* The scope object provides methods for the closure to do whatever thing(s) the caller wants to do (in the case of thread::scope(), spawn threads).
* The scope object internally keeps track of whatever thing(s) the closure does, and whatever cleanup operation(s) may be required in response (in the case of thread::scope(), join those threads to ensure they do not outlive the scope).
* Because the closure is called from the callee, the callee always gets control back after the closure returns (or panics, if unwinding is enabled and we use catch_unwind). The callee can ensure that all cleanup code is executed, by referencing the scope object's internal data. Since the closure does not receive ownership of the scope object, it cannot forget it.
* In the case of thread::scope(), the scope object also gives the closure "handles" which it can use to reference the operations (threads) it has performed (spawned). But if the closure forgets these handles, it has no effect on the scope object, because they're just handles - they do not need to be dropped for the scope object to do the appropriate cleanup.
* Lifetime parameters are used to prohibit the closure from smuggling these objects and references out to the caller and trying to carry out further mischief with them.
Locking
Locking
m.with_lock(|i: &mut i32| { println!("{}", i); });
let i: MutexGuard<_> = m.lock().unwrap();
println!("{}", i);
std::mem::forget(i); // this should be an error - it should be required to properly drop i (by letting it go out of scope, or calling i.unlock() which transfers ownership of i, etc)
Abuses of mutable references
Locking
So in the logic:
Affine: f(x) = a*x + b = a*x^1 + b*x^0, "use x once or zero times".
Locking
Locking