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

arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para #14881

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hujun260
Copy link
Contributor

Summary

arch/arm: syscall SYS_switch_context and SYS_restore_context use 0 para
reason:
simplify context switch
sys_call0(SYS_switch_context)
sys_call0(SYS_restore_context)

Impact

arm

Testing

ostest
mps2-an500:nsh
qemu-armv7a:smp

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium labels Nov 20, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 20, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details. Here's a breakdown:

  • Insufficient Summary: While it states the what, it's missing the why and how. Why is simplifying the context switch necessary? What specific changes were made to achieve this? How does using 0 parameters simplify it? What were the parameters before?

  • Incomplete Impact: Simply stating "arm" is not enough. Which ARM architectures? Which boards? Does this change affect any existing drivers? Does this have performance implications? Does it change any APIs that applications might use? It needs more specifics. Address all impact categories explicitly (even if the answer is NO with a brief justification).

  • Missing Testing Details: Listing platforms is insufficient. "ostest", "mps2-an500:nsh", and "qemu-armv7a:smp" are not descriptive enough. What specific tests were run? Crucially, the testing logs are empty. The logs demonstrating the before and after behavior are mandatory for demonstrating the change's correctness and verifying its impact. Provide actual command-line invocations and their output. Also specify the Build Host details.

Example of how to improve this PR description:

Summary

arch/arm: Simplify context switching by removing parameters from SYS_switch_context and SYS_restore_context

This change removes the parameters previously required for SYS_switch_context and SYS_restore_context syscalls. These parameters were redundant as the necessary context information is already managed internally within the scheduler. This simplification reduces overhead in context switching and improves code clarity. No related NuttX issue.

Impact

  • Is new feature added? Is existing feature changed? Existing feature changed (optimization).
  • Impact on user: YES. Applications directly using these syscalls (which is unlikely as these are typically used internally) will need to be updated to remove the parameters.
  • Impact on build: NO.
  • Impact on hardware: YES. This change affects ARM architectures (ARMv7-M, ARMv7-A, Cortex-M4 tested). Tested on mps2-an500 and simulated qemu-armv7a:smp.
  • Impact on documentation: YES. The syscall documentation needs to be updated to reflect the parameter removal. A documentation update is included in this PR.
  • Impact on security: NO.
  • Impact on compatibility: Backward compatibility is broken for applications directly using these syscalls. Forward compatibility is maintained.
  • Anything else to consider: Potential performance improvements.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): ARMv7-M (sim:qemu-armv7m), ARMv7-A (sim:qemu-armv7a:smp), Cortex-M4 (mps2-an500:nsh)

Testing logs before change:

# On mps2-an500:nsh
nsh> <command to demonstrate previous behavior with parameters>
<output demonstrating previous behavior>

Testing logs after change:

# On mps2-an500:nsh
nsh> <command to demonstrate new behavior without parameters>
<output demonstrating new behavior>

# On qemu-armv7a:smp
<commands and output demonstrating SMP functionality>

# ostest output (relevant sections) before and after
<ostest output before>
<ostest output after>

By providing complete and detailed information, the reviewers can effectively evaluate the PR and ensure its quality and correctness. Remember, the goal is to make the change clear, easy to understand, and demonstrate its impact and correctness.

reason:
simplify context switch
sys_call0(SYS_switch_context)
sys_call0(SYS_restore_context)

Signed-off-by: hujun5 <[email protected]>
reason:
We hope to keep g_running_tasks valid forever.

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: M The size of the change in this PR is medium labels Nov 21, 2024
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 Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants