On 18/05/2026 09:43, Boris Brezillon wrote:
> On Thu, 14 May 2026 10:09:10 -0700
> Chia-I Wu <[email protected]> wrote:
>
>> On Thu, May 14, 2026 at 6:24 AM Steven Price <[email protected]> wrote:
>>>
>>> On 13/05/2026 17:58, Boris Brezillon wrote:
>>>> Right now panthor is mixed bag of manual locks and guards. Let's
>>>> make that more consitent and thus encourage new submissions to go
>>>> for guards.
>>>
>>> I'm fine with encouraging guards for future code - but I'm a little wary
>>> of a big change like this - it's hard to review it and check that
>>> everything works the same. And it's a little dubious that the mechanical
>>> refactoring produces more readable code in some cases.
>> I agree with Steven in general, although I am in favor of landing now
>> that you've gone through the trouble.
>
> Honestly, I agree with you. The only reason I went for it is
> because the mix we have right now is pretty confusing. This has to do
> with the fact the scopes are often loosely defined unless you used
> scoped_guard(), so it's pretty easy to mess up the lock/unlock
> ordering. For instance,
>
> mutex_lock(locka);
> guard(lockb);
> mutex_unlock(locka);
>
> ...
>
> once expanded, turns into inconsistent locked sections, where the inner
> lock (lockb) is released after the outer one (locka).
I think that's a good argument for getting all the guard forms available
before tackling the conversion. Mostly I feel like it would be benefit
from being split up into multiple patches (maybe one per file?) so that
there are smaller units to review.
>>
>> I also have mixed feelings about some of the non-scoped guards. Their
>> scopes are extended slightly than before, supposedly to avoid adding
>> another level of indentation. But other than slightly slower,
>
> I tried to used scoped_guard()s every where the extra non-guarded
> section could be CPU heavy (the only bits left are some very simple
> bit/arithmetic ops, and a couple queue_work() IIRC).
>
>> it also
>> becomes less clear what exactly do the guards protect.
>
> I know, and I have pretty much the same feeling, but we've crossed that
> bridge when we started accepting non-scoped guard()s, unfortunately.
The problem with scoped guards is the extra level of indentation.
Personally I find a mixture of all three is appropriate depending on the
case.
E.g.
int small_simple_function() {
if (simple_condition)
return early;
guard(lock);
if (condition_that_needs_lock)
return early;
/* more work */
return late;
}
Here it's easy to reason because the lock is just held for the duration
of the function after the initial early-out condition is checked.
int short_lock() {
/* bunch of work */
scoped_guard(lock) {
tmp = read_value();
if (tmp == 42)
return -ESOLONGANDTHANKSFORALLTHEFISH;
tmp++;
write_value(tmp);
}
/* more work */
}
Here there's a small section of code which is working on the lock, so it
makes sense to indent it to show the boundaries of it. The other nice
thing is that the error return handles the locks for us.
int old_fashioned() {
if (lock_required)
mutex_lock(lock);
/* some work */
if (lock_required)
mutex_unlock(lock);
}
Generally a pattern to be avoided if possible, but IMHO this is much
better than the equivalent of:
int dodgy_function() {
/* some work */
}
int outer_function() {
if (lock_required) {
scoped_guard(lock)
dodgy_function();
} else {
dodgy_function();
}
}
which requires breaking out the work into an extra function and
duplicating the call (or worse duplicating the function body).
One thing I do agree is that mixing and matching within the same
function is a recipe for confusion and mistakes.
Thanks,
Steve
