Volatile member variables vs. volatile object?

Question

I'm trying to implement a multiple producer (via interrupt), single consumer (via application thread) queue on an embedded target in "MpscQueue.h" below.

I'm wondering if I can safely remove the some of the volatile usage below (see inline questions). I would also consider using volatile std::array in place of the C-style buffer_[] shown below, but I was unsure if I could trust its implementation to match the intent of the below. A third alternative would be to mark the MpscQueue object itself as volatile and qualify the relevant methods volatile, but it's not clear if that would result in all member variables (and what they point to, in the case of pointers) being treated as volatile.

Any guidance on this?

template<typename T, uint32_t depth>
class MpscQueue
{
public:
    MpscQueue(void);
    bool push(T& t);
    bool pop(T* const t);

private:
    T volatile buffer_[depth];  // Q1: is volatile unnecessary if never access buffer_[]?
    T volatile* const begin_;   // Q2: is volatile unnecessary if never access value
    T volatile* const end_;     //     via begin_/end_?
    T volatile* head_;          // volatile required so that thread always checks value
    T volatile* volatile tail_; // Q3: is 'T volatile' required so that ISR accounts
                                //     for other ISRs when setting value?
                                // Q4: is '* volatile' required so that ISR accounts
                                //     for other ISRs when checking pointer?
};

template<typename T, uint32_t depth>
MpscQueue<T, depth>::MpscQueue(void) :
    begin_(&buffer_[0]),
    end_(&buffer_[depth - 1]),
    head_(begin_),
    tail_(begin_)
{}

template<typename T, uint32_t depth>
bool MpscQueue<T, depth>::push(T& t)
{
    // "Multiple producer" ISRs can use this function to push at tail

    // Pseudo-code: if not full, *(tail_++) = t
}

template<typename T, uint32_t depth>
bool MpscQueue<T, depth>::pop(T* const t)
{
    // "Single consumer" thread can use this function to pop at head

    // Pseudo-code: if not empty, *t = *(head_++)
}

Edit: To focus the question in the right direction, let me clarify that I have taken care of thread-safety, and it is not part of the question here.

Because this queue is single consumer, there is no thread safety required on the read/pop side. On the write/push side, thread safety among interrupts will be handled by setting all relevant interrupts to the same priority level (otherwise, a lock will be used).

Remember: `volatile` does *not* mean "thread safe". And the *actual* use cases for `volatile` are very few and far between. It should almost *never* be used. — Jesper Juhl, Feb 09 '20 at 17:23
I'm sorry guys, but please read the question. This is on an EMBEDDED target with asynchronous, hardware-driven interrupts that are not linked into the main application. That's basically the poster child for ```volatile```. — abc, Feb 09 '20 at 17:25
@abc Half right. The poster child for volatile is embedded targets when accessing variables that aren't "normal" memory variables, but are memory mapped to hardware device registers. For accessing normal memory variables that are shared between an ISR and another ISR or non-ISR code what you need is appropriate small critical section surrounding the accesses for which you disable the appropriate interrupts. Since you usually will be reading the value once in the critical section and moving it to a non-shared variable, volatile is generally not needed for this case. — Avi Berger, Feb 09 '20 at 17:52
@AviBerger I see. Maybe I need to look more into what the compiler and linker are doing with interrupts, but it was my understanding that the main application has no visibility into the actual ISR, and therefore "global" variables (or static variables with namespace-protected access) that get touched in the ISR may have their reads in the main application optimized out by the compiler if not marked as volatile. — abc, Feb 09 '20 at 17:58
Before we get yet another "volatile can't be used for thread safety" discussion (indeed it can't, but that's not very relevant), kindly read [this](https://stackoverflow.com/a/6996259/584518). The main purpose of volatile in embedded systems is to block incorrect compiler optimizations when the main program communicates with an ISR. You need thread safety measures too, but those are two separate issues. — Lundin, Feb 09 '20 at 22:14
As for the question, the code written is much too high level to be speaking with interrupts. Write a ring buffer or similar low-level ADT, then implement the template stuff on a higher level. If you _must_ use templates in the first place, that is... what on earth are your ISRs doing if you end up with unknown data types from them? This seems like a solution which is looking for a problem that it solves :) — Lundin, Feb 09 '20 at 22:18
@Lundin The code *is* a ring (or "circular") buffer. There will be a "static class" that the ISRs and main will include to push/pop. The template is not for unknown type. It is for abstraction so that if I later change the element type that is pushed/popped, it will simply be a change in the static class' instantiation. — abc, Feb 09 '20 at 22:22
I really don't see why you would want it to have any other type than `uint8_t`. As for writing a ring buffer... what needs to be volatile depends on the compiler. Head/tail pointers certainly. Any lock/semaphore variables as well. The buffer itself... depends on compiler. If you are using gcc or clang, you should make it volatile too, or they might decide to do crazy incorrect pointer aliasing optimizations etc. Always compile with `-fno-strict-aliasing` when using those. If compiling with a regular embedded compiler, you don't have to worry as much. Still, disassemble to verify. — Lundin, Feb 09 '20 at 22:26
@Lundin The question still remains: to what extent do I need volatile to account for various ISRs performing push() on the code above. Regarding thread safety, the only possible issue I can imagine is if the compiler changes the order of operations (and increments the pointer before adding/removing the data). That would be pretty disappointing if that was the case, but I may have to look into it — abc, Feb 09 '20 at 22:28
@Lundin Yes, I will be inspecting the disassembly for this sort of stuff. The element type may be a simple enum or a full blown Event struct that is union of specific types of events (which would differ in the associated data) — abc, Feb 09 '20 at 22:30

Chris Dodd · Answer 1 · 2020-02-09T20:25:50.540

1

As written the code is not interrupt-safe -- if an interrupt occurs while the main thread is doing a read/pop, you have a race condition, and the data structure is likely to be corrupted. The way to fix this is to block interrupts in the main the thread whenever it does a read/pop. If you do that (and the functions that block/unblock interrupts are memory barrieres), the volatiles all become irrelevant and can be removed.

Volatile is pretty much useless for thread synchronization -- its primary use is for interacting with memory-mapped devices.

edited Feb 09 '20 at 20:25

answered Feb 09 '20 at 19:07

Chris Dodd

119,907
13
134
226

I appreciate you looking at the code, but I'm quite sure there are no thread safety issues. Although not made explicit in the code, empty is defined when head==tail, and full is defined when head==tail+1 (accounting for wraparound). Also, head (controlled when the main thread calls pop()) is not modified until after the queue element is copied, so there is no risk of corruption. I considered such issues when designing it. – abc Feb 09 '20 at 21:42
1

And *again*, as specified in the edit, I am not asking about thread synchronization. I keep seeing on this question and all over SO: "volatile is not for thread synchronization". I would appreciate if people stopped spouting off platitudes as soon as they see ```volatile```. I'm not dumb; I'm asking a subtle question about volatiles as they relate to ISRs. – abc Feb 09 '20 at 21:45
2

Then what are you using volatile for? As I said, they have no effect in your code and might as well be removed -- all you need are memory barriers and masking interrupts. – Chris Dodd Feb 09 '20 at 21:50
2

As I understand, when the "main" application is compiled, it has no knowledge of ISRs. E.g., you can't even call the ISRs from the main app. They get mapped by the linker. Therefore, you must inform the compiler not to optimize out reads from variables that could be modified by an unknown (in this case ISR) thread. Otherwise, the compiler may go "hey, your code never writes to this variable, so I will just return some constant value every time instead of storing a variable and actually reading it". If you don't deal with embedded, then maybe you are unaware of this, but it's common in embedded – abc Feb 09 '20 at 21:55
@ChrisDodd Please check this: https://electronics.stackexchange.com/a/409570 – Lundin Feb 09 '20 at 22:22
@abc: you don't need volatile for that, you need a memory barrier. Volatile gives you a memory barrier along with a bunch of other stuff, but its the memory barrier you need -- the rest is just a waste. – Chris Dodd Feb 11 '20 at 02:09
I probably do need a memory barrier in the pop function between the copy and increment if using a volatile head_ pointer won't achieve that. However, I also do need volatile everywhere shown in the OP, so that variables don't get compiled out and they are read every time. – abc Feb 11 '20 at 14:27
You don't show your actual code (just pseudo-code), but all you need for this to be safe is for the 'check for empty' check to be atomic (can't fail due to interrupts) and a memory barrier between that and reading the head (or looping to repeat the check). That's all you need (and technically the volatiles you do have are not adequate if `T` contains any pointers to non-volatile things, though most compilers will implicitly treat any volatile access as a memory barrier) – Chris Dodd Feb 11 '20 at 20:08

abc · Answer 2 · 2020-02-26T22:17:35.403

Here's what I will do regarding the private member variables, with rationale in comments:

T volatile buffer_[depth];  // will never touch buffer_[] via array handle,
                            //   but don't want compiler to optimize it out;
                            //   and technically, the elements are volatile due to push()
T volatile* const begin_;   // buffer_[] has elements of type 'T volatile', so
                            //   keep type of pointer consistent with what it points to
T volatile* const end_;     // "
T volatile* volatile head_; // value must be volatile, as unknown ISR thread will touch;
                            //   also, keep type of pointer consistent
                            // pointer should be volatile since ISRs will read outside
                            //   of "main" thread context
T volatile* volatile tail_; // value should be volatile since multiple ISRs will touch;
                            //   also, keep type of pointer consistent
                            // pointer should be volatile since multiple ISRs will touch

If I used a std::array instead of buffer_[], I'm not sure how I would enforce that not only the elements of the array, but also the underlying pointers/iterators were also volatile. E.g., std::array<T volatile, uint32_t depth> volatile?

If I made the whole MpscQueue object volatile, I'm not sure how I would enforce that the 'volatility' would not only trickle down to the pointers themselves (i.e. * volatile), but also to the pointed-to values (i.e. T volatile* volatile instead of just T* volatile).

See also https://electronics.stackexchange.com/a/409557/141113 and https://electronics.stackexchange.com/a/409569/141113 — abc, Feb 10 '20 at 20:52
Actually, the ```T volatile* volatile head_``` should probably be changed back to ```T volatile* head_```, as the pointer itself is not actually volatile. Really what is needed is a mechanism to enforce the order of operations among read/copy and increment in the pop function. Will have to look into how to achieve that. — abc, Feb 10 '20 at 21:19
Seems that ```asm volatile("" ::: "memory");``` may achieve the desired effect ("```volatile```" solving thread synchronization.. how about that?). Also, it sounds like the original solution posted in this answer might also have worked (i.e. setting the ```head_``` variable to volatile to prevent re-ordering.) according to the comment in https://stackoverflow.com/a/14983432/7775391, but I feel that muddies intent. — abc, Feb 10 '20 at 21:37

Volatile member variables vs. volatile object?

2 Answers2

Linked