Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Atomic extensions: 64-bit, bool, exchange #9600

Merged
merged 5 commits into from Feb 13, 2019

Conversation

Projects
None yet
7 participants
@kjbracey-arm
Copy link
Contributor

commented Feb 4, 2019

Description

Three extensions to the atomic functions in mbed_critical.h:

  • Exchange operations
  • Operations on bool (load, store, exchange, cas)
  • Operations on uint64_t (load, store, exchange, cas, incr, decr)

Bool exchange could have been used by #9520 - slightly tidying and shrinking the code.

Exchange and 64-bit operations are anticipated to be needed as part of ARMv6-M retargetting for ARMC6 atomics, as per its library documentation.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Release notes

(Also covering #9247)

  • mbed_critical.h now also offers atomic_load, atomic_store and atomic_exchange functions.
  • mbed_critical.h now supports atomic bool, int8_t, int16_t, int32_t, uint64_t and int64_t.

Previously code had to just do plain C loads and stores of "atomic" variables, as no atomic operations were provided by the library. Such direct accesses are indeed atomic in ARM hardware and compilers (up to 32-bit when properly aligned), but did not provide any ordering semantics. Having real atomic accesses provides a full and safe replacement for volatile for interthread or thread-to-interrupt synchronisation.

Old style example (very synthetic):

uint32_t data; 
volatile bool ready;

irq_handler() { // assume single-shot, so doesn't re-run while foreground processes
    data = REG;
    ready = true;  // XXX ready could be written before data!
}

// volatile qualifier forces reload each time
while (!ready) {
}
do_something(data); // XXX data could be loaded before "wait for ready" loop.

A workaround for the ordering problems above is to also mark data as volatile, but this scales poorly, and can impede optimisation of work on data. Plus it would still not be safe on a multi-CPU system - the compiler wouldn't reorder, but the CPU still might, leading to a different order visible on another CPU.

New style example:

uint32_t data; 
bool ready; // volatile not needed - atomic functions do the work

irq_handler() { // assume single-shot, so doesn't re-run while foreground processes
    data = REG;
    core_util_atomic_store_bool(&ready, true); // data is written first - atomic store "releases" it
}

// Compiler will reload each time
while (!core_util_atomic_load_bool(&ready) {
}
do_something(data); // data is loaded after seeing ready set (atomic load "acquires" the data)

Note that you don't need to mark atomic data or atomic-protected data as volatile, just as you don't need to mark mutexes or mutex-protected data as volatile. The atomics and mutexes are inherently multi-thread safe, and if they correctly synchronise the data, the data is not subject to racey accesses.

Migration guide

  • Check for existing uses of the volatile type qualifier in code. It should not be used anywhere except for accessing memory-mapped I/O registers. All other uses (eg inter-thread communication or thread<->interrupt communication) can be replaced with atomic operations on non-volatile types.
  • Check to see if uses of atomic_cas can be simplified to atomic_exchange - particularly for bool.
  • Remove casting or select a better type from the new atomic types available.
  • Use 64-bit atomic functions rather than rolling your own with enter/exit critical.

Reviewers

@pan-, @VeijoPesonen

@ciarmcom ciarmcom requested review from pan-, VeijoPesonen and ARMmbed/mbed-os-maintainers Feb 4, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-core Feb 4, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

Note that "exchange" naming is inconsistent with "cas" compare-and-swap, but I would rather stay close to C11/C++11. Maybe at some point "cas" gets renamed to match C++11.

Even when we move to C++11, we may not necessarily be able to totally abandon these functions in favour of built-in compiler atomics. Some compiler versions might use spinlocks that are not safe versus interrupts or hard RTOS thread priorities, and might insert unneeded SMP barriers. It seems compilers+libraries tend to assume "application code with non-hard thread priorities".

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_exchange_64 branch 2 times, most recently Feb 4, 2019

Show resolved Hide resolved platform/mbed_critical.c Outdated
Show resolved Hide resolved platform/mbed_critical.c Outdated

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_exchange_64 branch Feb 4, 2019

kjbracey-arm added some commits Feb 4, 2019

Add signed atomics
All signed implementations are inline - either directly or as inline
calls to out-of-line unsigned definitions.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:atomic_exchange_64 branch to 9e7c659 Feb 5, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Bit more added - signed versions of everything. Can see people needing it and being forced to do their own ugly casting.

@pan-

pan- approved these changes Feb 5, 2019

Copy link
Member

left a comment

Great additions. What would be missing to provide mbed::Atomic<T> ?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Great additions. What would be missing to provide mbed::Atomic ?

Low value for amount of code written ratio?

There are very few atomic users, and many of them seem to be problematic (some tidy+fix patches pending from me soon). This PR is partly as the result of that tree search - seeing the people whose code was ugly due to lack of bool, signed or exchange.

But if my concerns above do hold that the off-the-shelf C library atomics might not be suitable for protecting privileged code against interrupts, then maybe it is worth fleshing this out further, including templates.

On the whole though - I'm usually happier seeing people using mutexes. Code is more likely to be correct. I almost want don't want to encourage people to use this :/ Templates might make it too easy.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Added a chunk of release note text.

@cmonr

cmonr approved these changes Feb 11, 2019

@@ -100,6 +100,9 @@ void core_util_critical_section_exit(void)
}
}

/* Inline bool implementations in the header use uint8_t versions to manipulate the bool */
MBED_STATIC_ASSERT(sizeof(bool) == sizeof(uint8_t), "Surely bool is a byte");

This comment has been minimized.

Copy link
@cmonr

cmonr Feb 11, 2019

Contributor

😮

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 12, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 12, 2019

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Feb 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Final review from @ARMmbed/mbed-os-core requested

@0xc0170 0xc0170 requested a review from AnotherButler Feb 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@AnotherButler Please review docs and release notes section

@cmonr cmonr added ready for merge and removed needs: review labels Feb 12, 2019

@cmonr cmonr merged commit 709e6ff into ARMmbed:master Feb 13, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10531 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.