[atomic] Probable documentation issue: how do we know atomic is safe and/or efficient?
I'm looking to use Boost.Atomic for the double-checked-locking-idiom, but I'm having trouble discovering a) Whether the library is correctly implemented. b) How efficient (or otherwise) the library is on various platforms. I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff! I can't help thinking that questions like this could be allayed by better documentation - specifically what operations are used to ensure correctness on each supported platform, plus rationale and/or link to reference material. My apologies if this has been covered already in the review, and I realize that documentation is a lot of work, but I can see no way to ensure correctness other than through code inspection, so proper documentation is essential in that case. Regards, John.
On Tue, Dec 24, 2013 at 5:43 PM, John Maddock <john@johnmaddock.co.uk> wrote:
I'm looking to use Boost.Atomic for the double-checked-locking-idiom, but I'm having trouble discovering
a) Whether the library is correctly implemented.
I hope so. :) The tests are there, but they were mostly run on x86 in the test farm.
b) How efficient (or otherwise) the library is on various platforms.
In general, if the library reports the particular type as lock-free, you should expect performance comparable to std::atomic. In some cases (notably, older gcc) it's even better.
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
This is enough in case of x86, which has been the primary, if not the only target for MSVC for a long time. I'm not so experienced with ARM, so Boost.Atomic may need some adjustments for MSVC for this target architecture.
I can't help thinking that questions like this could be allayed by better documentation - specifically what operations are used to ensure correctness on each supported platform, plus rationale and/or link to reference material.
Well, if you have knowledge of a particular architecture, you probably know what atomic ops it supports and both Boost.Atomic and std::atomic are most likely based on them. The particular instructions and their code representation (i.e. compiler intrinsics, inline assembly, kernel calls, etc.), I think, is more an implementation detail. It could be generally described for a curious reader though.
My apologies if this has been covered already in the review, and I realize that documentation is a lot of work, but I can see no way to ensure correctness other than through code inspection, so proper documentation is essential in that case.
Documentation is hardly a measure of code correctness, but I admit, in case of Boost.Atomic it could be improved.
a) Whether the library is correctly implemented.
I hope so. :) The tests are there, but they were mostly run on x86 in the test farm.
atomic<>s are hard to test, so it is best to verify the correctness by analyzing the code.
b) How efficient (or otherwise) the library is on various platforms.
In general, if the library reports the particular type as lock-free, you should expect performance comparable to std::atomic. In some cases (notably, older gcc) it's even better.
for arm/ppc platforms i suppose that std::atomics will perform better, because (a) some operations are emulated with cas (no idea if this has a real-world impact) and (b) boost.atomic does not support all hardware instructions (iirc armv7 provides atomic double-width instructions which boost.simd does not make use of).
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
This is enough in case of x86, which has been the primary, if not the only target for MSVC for a long time. I'm not so experienced with ARM, so Boost.Atomic may need some adjustments for MSVC for this target architecture.
compare http://en.cppreference.com/w/cpp/atomic/memory_order, section "Relationship with volatile"
My apologies if this has been covered already in the review, and I realize that documentation is a lot of work, but I can see no way to ensure correctness other than through code inspection, so proper documentation is essential in that case.
fyi: the original author did the review and disappeared. i (as in review manager) was actually surprised that he committed the stuff to trunk ... it only ended up in the release, because andrey and me adopted the library, though personally i have limited time to work on stuff other than bug fixes (and no interest in improving the documentation) ... my personal recommendation: * if your compiler supports std::atomic, use it instead * if you need a good API documentation, check the std::atomic documentation * if you want to verify the correctness, use the source ;) cheers, tim
John Maddock wrote:
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
"volatile" was changed in VC++ 8.0 to guarantee acquire/release semantics, then changed back in 2012 to only do so when /volatile:ms is active, which it is by default, except on ARM. http://msdn.microsoft.com/en-us/library/12a04hfd.aspx
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
"volatile" was changed in VC++ 8.0 to guarantee acquire/release semantics, then changed back in 2012 to only do so when /volatile:ms is active, which it is by default, except on ARM.
So does this mean that Boost.Atomic is perhaps not safe after all? John.
On Tuesday 24 December 2013 17:17:37 John Maddock wrote:
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
"volatile" was changed in VC++ 8.0 to guarantee acquire/release semantics, then changed back in 2012 to only do so when /volatile:ms is active, which it is by default, except on ARM.
So does this mean that Boost.Atomic is perhaps not safe after all?
No, it's perfectly safe on x86. It is also safe on ARM if /volatile:ms is specified.
On Dec 24, 2013, at 2:24 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Tuesday 24 December 2013 17:17:37 John Maddock wrote:
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
"volatile" was changed in VC++ 8.0 to guarantee acquire/release semantics, then changed back in 2012 to only do so when /volatile:ms is active, which it is by default, except on ARM.
So does this mean that Boost.Atomic is perhaps not safe after all?
No, it's perfectly safe on x86. It is also safe on ARM if /volatile:ms is specified.
How do you ensure the use of /volatile:ms? Can users build Boost with /volatile:iso? ___ Rob (Sent from my portable computation engine)
On Wed, Dec 25, 2013 at 4:40 PM, Rob Stewart <robertstewart@comcast.net> wrote:
On Dec 24, 2013, at 2:24 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Tuesday 24 December 2013 17:17:37 John Maddock wrote:
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
"volatile" was changed in VC++ 8.0 to guarantee acquire/release semantics, then changed back in 2012 to only do so when /volatile:ms is active, which it is by default, except on ARM.
So does this mean that Boost.Atomic is perhaps not safe after all?
No, it's perfectly safe on x86. It is also safe on ARM if /volatile:ms is specified.
How do you ensure the use of /volatile:ms? Can users build Boost with /volatile:iso?
You can't, I was just answering John's question. To be clear, yes, I agree that this needs fixing. It's just no one got around to do it. A ticket would be welcome. Especially, with a patch. :)
On Dec 25, 2013, at 7:47 AM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Wed, Dec 25, 2013 at 4:40 PM, Rob Stewart <robertstewart@comcast.net> wrote:
On Dec 24, 2013, at 2:24 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
So does this mean that Boost.Atomic is perhaps not safe after all?
No, it's perfectly safe on x86. It is also safe on ARM if /volatile:ms is specified.
How do you ensure the use of /volatile:ms? Can users build Boost with /volatile:iso?
You can't, I was just answering John's question.
You used the phrase, "perfectly safe," which suggested that /volatile:ms was somehow ensured on x86.
To be clear, yes, I agree that this needs fixing.
I wouldn't have described it as you did, but things are clear now. ___ Rob (Sent from my portable computation engine)
On Wed, Dec 25, 2013 at 5:12 PM, Rob Stewart <robertstewart@comcast.net> wrote:
On Dec 25, 2013, at 7:47 AM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Wed, Dec 25, 2013 at 4:40 PM, Rob Stewart <robertstewart@comcast.net> wrote:
On Dec 24, 2013, at 2:24 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
So does this mean that Boost.Atomic is perhaps not safe after all?
No, it's perfectly safe on x86. It is also safe on ARM if /volatile:ms is specified.
How do you ensure the use of /volatile:ms? Can users build Boost with /volatile:iso?
You can't, I was just answering John's question.
You used the phrase, "perfectly safe," which suggested that /volatile:ms was somehow ensured on x86.
x86 guarantees correct memory view, except for very specific operations. And compiler barriers in the code guarantee that the actual instructions are generated in the right order. So yes, regardless of the /volatile:ms switch, on x86 it works as intended.
On Wed, Dec 25, 2013 at 4:47 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Wed, Dec 25, 2013 at 4:40 PM, Rob Stewart <robertstewart@comcast.net> wrote:
On Dec 24, 2013, at 2:24 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Tuesday 24 December 2013 17:17:37 John Maddock wrote:
I'm sure the answer to (a) is that it is correct, but I was surprised that atomic<int> relied on the volatile modifier for thread safe load/stores on MSVC, I had assumed that something more than that would have been required, but I know I'm out of date on this stuff!
"volatile" was changed in VC++ 8.0 to guarantee acquire/release semantics, then changed back in 2012 to only do so when /volatile:ms is active, which it is by default, except on ARM.
So does this mean that Boost.Atomic is perhaps not safe after all?
No, it's perfectly safe on x86. It is also safe on ARM if /volatile:ms is specified.
How do you ensure the use of /volatile:ms? Can users build Boost with /volatile:iso?
You can't, I was just answering John's question.
To be clear, yes, I agree that this needs fixing. It's just no one got around to do it. A ticket would be welcome. Especially, with a patch. :)
I have updated Boost.Atomic (develop branch) so that it should work on Windows ARM, but I don't have hardware to test it. To my knowledge, none of our testers have the hardware as well. So I would appreciate if someone tried it out and reported the results (both positive and negative). Thanks.
participants (5)
-
Andrey Semashev -
John Maddock -
Peter Dimov -
Rob Stewart -
tim