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

riscv: syscall SYS_switch_context and SYS_restore_context use 0 para #14981

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

hujun260
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

riscv: 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
148021 921 26944 175886 2af0e nuttx

after
text data bss dec hex filename
147995 921 26928 175844 2aee4 nuttx

size reduce -42

Impact

risc-v

Testing

ci ostest
rv-virt:smp

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: M The size of the change in this PR is medium labels Nov 29, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 29, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although more detail would strengthen it.

Here's a breakdown of why and where it could be improved:

Strengths:

  • Clear Summary: The summary explains the what and why of the change: simplifying context switching by reducing the parameters of two syscalls. The size reduction is a nice quantifiable benefit.
  • Impact Section (Partially Addressed): It identifies the affected architecture (RISC-V).
  • Testing Information (Partially Addressed): It mentions CI, ostest, and rv-virt:smp, indicating some testing was performed.

Weaknesses & Areas for Improvement:

  • Summary (More Detail): How does removing the parameters simplify context switching? Are these parameters unused? Were they previously passed unnecessarily? This detail is crucial.
  • Impact Section (Incomplete):
    • User Impact: Even if no user changes are needed, explicitly state "NO".
    • Build Impact: Explicitly state "NO" or detail any changes.
    • Hardware Impact: While RISC-V is mentioned, be more specific. Are all RISC-V boards affected? Any particular configurations?
    • Documentation: Does this change require documentation updates? If not, state "NO".
    • Security: Explicitly state "NO" or address any potential implications.
    • Compatibility: Explicitly state "NO" if no compatibility issues are expected.
  • Testing Section (Insufficient):
    • Build Host Details: Provide details of your build host OS, CPU architecture, and compiler version.
    • Target Details: Be more specific about the RISC-V target. "rv-virt:smp" is a start, but what specific configuration? QEMU version?
    • Missing Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. Include relevant log snippets demonstrating the change's effect. Show how the context switching behavior is different (ideally improved). If size is a focus, showing size output before/after is excellent, but also demonstrate functional correctness.

Example of Improved Testing Section:

Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.3.0
* Target(s): RISC-V (QEMU 7.2.0), rv-virt:smp config

Testing logs before change:

$ size nuttx
text data bss dec hex filename
148021 921 26944 175886 2af0e nuttx

Example: Show some timing or profiling data related to context switching if applicable.


Testing logs after change:

$ size nuttx
text data bss dec hex filename
147995 921 26928 175844 2aee4 nuttx

Example: Show corresponding data after the change. Demonstrate improvement.


By addressing these weaknesses, the PR will be significantly stronger and more likely to be accepted.  Remember, providing clear and comprehensive information makes it easier for reviewers to understand and evaluate your contribution.

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

size nuttx
before
   text    data     bss     dec     hex filename
 148021     921   26944  175886   2af0e nuttx

after
   text    data     bss     dec     hex filename
 147995     921   26928  175844   2aee4 nuttx

size reduce -42

Signed-off-by: hujun5 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 3c32517 into apache:master Dec 2, 2024
16 of 17 checks passed
@hujun260 hujun260 deleted the apache_40 branch December 5, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture 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