Remove usage of volatile keyword

[PSUdaemon]

I think we should remove volatile from our code. Here is an article describing it's correct usage. It's entirely likely that this will mostly replace ink_atomic with std::atomic as most of the incorrect usage of volatile is around thread safety. I have marked this as WIP and plan to add commits over time. Please feel free to review the current progress or debate the issue entirely.

[PSUdaemon]

Didn't have to remove as much of ink_atomic as I had thought I would. I can continue down that path though if people think it's appropriate for this PR, or perhaps start a new one just for that purpose. Thoughts?

[SolidWallOfCode]

Any idea what the machine code for this looks like?

[zwoop]

Sweet! I think you should split this into two PRs: One for the volatile removal, and one for starting the migration to use std::atomic.

[SolidWallOfCode]

Phil checked the machine code output and it looks the same as with the compiler intrinsics.

[PSUdaemon]

@SolidWallOfCode the std::atomic operator++ appears to produce identical x86_64 assembly to ink_atomic_increment.

EDIT: Thanks to @ushachar for reminding me about godbolt, here is an example of how this is the same: https://godbolt.org/g/6UPVa5

Needs to be squashed.

[SolidWallOfCode]

Shouldn't this be an atomic?

[PSUdaemon]

All this patch is doing is removing volatile, which did not give us thread safety. So this shouldn't really change anything with regard to that. If it should be atomic, that might be another issue.

[bryancall]

@PSUdaemon We should investigate why each of the variables were volatile and covert them to atomic in this PR. Having the changes to atomics in another PR would make it harder to see what needs/did change to atomic.

[PSUdaemon]

@bryancall Should I do the conversions with std::atomic or ink_atomic? I think I'd prefer the former, especially if we plan to remove the latter soon.

[PSUdaemon]

@SolidWallOfCode I don't think aio_buf needs to be an atomic. Can you explain why you think otherwise?

[bryancall]

@PSUdaemon I would use std::atomic

[PSUdaemon]

Good, cause that's what I did :)

[dragon512]

I think he means on that line. Is this variable shared between threads?

[PSUdaemon]

@dragon512 no, it's just a member of a struct for AIO. Each AIO operation should have its own struct. There is a lot of "Oh, we are going to update this then something else has to read it. Better make it volatile so the other thing sees the new data" in the code. Totally useless AFAICT.

[dragon512]

Ya this should be removed... it slowing the code down by forcing register load in the pipeline.

[SolidWallOfCode]

This looks more extensive than it was on my first review. I am concerned about the whole scale removal of volatile because it has more uses than just marking variables for atomic operations. Even with a mutex it is still possible to get stale data if the data is not marked volatile because the compiler might have cached it in memory or could even be in a processor cache while the actual value has been changed by another thread.

[PSUdaemon]

@SolidWallOfCode that is not my understanding of the volatile keyword at all. The atomics force a cache invalidation in the CPU so that those things you described can't happen. The volatile does nothing in this regard. It isn't a memory fence of any kind.

[PSUdaemon]

@zwoop I changed the first commit to not introduce std::atomic. I will leave this here for further discussion and can squash if we decide to merge.

[ushachar]

The article you mentioned glosses over a very valid & important use case of volatile:
"""
memory that is modified by an external agent or appears to be because of a screwy memory mapping
"""
External agent may very well be another thread adjusting a shared variable.
(there's a deeper discussion of this in the comments on the article)

Volatile is often used in this case to prevent the compiler from using a stale value from a register instead of re-reading the value from RAM.
(See comparison between:
https://godbolt.org/g/RXkcuk
and
https://godbolt.org/g/Xm8TC9)

It's true that most (if not all) volatiles you remove in this PR appear to be of the "Oh -- I'm using threads so let's write volatile for no reason other then to kill performance" kind, but it requires a careful look to make sure we're not missing a valid use case....

[PSUdaemon]

@ushachar if you are altering it from another thread you should be using a concurrency primitive to protect it. A mutex or atomic loads and stores.

https://godbolt.org/g/hgpkZx

If you are suggesting that maybe someone was misusing volatile and we need to see if we should add extra atomics, then I agree.

[PSUdaemon]

And here is a bit of a contrived example, but shows why using volatile is not the correct solution.

https://godbolt.org/g/9Y7o4J

[ywkaras]

Hmm I'm not sure about changing unsigned long to uint64_t. Are we sure unsigned long is not uint32_t on any system that TS runs on?

If you're on a 32-bit architecture, with no bus-locking 64-bit load/store instructions, I'm pretty sure using std::atomic<uint64_t> will generate code with retry loops.

[PSUdaemon]

a) We don't support 32bit
b) This is an example plugin and that is just a counter for monotonically increasing values

TBH, I was sort of just being cute by making that change. There was a comment that said "use a mutex" or similar, so I just added atomics.

[dragon512]

Why are these value volatile in the first place? All this says to the compiler is to not assume the cache is correct. It does not make it atomic, or thread safe.

[dragon512]

I am for this. I just don't know our code enough to know which vars have locks and which one do not. I am +1 with using std::atomic. The exists of volatile is not going hurt ( might help a little) it is not a replacement for locks or correct interlock calls ( ie std::atomic<>)

[ywkaras]

Volatile isn't meant to help with cache incoherency. There is no portable way to deal with cache incoherency.

I've only worked with one multi-core processor (back in 2003) with per-core cache where there was not transparent coherency/sniffing between the cores. But this was not by design, it was an admitted manufacturing defect. Otherwise, I've only seen cache incoherency when using DMA. And I've always seen it dealt with by using the asm directives to insert (machine-specific) cache flush and invalidate instructions. Volatile is not helpful for this.

Volatile was meant originally for memory mapped I/O. It tells the compiler that all nominal load/stores have to correspond, one-to-one, to machine code loads and stores, which are (nominally) executed in the nominal order given by the program. On a processor with strong memory ordering (like the older x86s have), you don't need memory fences to make sure the nominal machine execution order of loads and stores is the same as the actual execution order. On these machine, if you use types that are "naturally" atomic (with proper address alignment), you can do lock-free multithreading using volatile. Sorry that I cannot give a non-shitty explanation or give a link to a non-shitty explanation.

[ywkaras]

So anyways, my actual point is, I think we can change volatile to std::atomic, it may help and shouldn't hurt. But it's unlikely to eliminate race conditions in the code.

[dragon512]

moving to std::atomic is a good thing but we have to be smart about using it. It adds instructions to make sure certain memory ordering happens correctly... https://bartoszmilewski.com/2008/12/01/c-atomics-and-memory-ordering/ This can have performance impacts on the code, if we are unwise in the usage as the link shows. I would say that std::atomic is a replacement for the function such as ink_atomic_decrement, etc . I would say that after this change we should remove all usage of ink_atomic_xxx with std::atomic

[PSUdaemon]

@dragon512 that is the plan. Going to do a second PR that does ink_atomic -> std::atomic.

[SolidWallOfCode]

Go big or go home. I understand the concerns about volatile and register caching but I think that that can either be replaced with std::atomic or is irrelevant because the value isn't read outside a lock. We'll just have to keep an eye out for potential thread issues to fix with judicious application of std::atomic.