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

Revert "sched/wqueue: some minor improve to reduce sched_lock range" #15687

Closed
wants to merge 1 commit into from

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Jan 24, 2025

Summary

This reverts commit
fc694db0-9a9f-4fc1-a32b-26ab6aca758b
d5b1b30d-dd74-4851-bc2f-ebbe1daefb4d

reason:
may cause dead lock

Impact

wqueue

Testing

sim

This reverts commit 2779989.

reason:
may cause deadlock, fix regression from apache#15587

Signed-off-by: hujun5 <[email protected]>
@hujun260 hujun260 changed the title Revert "arch: set (*running_task)->xcp.regs to NULL when exit from irq/exception" Revert "sched/wqueue: some minor improve to reduce sched_lock range" Jan 24, 2025
@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 Jan 24, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 24, 2025

[Experimental Bot, please feedback here]

No. This PR does not meet the NuttX requirements. It is missing critical information.

Here's why:

  • Insufficient Summary: Simply stating "reverts commit ..." and providing a reason of "may cause deadlock" is inadequate. The summary needs to explain what the original commit changed, what the deadlock was, how this revert fixes it, and which functional area of the code is affected (e.g., work queues). It needs to be understandable without having to look up the original commit.
  • Vague Impact: "wqueue" is not sufficient. It needs to specify the impact on the user. Will applications using work queues behave differently? Will any APIs change? It also needs to specify the impact on the build, hardware, documentation, security, and compatibility, even if the answer is "NO" for each.
  • Missing Testing Details: "sim" is inadequate. What simulator? What test case demonstrated the deadlock before the revert and the correct behavior after the revert? Actual log output demonstrating the issue and the fix is required, not just screenshots. Build host information is also missing.

The PR author needs to provide significantly more detail for this to be considered.

@anchao
Copy link
Contributor

anchao commented Jan 24, 2025

let me fix this issue

@hujun260
Copy link
Contributor Author

let me fix this issue

ok

@anchao
Copy link
Contributor

anchao commented Jan 24, 2025

@hujun260 spin_lock already has the function of sched_lock. could the sched_lock/unlock here be deleted directly?

@@ -151,6 +152,7 @@ int work_queue_wq(FAR struct kwork_wqueue_s *wqueue,
*/

flags = spin_lock_irqsave(&wqueue->lock);
sched_lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove it

@hujun260 hujun260 closed this Jan 24, 2025
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