Re: [PATCH v0] cgroup: Add lock guard support

[Jemmy Wong <jemmywong512@gmail.com>]

Hi Johannes,

Thanks for your feedback. I understand your concerns about the 
indentation and visual clarity with the lock guard. 
Let me explain why I think it’s still a good choice:

1. Less Error-Prone
Lock guards automatically release the lock when the scope ends, 
even with early returns or errors. Manual lock/unlock can miss an unlock, 
causing deadlocks—lock guards prevent this.

2. Clear Critical Sections
The scoped_guard clearly shows where the critical section starts and ends, 
making the code easier to follow and maintain, even if it adds indentation.

3. Fixing the Concerns
I agree the extra indentation can look messy. I can refactor to reduce nesting 
or adjust the style to make critical sections clearer. 
I’ll also ensure the lock guard only covers the critical section, 
avoiding non-critical work inside.

Many modern programming languages provide lock guard-like constructs
to manage synchronization safely, 
showing that this approach is a widely accepted best practice. 
Manual lock/unlock is often discouraged because it’s error-prone, 
especially in complex codebases. Here are a few examples:

C++:
std::mutex m;
void example() {
    std::lock_guard<std::mutex> lock(m); // Locks m
    // Critical section
} // Automatically unlocks m when scope ends


Rust:
use std::sync::Mutex;
let m = Mutex::new(0);
{
    let mut guard = m.lock().unwrap(); // Locks mutex
    *guard += 1; // Critical section
} // Automatically unlocks here

Java:
ReentrantLock lock = new ReentrantLock();
try (lock.lock();) { // Locks and ensures unlock
    // Critical section
} // Automatically unlocks here

Python:
from threading import Lock
lock = Lock()
with lock: # Locks here
    # Critical section
# Automatically unlocks here

I think lock guards make the code safer and clearer, 
but I’m happy to tweak the implementation to address your concerns. 
Let me know your thoughts!

Best,
Jemmy


> On Jun 6, 2025, at 6:42 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> On Thu, Jun 05, 2025 at 05:54:15PM -0700, Alexei Starovoitov wrote:
>> On Thu, Jun 5, 2025 at 2:11 PM Jemmy Wong <jemmywong512@gmail.com> wrote:
>>> 
>>> This change replaces manual lock acquisition and release with lock guards
>>> to improve code robustness and reduce the risk of lock mismanagement.
>>> No functional changes to the cgroup logic are introduced.
>>> 
>>> Signed-off-by: Jemmy Wong <jemmywong512@gmail.com>
>>> 
>>> ---
>>> include/linux/cgroup.h     |   7 +
>>> kernel/bpf/cgroup.c        |  96 +++---
>>> kernel/bpf/local_storage.c |  12 +-
>> 
>> Nack for bpf bits.
>> It only uglifies the code.
> 
> I agree with this.
> 
> The extra indentation from scoped guard is unfortunate.
> 
> The guard with implicit unlock gives me the heebeejeebees - it's
> asymmetric and critical sections don't stand out visually at all.

There’re two types of guard: guard and socped_guard. 

The guard is asymptomatic and doesn’t stand out visually very well.
We could choose socped_guard which clearly defines the boundaries of critical section.

To clarify usage:
- For critical sections is part of a function, prefer socped_guard, 
    as it explicitly delineates the boundaries of the critical section.
- For critical sections spanning an entire function, prefer guard, 
    as it better suits the broader scope.

> Adjusting critical section boundaries with guard means either:
> * indentation churn to convert to scoped guard,
> * forcing abstraction splits along critical sections (non-API
>  _locked functions), which makes the code flow harder to follow,
> * or straight-up violating abstraction layering and adding
>  non-critical stuff to callers where it doesn't make sense.
> 
> I don't remember the last production bug from forgetting to drop a
> lock. Meanwhile, the things that people actually seem to struggle with
> when it comes to locks appear to become harder with those primitives.