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

arm64: syscall SYS_switch_context and SYS_restore_context use 0 para #14911

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Nov 23, 2024

Summary

arm64: 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)

size nuttx
before
text data bss dec hex filename
504644 54968 81085 640697 9c6b9 nuttx

after
text data bss dec hex filename
504360 54968 81085 640413 9c59d nuttx

size reduce -284

Impact

arm64

Testing

ostest
qemu-armv8a:nsh_smp

…para

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

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small labels Nov 23, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 23, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although some sections could be improved. Here's a breakdown:

Strengths:

  • Clear Summary of "What" and "Why": The summary explains the change (using zero-parameter syscalls for context switching) and the rationale (simplification).
  • Quantifiable Impact: The impact section shows a reduction in binary size, which is a concrete benefit.
  • Testing Evidence: Testing logs are included (although the content is just "ostest" and "qemu-armv8a:nsh_smp," which needs more detail). The mention of the target architecture (arm64/qemu-armv8a) is good.

Weaknesses & Areas for Improvement:

  • Missing Issue References: If this relates to a specific issue in the NuttX or NuttX Apps repositories, those should be linked.
  • Incomplete "How" in Summary: The summary mentions what syscalls are changed but lacks detail on how the change is implemented. More details on the mechanism of switching to zero-parameter syscalls would be beneficial.
  • Impact Section Needs More "YES/NO" Clarity: The impact section is mostly just "arm64" and size reduction. Explicitly stating "YES" or "NO" for each impact category (user, build, hardware, documentation, security, compatibility) with explanations would make the impact clearer. For example:
    • Impact on user: NO
    • Impact on build: NO
    • Impact on hardware: YES (affects arm64 architecture)
    • ...and so on.
  • Insufficient Testing Logs: "ostest" and "qemu-armv8a:nsh_smp" are not detailed enough. The logs should show the relevant parts of the test output demonstrating correct functionality both before and after the change. Ideally, there should be tests specifically targeting the context switching functionality.
  • Missing Build Host Information: The testing section should specify the build host OS, CPU, and compiler used.
  • "Anything else to consider?" is unanswered: This section is meant to capture any unusual edge cases or potential issues that reviewers should be aware of.

Recommendation: Revise the PR to address the weaknesses listed above. Providing more detail and explicit answers to all sections of the template will greatly improve clarity and make it easier for reviewers to assess the change.

@xiaoxiang781216 xiaoxiang781216 merged commit fccd908 into apache:master Nov 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm64 Issues related to ARM64 (64-bit) architecture 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.

3 participants