Locking [LWN.net]

Locking

Posted Sep 24, 2024 20:49 UTC (Tue) by viro (subscriber, #7872)
In reply to: Locking by Cyberax
Parent article: Resources for learning Rust for kernel development

... 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.

to post comments

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.