CriticalSectionLock class improvement

[maciejbocianski]

Description

Several improvements added to make it more versatile:

• creation in not locked state: to allow more meaningful lock/unlock usage (see example below)
• store lock state to prevent erroneous lock usage

{
   CriticalSectionLock cs(false);
   // some code here

   cs.lock();
      // Code in this block will run with interrupts disabled
   cs.unlock();
   // interrupts will be restored to their previous state

   cs.lock();
      // Code in this block will run with interrupts disabled
   cs.unlock();
   // interrupts will be restored to their previous state
}

Status

READY

Migrations

NO

[maciejbocianski]

@scartmell-arm @0xc0170 @bulislaw
I did some improvements
what do you think?

[ghost]

_locked doesn't need setting while destructing.

[maciejbocianski]

fixed

[ghost]

It might be worth adding an is_locked function

[deepikabhavnani]

Its good to have selection between RAII approach and normal critical section lock in constructor.
@0xc0170 @c1728p9 - Do we need to make lock/unlock functions as static now? As per our discussion it was either this approach or functions as static, correct?

[c1728p9]

Why is recursive locking not supported

[maciejbocianski]

The idea was to prevent user from lock/unlock mismatch e.g. calling more locks then unlocks and vice versa. The recursion could be handled by one CriticalSectionLock instance on each level

[deepikabhavnani]

Mismatch is taken care of in core_util_critical_section_enter / core_util_critical_section_exit functions.
Counter interrupt_enable_counter is maintained to keep track

[c1728p9]

This change blurs the line even further between a regular lock and an RAII class so I would argue this is the wrong direction to be taking the code.

[c1728p9]

As @deepikabhavnani mentioned, I would recommend either making this static or deprecate this and create another class for non-raii locking.

[pan-]

Its good to have selection between RAII approach and normal critical section lock in constructor.

This. If you look at ***_lock classes in the C++ standard library the lock operation can be made at construction time or deferred at a latter point chosen by the application code. We apply the same pattern. As an example you can look at unique_lock.

[kjbracey]

Was initially skeptical about this, but given the current state of CriticalSectionLock, I think this is good. (I would have probably originally made it pure RAII, as manual control is a tad fancy.)

As it stands, the visible lock() and unlock() methods are dangerous, and this makes them work safely in an RAII+combo, akin to unique_lock.

There should be no need for recursion support within this object itself, as it's designed for local scope - anyone calling lock() or unlock() must surely know what its current state is (which is a separate issue from the total crititcal count).

In the new example, the last unlock is redundant - leave it out, maybe?

[0xc0170]

Was initially skeptical about this, but given the current state of CriticalSectionLock, I think this is good. (I would have probably originally made it pure RAII, as manual control is a tad fancy.)

That is what we were discussing the last week, if we should not deprecate these 2 methods, made it scoped class like only.

As it stands, the visible lock() and unlock() methods are dangerous, and this makes them work safely in an RAII+combo, akin to unique_lock.

What about this proposal from above (not having the combo) :

As @deepikabhavnani mentioned, I would recommend either making this static or deprecate this and create another class for non-raii locking.

[maciejbocianski]

Summing up the discussion:

1. Will leave CriticalSectionLock as is and deprecate lock/unlock functions to make it pure RAII (for scope usage)
2. Will create CriticalSection class with static functions lock/unlock for free locking

[0xc0170]

Thanks for the summary, this should help to finalize this change. I want to hear the rest as I did not find a conclusion for this.

Will leave CriticalSectionLock as is and deprecate lock/unlock functions to make it pure RAII (for scope usage)

This one (scoped lock) might be sufficient for most of use case, might not?
@c1728p9, @kjbracey-arm and me are inclined for this one, as this was intention with this class.

@pan- Your conclusion for CriticalSectionLock ?

[pan-]

Will leave CriticalSectionLock as is and deprecate lock/unlock functions to make it pure RAII (for scope usage)

@0xc0170 @maciejbocianski If it's scoped and only meant to be used as an RAII guard, why not deprecate the existing type and create a new one with a name which expose that property: ScopedCriticalSectionLock or CriticalSectionLockGuard ? I personally believe that naming matter and should be as much as possible aligned with concepts present in the standard library.

