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

CircularBuffer: volatile specifier removal #7727

Merged
merged 1 commit into from Aug 9, 2018

Conversation

Projects
None yet
6 participants
@0xc0170
Member

0xc0170 commented Aug 8, 2018

Volatile specifier in this case it not required as we currently have all accesses
to the buffer protected by critical section. This shall optimize accesses in
some cases to the buffer.

Fixes #7702

Description

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change
CircularBuffer: volatile specifier removal
Volatile specifier in this case it not required as we currently have all accesses
to the buffer protected by critical section. This shall optimize accesses in
some cases to the buffer.

Fixes #7702

@0xc0170 0xc0170 requested review from pan- and kjbracey-arm Aug 8, 2018

@pan-

pan- approved these changes Aug 8, 2018

@kjbracey-arm

Only thing to worry about I can see is construction, but any execution context that invokes the constructor and then hands the buffer to another context /should/ already have its own ordering for that handover, otherwise the handover would be a race.

But this is not a "fix". It's not fixing an actual bug, and it could conceivably trip a problem in something lacking correct init synchronisation.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 8, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 8, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 8, 2018

Build : SUCCESS

Build number : 2759
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7727/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 9, 2018

Hmm...
/morph test

But this is not a "fix". It's not fixing an actual bug, and it could conceivably trip a problem in something lacking correct init synchronisation.

@kjbracey-arm Like for example, the lp-timeout callback test?

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 9, 2018

@cmonr cmonr merged commit 0346d22 into ARMmbed:master Aug 9, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 566 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9833 cycles (+767 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Aug 9, 2018

@bmcdonnell-ionx

This comment has been minimized.

Contributor

bmcdonnell-ionx commented Aug 9, 2018

@cmonr / @0xc0170,

Do you think it's likely that someone else will look at the code in the future and ask questions rehashing the conversation in #7702? If so, consider adding comments in the code.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Aug 10, 2018

Once the volatiles have gone from here, not sure it's worth noting their absence.

I think it is worth noting in the documentation of the enter/exit critical functions themselves. If you're using those functions, using ordinary variables without volatile should be as "routine" as using variables without volatile in multiple threads with Mutex::lock().

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Aug 10, 2018

Relabelled as a refactor and release in 5.10.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 10, 2018

Do you think it's likely that someone else will look at the code in the future and ask questions rehashing the conversation in #7702? If so, consider adding comments in the code.

@bmcdonnell-ionx The details are in the commit message (why they are being removed and what we are fixing). I found it sufficient for this case.

@mbed-ci

This comment has been minimized.

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7727 from 0xc0170/fix_#7702
CircularBuffer: volatile specifier removal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment