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.

@hujun260 hujun260 force-pushed the apache_20 branch 2 times, most recently from e120df4 to ea9129f Compare November 21, 2024 08:51
@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
arch/arm/src/armv6-m/arm_sigdeliver.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/arm_sigdeliver.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-m/arm_sigdeliver.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-r/arm_sigdeliver.c Outdated Show resolved Hide resolved
arch/arm/src/armv8-m/arm_sigdeliver.c Outdated Show resolved Hide resolved
arch/arm/src/common/arm_internal.h Outdated Show resolved Hide resolved
arch/arm/src/armv8-r/arm_sigdeliver.c Outdated Show resolved Hide resolved
arch/arm/src/armv8-m/arm_svcall.c Outdated Show resolved Hide resolved
arch/arm/src/armv8-m/arm_svcall.c Outdated Show resolved Hide resolved
@hujun260 hujun260 force-pushed the apache_20 branch 5 times, most recently from b1d9bfc to ed48c90 Compare November 26, 2024 13:10
@github-actions github-actions bot added Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. and removed Size: L The size of the change in this PR is large labels Nov 26, 2024
reason:
up_set_current_regs initially had two functions:

1: To mark the entry into an interrupt state.
2: To record the context before an interrupt/exception. If we switch to
   a new task, we need to store the upcoming context regs by calling up_set_current_regs(regs).

Currently, we record the context in other ways, so the second function is obsolete.
Therefore, we need to rename up_set_current_regs to better reflect its actual meaning,
which is solely to mark an interrupt.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
reason:
We hope to keep g_running_tasks valid forever.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Nov 26, 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