[maciejbocianski]

Updated plan:

1. Deprecate CriticalSectionLock class and substitute it with RAII class ScopedCriticalSectionLock
2. Create CriticalSection class with static functions lock/unlock for free locking
3. Since both classes are very small put them in the same file platform/CriticalSection.h

[maciejbocianski]

any comments?

[0xc0170]

Sounds sensible the proposal. What others think? You can add additional commits that would add these changes ?

[kjbracey]

Sounds fine to me. But some thoughts:

1. My first thought was that the name CriticalSectionLockGuard was better as it aligned with C++ more - scoped_lock being an early Boost name for unique_lock. But I gather C++17 redefines scoped_lock as a generalised multi-lock form of lock_guard, so I guess it fits the latest C++. And the intent of ScopedXLock is more obvious to me than XLockGuard. If we want a Mutex equivalent (I do) it's ScopedMutexLock then?
2. In my PR Add absolute millisecond tick count to RTOS classes #5419 @pan- suggested a namespace rather than class if the whole class is going to be static-only, so that's what I did. But in this case, I think you would want a class just so you could do C++11 std::lock_guard<CriticalSection> one day. (Not expecting C++11 any time soon, I gather, or you could do that and forget ScopedCriticalSectionLock).
3. No issue with the same header file.

Don't know how much we like templates - would it be worth templating so we can do ScopedLock<CriticalSection> and ScopedLock<Mutex>? Or does everyone find the <> too annoying for frequent use? I kind of do. And Mutex slightly misfits there because it returns errors rather than barfing, but I guess that could be covered by a template specialisation.

(Would the template version have to actually be ScopedLock<CriticalSection> lock(CriticalSection());? Pretty painful...)

I do think a more-advanced manually-controllable unique_lock-like thing like your original suggestion would still have value for both mutexes and critical sections, but you do want the simpler lock_guard/scoped_lock case first, for code size and its documentation properties - "this is a simple lock with no shenanigans that is held for the entire scope". This simple form needs to exist so you can easily distinguish the more complicated cases by the fact that they use the unique_lock.

[pan-]

In my PR #5419 @pan- suggested a namespace rather than class if the whole class is going to be static-only, so that's what I did. But in this case, I think you would want a class just so you could do C++11 std::lock_guard one day. (Not expecting C++11 any time soon, I gather, or you could do that and forget ScopedCriticalSectionLock).

That's a reasonable reason to use a class. @maciejbocianski The class should implement the BasicLockable concept.

Don't know how much we like templates - would it be worth templating so we can do ScopedLock and ScopedLock? Or does everyone find the <> too annoying for frequent use? I kind of do.

The template implementation avoid code duplication which is good from maintenance perspective as well as extensibility: if a type implement the BasicLockable concept then it can be used in a scoped lock. There is nothing preventing us to create alias for common scoped locks such as the one for the critical section or the mutex and use them in code. What do you think ?

And Mutex slightly misfits there because it returns errors rather than barfing, but I guess that could be covered by a template specialisation.

I would be interested to have a common pattern regards the reporting of errors occurred during object construction; this may be an issue for all our RAII guards (and other kind of objects). I also wonder if the Lockable concepts should not be reworked a bit to take into account the fact that we don't throw exceptions to signal errors.

(Would the template version have to actually be ScopedLock lock(CriticalSection());? Pretty painful...)

Not exactly, the parameter in input should outlive the lock therefore it cannot be constructed at the call site:

CriticalSection critical_section;
ScopedLock<CriticalSection> lock(critical_section);

One solution would be to have a global critical_section object and an alias the ScopedLock (Or two different classes as you proposed ...).

[kjbracey]

I also wonder if the Lockable concepts should not be reworked a bit to take into account the fact that we don't throw exceptions to signal errors.

I think failure of a lock() claim on a mutex is fatal enough to be worth mbed_error-ing - it means a programming error or fatal system error. I definitely don't want it returning without the lock held. It would be an untrapped exception normally. (Actually, is the RTX event monitoring catching this now - 5.6 added some asserts for some RTX errors right?)

