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 #456, Perf log threading and concurrency issues #595

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Apr 9, 2020

Describe the contribution

Replaces the OS_IntLock with a standard OSAL mutex for protecting the shared/global perflog data structure. This may introduce unexpected task switches when contention occurs, but it ensures proper exclusion with respect to the data structures.

Removes the temporary child worker task that was spawned for writing the log data to a file, and replace with a more generic CFE ES background task. The background task is started at boot and
pends on a semaphore until there is work to do.

The background performance log dump is implemented as a state machine which is called repeatedly over time from the background job task. This performs a limited amount of work on each invocation, and resumes where it left from the previous invocation.

Fixes #456
Fixes #324

Testing performed
Build CFE with ENABLE_UNIT_TESTS=TRUE
Confirm all unit tests pass
Confirm near 100% coverage on all newly added/modified code
Run CFE and send commands to start performance logging
Send other CFE commands to generate performance log activity
Send command to stop performance log and generate a dump file
Confirm validity of dump file by opening with Java tool. No errors reported when opening file.

Expected behavior changes
No impact to behavior. Previously the perf log dump file frequently contained errors due to out of order or otherwise corrupted entries, which is now fixed.

System(s) tested on

  • Ubuntu 18.04 LTS 64-bit (native)
  • MIPS 32 target running in QEMU (big endian)

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

Replaces the `OS_IntLock` with a standard OSAL mutex for protecting
the shared/global perflog data structure.  This may introduce unexpected
task switches when contention occurs, but it ensures proper exclusion
with respect to the data structures.

Removes the temporary child worker task that was spawned for writing
the log data to a file, and replace with a more generic CFE ES
background task.  The background task is started at boot and
pends on a semaphore until there is work to do.

The background performance log dump is implemented as a state machine
which is called repeatedly over time from the background job task.  This
performs a limited amount of work on each invocation, and resumes where
it left from the previous invocation.
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

Looks good.

@skliper
Copy link
Contributor

skliper commented Apr 9, 2020

@ejtimmon Is this design change an impact on the training slides? Might be worth a state diagram and a couple bullets...

@jphickey
Copy link
Contributor Author

jphickey commented Apr 9, 2020

It should not change the perf log feature from a user standpoint. One still sends the same command to start logging, set the trigger, and stop logging, and then the dump file appears.

The only thing that a user might care about is that CFE_ES_PerfLogAdd() might cause a task switch because it now uses a mutex. This makes it less deterministic. Of course this is only a risk when using the perf log (i.e. no chance of this happening until you send the start command).

@skliper
Copy link
Contributor

skliper commented Apr 9, 2020

I understand it's not a user impact, which is why I'm not requesting user's guide updates. We are required to maintain current design documentation, and this is a significant design change.

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 9, 2020
@astrogeco
Copy link
Contributor

CCB Fast Track - Approved, @skliper should we open a ticket to address your documentation comment?

@skliper
Copy link
Contributor

skliper commented Apr 9, 2020

cFE design documentation isn't currently in the repo, so more like an action item. I'm suggesting a slide or two overview in the context of ES covering the new background task and work it performs.

@astrogeco astrogeco changed the base branch from master to integration-candidate April 9, 2020 22:40
@astrogeco astrogeco added IC - 20200401 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 9, 2020
@astrogeco astrogeco merged commit 04f1fdd into nasa:integration-candidate Apr 9, 2020
@jphickey jphickey deleted the fix-456-perflog-race branch May 1, 2020 17:32
@skliper skliper linked an issue May 12, 2020 that may be closed by this pull request
@skliper skliper added this to the 6.8.0 milestone Jun 1, 2020
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.

CFE_ES_PerfLogAdd needs better mutual exclusion ES PerfLog dumper thread should be persistent
3 participants