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

sched/pthread: There is no need to use sched_[un]lock #14089

Merged
merged 6 commits into from
Oct 12, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Oct 11, 2024

Summary

purpose:
1 sched_lock is very time-consuming, and reducing its invocations can improve performance. 2 sched_lock is prone to misuse, and narrowing its scope of use is to prevent people from referencing incorrect code and using it

test:
We can use qemu for testing.
compiling
make distclean -j20; ./tools/configure.sh -l qemu-armv8a:nsh_smp ;make -j20 running
qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic -machine virt,virtualization=on,gic-version=3 -net none -chardev stdio,id=con,mux=on -serial chardev:con -mon chardev=con,mode=readline -kernel ./nuttx

We have also tested this patch on other ARM hardware platforms.

Impact

sched

Testing

ci

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small Arch: arm Issues related to ARM (32-bit) architecture Arch: mips Issues related to the MIPS architecture Arch: renesas Issues related to the Renesas chips Arch: z80 Issues related to the Z80 architecture Board: arm Board: sparc Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Oct 11, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

The provided PR description lacks sufficient information to determine if it meets NuttX requirements. Here's a breakdown:

What's Missing & Why It's Important:

  • Summary:

    • Vague Description: "sched_lock is time-consuming" is not specific enough. What code uses it excessively? How is the scope narrowed? Concrete examples are crucial.
    • Missing Technical Details: How does the change exactly work? Code examples or a clear explanation of the modification are needed.
    • No Issue References: Linking to related issues helps reviewers understand the context and prior discussions.
  • Impact:

    • All sections are empty! This is the most critical part. Reviewers need to understand the potential consequences of this PR:
      • User Impact: Will applications break? Do configurations need changes?
      • Build Impact: Are new dependencies introduced? Do build flags change?
      • Hardware Impact: Are specific architectures or boards affected?
      • Security Impact: Could this change introduce vulnerabilities?
      • Compatibility: Will older applications or kernels still work?
  • Testing:

    • Build Hosts: List all operating systems, CPUs, and compilers used for testing.
    • Targets: Be specific about the boards and configurations tested beyond "other ARM platforms."
    • Logs: The provided logs are placeholders. Include actual logs demonstrating the issue before the change and the improvement afterward.

How to Improve:

  1. Be Specific: Use concrete examples, code snippets, and clear explanations.
  2. Address All Impact Points: Even if the answer is "NO", briefly explain why.
  3. Provide Real Test Data: Logs should demonstrate the problem being solved and the effectiveness of the solution.

Example for "Impact" (assuming no user-facing changes):

## Impact

* Is new feature added? Is existing feature changed? NO - This optimizes existing code.
* Impact on user (will user need to adapt to change)? NO - Internal change, no user impact expected.
* Impact on build (will build process change)? NO - No new dependencies or build flags.
* Impact on hardware (will arch(s) / board(s) / driver(s) change)?  NO - Change is architecture-agnostic.
* Impact on documentation (is update required / provided)?  MAYBE - If the usage of `sched_lock` is now restricted, documentation should reflect this.
* Impact on security (any sort of implications)? NO - Performance improvement, no security implications anticipated.
* Impact on compatibility (backward/forward/interoperability)? NO - Existing applications should continue to function as before. 
* Anything else to consider? N/A

By addressing these points, you'll significantly increase the chances of your PR being reviewed and merged quickly.

sched/pthread/pthread_create.c Outdated Show resolved Hide resolved
@hujun260 hujun260 force-pushed the apache_9 branch 3 times, most recently from 3a971a9 to e13603f Compare October 11, 2024 09:10
@anchao
Copy link
Contributor

anchao commented Oct 12, 2024

Please be as cautious as possible when making changes to sched_lock(). In my impression, which has brought regression many times. eg: #14032 @zyfeier waste more time debugging for you regression. Please try to fully verify similar submissions in internal products.

@hujun260
Copy link
Contributor Author

hujun260 commented Oct 12, 2024

Please be as cautious as possible when making changes to sched_lock(). In my impression, which has brought regression many times. eg: #14032 @zyfeier waste more time debugging for you regression. Please try to fully verify similar submissions in internal products.

Thank you for the reminder. This change has been in our local code repository for nearly a year. The bug you mentioned was addressed along with my initial modification. However, subsequently, some individuals pick the community code and resolved the conflicts incorrectly, leading to the reoccurrence of the issue locally.

@anchao anchao merged commit 4c1c051 into apache:master Oct 12, 2024
37 checks passed
@hujun260 hujun260 deleted the apache_9 branch October 13, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: mips Issues related to the MIPS architecture Arch: renesas Issues related to the Renesas chips Arch: z80 Issues related to the Z80 architecture Area: OS Components OS Components issues Board: arm Board: sparc Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants