0

I have the following C code:

/* the memory entry points to can be changed from another thread but 
 * it is not declared volatile */
struct myentry *entry;

bool isready(void)
{
    return entry->status == 1; 
}

bool isready2(int idx)
{
    struct myentry *x = entry + idx;        
    return x->status == 1; 
}

int main(void) {
    /* busy loop */
    while (!isready()) 
        ; 
    while (!isready2(5)) 
        ; 
}

As I note in the comment, entry is not declared as volatile even though the array it points to can be changed from another thread (or actually even directly from kernel space).

Is the above code incorrect / unsafe? My thinking is that no optimization could be performed in the bodies of isready, isready2 and since I repeatedly perform function calls from within main the appropriate memory location should be read on every call.

On the other hand, the compiler could inline these functions. Is it possible that it does it in a way that results in a single read happening (hence causing an infinite loop) instead of multiple reads (even if these reads come from a load/store buffer)?

And a second question. Is it possible to prevent the compiler from doing optimizations by casting to volatile only in certain places like that?

void func(void)
{
    entry->status = 1;
    while (((volatile struct myentry *) entry)->status != 2)
        ;
}

Thanks.

ilstam
  • 1,473
  • 4
  • 18
  • 32
  • 1
    *Is the above code incorrect / unsafe?* Yes, it's unsafe. There are no guarantees that updates to `entry` are atomic. If you read `entry` in the middle of an update the value could be nonsense. There are probably a lot of other problems in you assumption that "no optimization could be performed", too. – Andrew Henle May 03 '20 at 11:27
  • 1
    @chux-ReinstateMonica Right. The memory it points to can change, not the value of the pointer. – ilstam May 03 '20 at 11:28
  • @AndrewHenle updates to the status field are preceded by a memory barrier so it's the last field to be updated. And after the status changes no more updates are done to entry (until status changes again). Does this change things? – ilstam May 03 '20 at 11:34
  • `it is not declared volatile` --> why not change `struct myentry *entry;` --> `volatile struct myentry *entry;`? – chux - Reinstate Monica May 03 '20 at 11:34
  • @chux-ReinstateMonica For the most part in my code I don't care if somebody else changes entry. I only care in one-two certain places (that's why I ask the second question). If I mark the variable as volatile then I have to use so many casts to volatile in the rest of the code. – ilstam May 03 '20 at 11:46
  • Better to code right with many similar edits than code incorrectly in some. – chux - Reinstate Monica May 03 '20 at 11:50
  • 1
    @chux-ReinstateMonica I just wonder whether it's really "right". E.g. Linus says the following here (https://lwn.net/Articles/233482/) that seems to make sense: """ In C, it's "data" that is volatile, but that is insane. Data isn't volatile - _accesses_ are volatile. So it may make sense to say "make this particular _access_ be careful", but not "make all accesses to this data use some random strategy". """ – ilstam May 03 '20 at 12:12
  • Are you talking specifically about x86? If so, then tag with x86. – Marco Bonelli May 03 '20 at 12:42

1 Answers1

2

If the memory entry points to can be modified by another thread, then the program has a data race and therefore the behaviour is undefined . This is still true even if volatile is used.

To have a variable accessed concurrently by multiple threads, in ISO C11, it must either be an atomic type, or protected by correct synchronization.

If using an older standard revision then there are no guarantees provided by the Standard about multithreading so you are at the mercy of any idiosyncratic behaviour of your compiler.

If using POSIX threads, there are no portable atomic operations, but it does define synchronization primitives.

See also:

The second question is a bugbear, I would suggest not doing it because different compilers may interpret the meaning differently, and the behaviour is still formally undefined either way.

M.M
  • 138,810
  • 21
  • 208
  • 365
  • Let me explain my case in more detail. I only have 2 threads A, B. A only posts entries and B only consumes them asynchronously. B continually scans the array (essentially doing polling) checking if an entry has status SUBMITTED and in that case consumes it and changes its status to FREE. Thread A also scans the array looking for a FREE entry, updates its fields, performs a memory barrier and then changes the status to SUBMITTED (thus ensuring that the status change is written last). I think no data races are possible in that case and I only worry about doing polling on the status field. – ilstam May 03 '20 at 22:21
  • @ilstam I couldn't comment on the correctness of synchronization code without seeing it, perhaps you could post a new question that contains your actual code – M.M May 03 '20 at 22:26
  • I forgot to say that also thread B uses a memory barrier before changing the status. Hence there are not race conditions, since there are only 2 threads waiting for distinct value of status before altering data. – ilstam May 03 '20 at 22:26
  • Okay, maybe I could try writing a minimal working example. – ilstam May 03 '20 at 22:27
  • @ilstam that would be a good idea, I would suggest making a new question rather than editing this one since this question where the reading code has no synchronization is already asked and answered – M.M May 03 '20 at 22:29