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

Fix #673 and #677, update global locks #678

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Dec 9, 2020

Describe the contribution
Reworks the POSIX global lock implementation.

Testing performed
Build and run all unit tests
Build and sanity check CFE

Expected behavior changes
No longer changing signal masks repeatedly/unexpectedly. (may be relevant to some BSP/driver developers)

System(s) tested on
Ubuntu 20.04
RTEMS 4.11

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey changed the base branch from main to integration-candidate December 9, 2020 14:17
@jphickey jphickey marked this pull request as draft December 9, 2020 14:18
@jphickey
Copy link
Contributor Author

jphickey commented Dec 9, 2020

Only marked as draft because it requires a baseline which is not yet in main - will rebase once next baseline is settled.

@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Dec 9, 2020
@astrogeco astrogeco added CCB-20201209 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Dec 9, 2020
@astrogeco
Copy link
Contributor

CCB 2020-12-09 APPROVED

  • Do we expect other platforms to support the condition variables from posix? There are "barriers" in RTEMS but they're not paired with mutex. VxWorks has binary semaphores but also suffers from the non atomic issue. These have a risk that the state can change in the midst of the call.
  • RTEMS does have a posix layer that might include the needed functionality
  • Caution against having a difference platform implementation for a minor performance gain
  • Concerns that the polling and delay would result in taking more time for this operation than expected
  • @jphickey open issue about this in VxWorks and RTEMS

Removes the signal mask updates from the POSIX global lock (not needed).
Adds a condition variable to the structure, which can be used to
directly wake up a waiting task rather than requiring that task to poll
the global state.
@jphickey jphickey force-pushed the fix-673-677-global-locks branch from 1412ff1 to a41f748 Compare December 11, 2020 00:32
@jphickey
Copy link
Contributor Author

Rebased to main.

Also fixed an intermittent issue I found in testing where the mutex would not be released. There is a test case where a task gets deleted immediately after creation, and sometimes this would happen while it was still initializing and holding the table mutex. Solution is to add a cleanup handler around the pthread_cond_wait just like bin sems use, so if the task is deleted, the mutex also gets unlocked.

@astrogeco
Copy link
Contributor

@jphickey I resolved a small conflict here, I'll do a squash-commit on this unless you want to rebase and force push.

@jphickey
Copy link
Contributor Author

@jphickey I resolved a small conflict here, I'll do a squash-commit on this unless you want to rebase and force push.

Yes, I recall a small conflict with the #endif comment.

Does it really need a rebase though? Can't we just make a normal merge commit with the conflict resolved? Do you want me to do that in the integration candidate?

I don't like squashes at this point because it makes it hard to track what branches in my fork can be deleted (i.e. there is no specific git record of the merge, and git branch --merged doesn't report it, and so on).

@astrogeco
Copy link
Contributor

@jphickey I resolved a small conflict here, I'll do a squash-commit on this unless you want to rebase and force push.

Yes, I recall a small conflict with the #endif comment.

Does it really need a rebase though? Can't we just make a normal merge commit with the conflict resolved? Do you want me to do that in the integration candidate?

I don't like squashes at this point because it makes it hard to track what branches in my fork can be deleted (i.e. there is no specific git record of the merge, and git branch --merged doesn't report it, and so on).

I suggested a rebase to avoid having f5b84da in the commit history.

Regarding the squash, I think github gives me the option to delete the branch after merging

@jphickey
Copy link
Contributor Author

No worries about f5b84da we can avoid this either way.

Regarding deleting branches I also need to delete them on my local machine - not just the fork. I very much prefer not to break git branch --merged command.

I can easily/quickly push a merge commit to integration-candidate that fixes it. Or alternatively if you really prefer a rebase I can do that too but I'd prefer to do it myself as it keeps my local branches in sync with my fork branches.

@jphickey jphickey force-pushed the fix-673-677-global-locks branch from f5b84da to a41f748 Compare December 17, 2020 16:32
@astrogeco astrogeco merged commit 25cd5df into nasa:integration-candidate Dec 17, 2020
@jphickey jphickey deleted the fix-673-677-global-locks branch January 27, 2021 14:09
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement better way to wait for status change POSIX unnecessarily setting signal masks in global lock
3 participants