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: replace sync pause with async pause for nxtask_restart #13735

Merged
merged 1 commit into from
Oct 5, 2024

Conversation

hujun260
Copy link
Contributor

Summary

In the kernel, we are planning to remove all occurrences of up_cpu_pause as one of the steps to simplify the implementation of critical sections. The goal is to enable spin_lock_irqsave to encapsulate critical sections, thereby facilitating the replacement of critical sections(big lock) with smaller spin_lock_irqsave(small lock)

Impact

nxtask_restart

Testing

Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ 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

reason:
In the kernel, we are planning to remove all occurrences of up_cpu_pause as one of the steps to
simplify the implementation of critical sections. The goal is to enable spin_lock_irqsave to encapsulate critical sections,
thereby facilitating the replacement of critical sections(big lock) with smaller spin_lock_irqsave(small lock)

Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ 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

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Sep 29, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 29, 2024

[Experimental Bot, please feedback here]

The PR summary is missing some key information required by the NuttX PR requirements. Let's break it down:

What's Missing in the Summary:

  • Why is this change necessary? The summary mentions simplification and enabling spin_lock_irqsave. It needs to explain the benefits clearly. Why is removing up_cpu_pause important? What are the downsides of the current approach? (e.g., performance issues, complexity making code harder to maintain)
  • What functional part of the code is being changed? The summary mentions nxtask_restart. Be more specific. Which files/modules are affected? Is it just the implementation of nxtask_restart or are there changes in how it's used elsewhere?
  • How does the change exactly work? Provide a concise explanation of the technical changes made to remove up_cpu_pause and how it helps achieve the goal of using spin_lock_irqsave.

What's Missing in the Impact Section:

  • Incomplete information: The impact section only mentions "nxtask_restart." This is not sufficient. You must address all the points listed in the requirements. Consider these questions for each point:
    • New feature? Existing feature changed? - Is this just refactoring or does it alter functionality?
    • Impact on the user? - Will applications need to change?
    • Impact on the build? - Will build scripts need modification? New dependencies?
    • Impact on hardware? - Are any specific architectures or boards affected more than others?
    • Impact on documentation? - Does any documentation need updating to reflect the changes?
    • Impact on security? - Could this change introduce any potential security vulnerabilities?
    • Impact on compatibility? - Will this break compatibility with older NuttX versions or applications?

What's Missing in the Testing Section:

  • Insufficient Testing Details:
    • Build Hosts: List all operating systems, CPU architectures, and compiler versions used for testing.
    • Targets: Specify all architectures (including simulators), boards, and configurations tested.
    • Testing Logs: You've provided placeholders. Include the actual logs from before and after the change. Highlight the relevant differences that demonstrate the fix or improvement.

How to Improve the PR:

  1. Expand the Summary: Provide the missing details about the "why," "what," and "how" of your changes.
  2. Complete the Impact Assessment: Thoroughly address each point in the impact section, providing specific details and explanations.
  3. Provide Comprehensive Testing Information: List all build hosts and target environments used. Include relevant excerpts from your testing logs to demonstrate the change's effectiveness.

By providing this additional information, you will make it much easier for the NuttX maintainers to review and merge your PR.

@xiaoxiang781216 xiaoxiang781216 changed the title sched: replace sync pause with async pause for sched: replace sync pause with async pause for nxtask_restart Sep 30, 2024
@xiaoxiang781216 xiaoxiang781216 merged commit 5df3625 into apache:master Oct 5, 2024
29 checks passed
@hujun260 hujun260 deleted the apache_9 branch October 10, 2024 05:32
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: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants