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

pthread: remove enter_critical_section in pthread_mutex #15126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hujun260
Copy link
Contributor

Summary

pthread: remove enter_critical_section in pthread_mutex

reason:
We would like to replace the critical section with a small lock.

This PR requires #14578 to be merged first.

Impact

pthread

Testing

ci ostest

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Dec 11, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 11, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a reason for the change and identifies a related PR, it lacks crucial information.

Here's what's missing:

  • Insufficient Summary: The summary needs to be more detailed. It should explain how the change works (the mechanism of replacing the critical section) and what functional part of the code is affected beyond just "pthread."
  • Missing Impact Details: Simply stating "pthread" is not enough. The impact section requires YES/NO answers for all listed items (user impact, build impact, hardware impact, documentation, security, compatibility). If any answer is YES, a description is mandatory. For example, even if the answer to "Impact on user" is NO, you should still explicitly state "NO".
  • Inadequate Testing Information: "ci ostest" is insufficient. The requirements ask for:
    • Specific build host details (OS, CPU, compiler version).
    • Specific target details (architecture, board, configuration).
    • Actual testing logs before and after the change, not just a reference to CI. While CI is important, the PR should demonstrate local verification.

Therefore, the PR needs substantial revision to meet the stated requirements before it can be properly reviewed.

@@ -92,7 +98,7 @@ static void pthread_mutex_remove(FAR struct pthread_mutex_s *mutex)
FAR struct pthread_mutex_s *prev;
irqstate_t flags;

flags = enter_critical_section();
flags = spin_lock_irqsave(&g_mutex_spinlock);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need lock? is mhead list only modified by one thread(rtcb)?

* Public Data
****************************************************************************/

static spinlock_t g_mutex_spinlock = SP_UNLOCKED;
Copy link
Contributor

Choose a reason for hiding this comment

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

move to tcb_s? only need when CONFIG_PTHREAD_MUTEX_UNSAFE isn't true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

need global lock here, for the instance of mutex might be accessed by multiple threads

image

reason:
We would like to replace the critical section with a small lock.

Signed-off-by: hujun5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants