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: wrap logging buffer writes in critical section in Windows logging implementation #1039

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ydhuang28
Copy link
Contributor

Wrap logging buffer writes in critical section in Windows logging implementation

Description

WinSim (by virtue of Windows behavior) does not change task priority correctly/immediately. previously the logging buffer writes were protected by raising and lower task priority, which doesn't protect the buffer properly. This fix addresses that.

Test Steps

n/a

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

n/a

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ydhuang28 ydhuang28 requested a review from Skptak July 17, 2023 18:19
* reading from the buffer). */
xCurrentTask = GetCurrentThread();
iOriginalPriority = GetThreadPriority( xCurrentTask );
SetThreadPriority( GetCurrentThread(), THREAD_PRIORITY_TIME_CRITICAL );
Copy link
Member

Choose a reason for hiding this comment

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

A naive question: Why wasn't this able to achieve the same thing as taskENTER_CRITICAL()?

kar-rahul-aws
kar-rahul-aws previously approved these changes Jul 20, 2023
ActoryOu
ActoryOu previously approved these changes Jul 20, 2023
@ActoryOu ActoryOu requested review from ActoryOu and removed request for ActoryOu July 20, 2023 06:25
@ydhuang28 ydhuang28 requested a review from a team as a code owner July 24, 2023 23:30
@Skptak
Copy link
Member

Skptak commented Jan 23, 2024

/bot run formatting

@github-actions github-actions bot dismissed stale reviews from kar-rahul-aws and ActoryOu via 49e66ea January 23, 2024 22:11
@aggarg
Copy link
Member

aggarg commented Jan 24, 2024

@ydhuang28 I think taskENTER_CRITICAL will not prevent prvWin32LoggingThread from accessing xLogStreamBuffer as this is a Windows Thread and not known to FreeRTOS Kernel. What is the reason for this change? Are you facing any issue?

every-breaking-wave pushed a commit to every-breaking-wave/FreeRTOS that referenced this pull request Nov 15, 2024
* Adding SMP coverity example

* Add coverity scan flow

* Fix format

* Update README.md

* Code review suggestions

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>

---------

Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
Co-authored-by: Ubuntu <ubuntu@ip-172-31-34-245.ap-northeast-1.compute.internal>
Co-authored-by: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com>
Co-authored-by: Gaurav Aggarwal <aggarg@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants