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_mutex:add deadlock assert #13456

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Conversation

anjiahao1
Copy link
Contributor

Summary

add deadlock check

Impact

pthread mutex

Testing

sim & qmue ust pthread mutex

@nuttxpr
Copy link

nuttxpr commented Sep 19, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Here's why and how to improve it:

Missing Information:

  • Summary:
    • Why: What problem does the deadlock check solve? Be specific.
    • What: Which part of the pthread mutex code is changed?
    • How: Provide a technical explanation of the deadlock detection mechanism.
    • Issues: Link any relevant NuttX or NuttX Apps issues.
  • Impact:
    • New Feature/Existing Change: Clearly state if this is new or modifies existing functionality.
    • User Impact: Will users notice any changes in behavior? Will they need to modify their code?
    • Build Impact: Are there new build options, dependencies, or configurations?
    • Hardware Impact: Does this affect specific architectures, boards, or drivers?
    • Documentation Impact: Does this require documentation updates? If so, are they included?
    • Security Impact: Does this introduce any potential security vulnerabilities or fix existing ones?
    • Compatibility Impact: Does this break compatibility with older NuttX versions, applications, or hardware?
  • Testing:
    • Build Host Details: Provide specifics about your development environment (OS version, compiler version, etc.).
    • Target Details: Provide the full details of the targets you tested on (e.g., sim:qemu-arm, board:stm32f4discovery).
    • Complete Logs: The provided logs are insufficient. Include relevant portions of build logs and runtime logs that demonstrate the issue before the change and the correct behavior after the change.

Example Improvements:

## Summary

This PR introduces a deadlock detection mechanism to the pthread mutex implementation to identify and prevent potential deadlocks during development. The mechanism identifies circular dependencies in mutex acquisition order and logs an error message when a deadlock is detected.

This addresses [NuttX Issue #123](https://github.com/apache/nuttx/issues/123) which reported intermittent deadlocks in the networking stack.

## Impact

* **New Feature:** Deadlock detection for pthread mutexes.
* **User Impact:** Users may see deadlock error messages in their logs if their applications have mutex acquisition order problems. No code changes are required.
* **Build Impact:** No changes to the build process.
* **Hardware Impact:** No impact on specific architectures, boards, or drivers.
* **Documentation Impact:** Updated the pthread mutex documentation to describe the new deadlock detection feature.
* **Security Impact:** None.
* **Compatibility Impact:** None. 

## Testing

I confirm that changes are verified on the following setup:

* **Build Host:** Ubuntu 20.04, GCC 9.4.0
* **Targets:** 
    * `sim:qemu-arm`
    * `board:stm32f4discovery:nsh` 

**Testing logs before change:**

[logs showing deadlock occurring]


**Testing logs after change:**

[timestamp] ERROR: pthread_mutex_lock: Deadlock detected!
[timestamp] ERROR: Mutex 0x12345678 already held by thread 1234
... [stack trace showing mutex acquisition order] ...

By providing specific details, you make it much easier for reviewers to understand, evaluate, and merge your PR.

@xiaoxiang781216 xiaoxiang781216 merged commit 112cc08 into apache:master Sep 20, 2024
29 checks passed
@anchao
Copy link
Contributor

anchao commented Sep 22, 2024

You should return the EDEADLK instead of assert, @xiaoxiang781216
https://pubs.opengroup.org/onlinepubs/007908799/xsh/pthread_mutex_lock.html

[EDEADLK]
The current thread already owns the mutex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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