Skip to content

InternetSocket: use atomics, not volatile #9248

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 21, 2019
Merged

InternetSocket: use atomics, not volatile #9248

merged 2 commits into from
Jan 21, 2019

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Jan 3, 2019 •
edited
Loading

Description

Use a better tool for the job for the _pending counter, and properly protect _callback.

Pull request type

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

Reviewers

@kjbracey kjbracey mentioned this pull request Jan 3, 2019
Merged
@ciarmcom ciarmcom requested review from a team January 3, 2019 18:00
@ciarmcom
Copy link
Member

ciarmcom commented Jan 3, 2019

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@kjbracey kjbracey force-pushed the socket_pending_atomic branch 3 times, most recently from de9153b to 2fcfe7c Compare January 4, 2019 13:46
@kjbracey
Copy link
Contributor Author

kjbracey commented Jan 4, 2019

Adjusted to use atomic_flag rather than a uint32_t counter. We don't need the count. This means it no longer depends on #9247.

deepikabhavnani
deepikabhavnani approved these changes Jan 4, 2019
View reviewed changes
Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build failures because of missing , in constructor.
Looks good to me 👍

@cmonr
Copy link
Contributor

cmonr commented Jan 4, 2019

CI started

@cmonr
Copy link
Contributor

cmonr commented Jan 4, 2019

Whoops, jumped the gun.

@kjbracey-arm Please take a look at the Travis CI failures.

@kjbracey kjbracey force-pushed the socket_pending_atomic branch from 2fcfe7c to 909260d Compare January 4, 2019 15:55
@mbed-ci
Copy link

mbed-ci commented Jan 4, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@kjbracey kjbracey force-pushed the socket_pending_atomic branch from 909260d to 4bfc874 Compare January 7, 2019 07:48
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 8, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 8, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 8, 2019

Unittest failures, please review

@kjbracey kjbracey force-pushed the socket_pending_atomic branch from 4bfc874 to 7776315 Compare January 8, 2019 09:52
@kjbracey kjbracey force-pushed the socket_pending_atomic branch from 9408b45 to 9829076 Compare January 14, 2019 10:03
@kjbracey
Copy link
Contributor Author

Missed TCPServer - try again.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 14, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jan 14, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@kjbracey kjbracey force-pushed the socket_pending_atomic branch 2 times, most recently from cdcb274 to 806bfde Compare January 16, 2019 11:59
@cmonr
Copy link
Contributor

cmonr commented Jan 17, 2019

NOTE: This PR has now been rebased.

If this was made in error, feel free to force-push your local branch to restore the PR.

@cmonr cmonr force-pushed the socket_pending_atomic branch from 806bfde to a8cc461 Compare January 17, 2019 05:15
@cmonr
Copy link
Contributor

cmonr commented Jan 17, 2019

@kegilbert Could you help verify if the docy-spellcheck errors are valid?

@cmonr
Copy link
Contributor

cmonr commented Jan 17, 2019

SIGH

travis-ci/doxy-spellcheck failure is caused by something slipping in whilst the PR that enabled the job was making it's way through testing.

mbed-os master is broken (https://travis-ci.org/ARMmbed/mbed-os/jobs/480700843) and needs a fix to progress this PR. Hold tight.

@cmonr
Copy link
Contributor

cmonr commented Jan 17, 2019

NOTE: This PR has now been rebased.

If this was made in error, feel free to force-push your local branch to restore the PR.

@cmonr cmonr force-pushed the socket_pending_atomic branch from a8cc461 to 2c7bb5a Compare January 17, 2019 16:52
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 18, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 18, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@kjbracey kjbracey force-pushed the socket_pending_atomic branch from 2c7bb5a to c0775ce Compare January 18, 2019 12:14
@kjbracey
Copy link
Contributor Author

There was a rebase error somewhere in all that - here's yet another version.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 18, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jan 18, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@kjbracey
InternetSocket: Use atomics, not volatile
0f6a4cd 
Use a better tool for the job.
@kjbracey
InternetSocket: better protect _callback
ab037c6 
sigio callbacks can be triggered from interrupt, so changing _callback
needs critical section protection, not just a mutex.
@kjbracey kjbracey force-pushed the socket_pending_atomic branch from c0775ce to ab037c6 Compare January 18, 2019 13:37
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 18, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jan 19, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 8
Build artifacts

@0xc0170 0xc0170 merged commit 67dd449 into ARMmbed:master Jan 21, 2019
@kjbracey kjbracey deleted the socket_pending_atomic branch January 24, 2019 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

+2 more reviewers

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@deepikabhavnani deepikabhavnani deepikabhavnani approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
release-version: 5.12.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kjbracey @ciarmcom @cmonr @mbed-ci @0xc0170 @SeppoTakalo @deepikabhavnani