Without looking, I bet there's no actual recovery from failure of non-timed Mutex::lock() in the system anywhere - only asserts, or maybe passing up an error code to another layer with no checking or recovery.

There is nothing preventing us to create alias for common scoped locks such as the one for the critical section or the mutex and use them in code. What do you think ?

True. Make the template, and have typedefs or whatever for the Mutex and CriticalSection forms.

it cannot be constructed at the call site

Does it matter in this case, where lock() and unlock() are static? Even so, I guess that knowledge of them being static shouldn't be baked into callers.

To save C++ verbosity, I suppose we can give ScopedCriticalSectionLock a default constructor that grabs the default object, thus:

class ScopedCriticalSectionLock : public ScopedLock<CriticalSection> {
    public ScopedCriticalSectionLock() : ScopedLock<CriticalSection>(default_cs_object) {}
};

(I hope that at -O2 that object gets optimised out...)

[pan-]

Does it matter in this case, where lock() and unlock() are static? Even so, I guess that knowledge of them being static shouldn't be baked into callers.

Yes it does matter because if the signature of lock is: void lock(T&) it means that the function accept an rvalue reference. A constructor call return an lvalue reference which may decay to a const rvalue reference; not to a non const rvalue reference. In other words, it won't compile.

Even if it was possible due to the static nature of the critical section lock; I don't think it would be a good thing to encourage such construct because some users may blindly reproduce the pattern invalid pattern with other lockable objects.

[maciejbocianski]

Below my proposition

• one ScopedLock class to use with all BasicLockable objects (http://en.cppreference.com/w/cpp/concept/BasicLockable)
• one ScopedLock class for scope locking using local/private or global/external lock object
• make CriticalSection class as NonCopyable similar to Mutex what prevent misuse ScopedLock<CriticalSection> cs_lock(g_cs) instead ScopedLock<CriticalSection&> cs_lock(g_cs)

The only drawback of this solution I can see is that using global lock object requires class instantiation with reference to LockType (LockType&) what may look a bit strange or complicated for typical user

template <typename LockType>
class ScopedLock {
public:
    ScopedLock()
    {
        _lock.lock();
    }

    ScopedLock(LockType lock): _lock(lock)
    {
        _lock.lock();
    }

    ~ScopedLock()
    {
        _lock.unlock();
    }
private:
    LockType _lock;
};

class CriticalSection : private mbed::NonCopyable<CriticalSection> {
public:
    void lock()
    {
        core_util_critical_section_enter();
    }

    void unlock()
    {
        core_util_critical_section_exit();
    }
};

// usage

CriticalSection g_cs;
Mutex g_mutex;

{
    // local/private lock objects
    ScopedLock<CriticalSection> cs_lock;
    ScopedLock<Mutex> mutex_lock;
}
{
    // global lock objects
    ScopedLock<CriticalSection&> cs_lock(g_cs);
    ScopedLock<Mutex&> mutex_lock(g_mutex);
}

[pan-]

The mission of scoped lock is to lock at construction time unlock at destruction time an existing Lockable object. From my perspective it was assumed that it would take and kept a reference to the lockable object it operates on. It is easy to make mistakes by writing ScopedLock<Mutex> instead of ScopedLock<Mutex&>.

I would amend the proposal the following way:

// Mark the type as non copyable, it is just a guard which lock at construction 
// and unlock at destruction. 
template <typename Lockable>
class ScopedLock : private mbed::NonCopyable<ScopedLock<Lockable> > {
public:
    ScopedLock(Lockable& lockable): _lockable(lockable)
    {
        _lockable.lock();
    }

    // Note: remove virtual specifier, it is not desired to inherit from this class in 
    // a polymorphic way.
    ~ScopedLock()
    {
        _lockable.unlock();
    }
private:
    Lockable& _lockable;
};

class CriticalSection : private mbed::NonCopyable<CriticalSection> {
public:
    // this can be static; access operators (. and ->) will continue to work
    static void lock()
    {
        core_util_critical_section_enter();
    }

    static void unlock()
    {
        core_util_critical_section_exit();
    }
};

// Typedef for the mutex lock
typedef ScopedLock<Mutex> ScopedMutexLock;

// Note: inherit from CriticalSection and ScopedLock to use empty base optimization
class ScopedCriticalSectionLock : private CriticalSection, private ScopedLock<CriticalSection> { 
public:
    ScopedCriticalSectionLock() : 
        CriticalSection(), 
        ScopedLock<CriticalSection>(static_cast<CriticalSection&>(*this)) { 
    }
};

// usage

void foo(Mutex& m) { 
    ScopedMutexLock lock(m);
}

void bar() { 
    ScopedCriticalSectionLock lock;
}

template<typename Lockable>
void baz(Lockable& lockable) { 
   ScopedLock<Lockable> lock(lockable);
}

At the end, user code would mostly rely on ScopedMutexLock and ScopedCriticalSectionLock which are easier to write and a bit specific in the case of the CriticalSection; generic code can rely on ScopedLock.

[kjbracey]

@pan-, that looks good to me - tried compiling with it briefly.

• typedef for ScopedMutexLock is backwards
• ScopedCriticalSectionLock is missing a public
• Terminology nit to avoid apparent tautological loop - the LockType &lock should be using the word "lockable" or "mutex". The lock is what you're defining here, not what you're working on.
• The first bit of code I tried it on actually needed a more-advanced UniqueLock-a-like in places, due to ConditionVariable, but hey-ho.

[maciejbocianski]

@pan- Your solution looks good.

BTW
In my solution we can also use typedefs to hide complex template instatianialization
typedef ScopedLock<CriticalSection> ScopedCriticalSectionLock;
typedef ScopedLock<Mutex&> ScopedMutexLock;

I also noticed that size of ScopedCriticalSectionLock in my implementation is 1B comparing to 4B in yours since your implementation holds reference to itself

[kjbracey]

Size of ScopedCriticalSectionLock not really relevant once the compiler optimisation is on - the whole thing gets optimised to a direct call to core_util_enter_critical_section(), with no actual real object. (I was curious, so checked this - it's true for -O1 or higher on gcc and -O2 or higher on clang).

And even without optimisation, it's only on the stack anyway.

[maciejbocianski]

So I will implement @pan- proposition and will move ScopedLock to separate file platform/ScopedLock.h

[0xc0170]

Triggered uvision CI, should finish soon

This will become ready for integration. @maciejbocianski thanks for the improving critical sections classes. This will require update in the docs

cc @AnotherButler

[sg-]

I dont understand why we went the path of deprecating entire classes that are just 1 release old. The initial PR intention was great but is disappointing to see how we got to the final solution. I would have expected deprecation on local lock/unlock and static members compared to what we ended up with here. That would have demonstrated good engineering hygiene and discipline.

This PR is a great example of lots of bikesheding and wasted time. We need to commit to the code and fix things rather than always looking elsewhere to how it could/should be. This will result in less effort twiddling such trivial things, unplanned things.

[maciejbocianski]

To preserve work done in this PR for future use this PR will be closed soon.
Further work was moved to #5621

[kjbracey]

Okay, that's a no on renaming CriticalSectionLock. What's the plan for ScopedLock<> and ScopedMutexLock?

[0xc0170]

Okay, that's a no on renaming CriticalSectionLock. What's the plan for ScopedLock<> and ScopedMutexLock?

@maciejbocianski Can we close this PR and create a tracking issue for this improvements that were made here?

[kjbracey]

I guess we can still do something useful after #5621, but it will be much less coherent - the CriticalSectionLock will now end up being a special case, rather than a ScopedLock.

I would suggest adding ScopedLock<> and ScopedMutexLock as per this PR, and ignoring what happened to the critical sections.

[kjbracey]

So, maybe just continue this PR, dropping all the changes to CriticalSectionLock.h?

[maciejbocianski]

Sure I can do that!

But I will move it to new PR since this PR is about critical section and changes will cover ScopedLock and Mutex stuff @kjbracey-arm @0xc0170 @pan- @sg-

[maciejbocianski]

Any comments about the proposed action?

[bulislaw]

To be fair I lost the track of what's going on here over a month ago, so opening new PR that limits the noise and confusion would be very much appreciated. That is if it makes sense and original scope changed.

[maciejbocianski]

Work moved to #5621 #5852