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

remove sched_lock in pthread_cond_broadcast #14814

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

hujun260
Copy link
Contributor

Summary

remove sched_lock in pthread_cond_broadcast
reason:
Since pthread_cond_broadcast is already protected by a mutex, even if sem_post causes a context switch, it will not affect the count of wait_count.

Impact

pthread cond_wait

Testing

ci ostest

reason:
Since pthread_cond_broadcast is already protected by a mutex,
even if sem_post causes a context switch, it will not affect the count of wait_count.

Signed-off-by: hujun5 <[email protected]>
@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 Nov 16, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 16, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is lacking in detail and completeness. Here's a breakdown:

Strengths:

  • Clear Summary of the "What": It clearly states what is being changed (removing sched_lock in pthread_cond_broadcast).
  • Provides a Reason: It explains why the change is believed to be necessary.
  • Indicates Testing: It mentions testing with ci ostest.

Weaknesses:

  • Insufficient Detail in Summary: The how is missing. How exactly does removing sched_lock work in this context? What specific code changes were made? This is crucial for reviewers.
  • Vague Impact: "pthread cond_wait" is too general. Does this impact all users of condition variables? Are there performance implications (positive or negative)? Be specific.
  • Missing Testing Details: "ci ostest" isn't enough. Which specific tests within ostest were run? What were the expected outcomes? Critically, the "before" and "after" log sections are empty. Provide concrete evidence that the change works as intended. Include the actual logs.
  • Unanswered Impact Questions: The PR completely ignores most of the required "Impact" questions. This makes it difficult to assess the broader implications of the change. Each point needs a "YES" or "NO" answer, with explanation where necessary. Even if the answer is "NO," state it explicitly.
  • Missing Build Host/Target Details: Under "Testing," specify the build host operating system, compiler, target architecture, and board configuration used for testing.

Recommendation:

The PR needs significant revision before it's ready for review. Focus on providing the missing detail, particularly in the "Impact" and "Testing" sections. Provide concrete evidence of testing and address all the required questions, even if the answer is "NO." Clear, concise, and complete information is essential for efficient and effective code review.

@xiaoxiang781216 xiaoxiang781216 merged commit 2714f1b into apache:master Nov 16, 2024
27 checks passed
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.

3 participants