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: g_current_regs is only used to determine if we are in irq #13561

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Sep 20, 2024

Why

When a context switch occurs currently, the context before the interrupt is fetched from the global variable g_current_regs and then stored in the corresponding tcb->xcp.regs. Subsequently, the new context is assigned to g_current_regs. When returning from the interrupt context, the context is retrieved from g_current_regs and used.

In reality, we do not necessarily require this intermediate variable g_current_regs to accomplish a context switch. Eliminating it avoids multiple assignments, especially during multiple context switches within interrupts, which can lead to repeated assignments.

All we need is to work directly with the context of the currently running task. This approach enhances the speed of context switching, reduces the code base, and optimizes signal handling logic. By simply leveraging the context of the active task, we can streamline the process and improve overall efficiency.

reason:
by doing this we can reduce context switch time,
When we exit from an interrupt handler, we directly use tcb->xcp.regs
Missing Information:
Related Issues: none
NuttX Apps Impact: none

Impact
Is a new feature added? NO
Is an existing feature changed? yes
g_current_regs is only used to determine if we are in irq with other functionalities removed.
We need to use up_interrupt_context for interrupt identification instead of relying on up_current_regs for that purpose.

before
text data bss dec hex filename
138805 337 24256 163398 27e46 nuttx

after
text data bss dec hex filename
138499 337 24240 163076 27d04 nuttx

size -322

Testing
Build Host:

OS: Ubuntu 20.04
CPU: x86_64
Compiler: GCC 9.4.0
Target:

Arch: riscv
logs:
NO change

       with other functionalities removed.

reason:
  by doing this we can reduce context switch time,
  When we exit from an interrupt handler, we directly use tcb->xcp.regs

before
   text    data     bss     dec     hex filename
 138805     337   24256  163398   27e46 nuttx

after
   text    data     bss     dec     hex filename
 138499     337   24240  163076   27d04 nuttx

 szie change -322
Signed-off-by: hujun5 <[email protected]>
@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 Sep 20, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 20, 2024

[Experimental Bot, please feedback here]

The PR Summary and Testing sections appear to meet NuttX requirements, but the Impact section needs more detail.

Here's a breakdown and suggestions for improvement:

Good:

  • Summary: Clearly explains the "why", "what" and "how" of the change. The size reduction is a good quantitative measure of the impact.
  • Testing: Specifies the testing environment ("ostest") and provides evidence of testing. Ideally, the "before" and "after" logs would highlight the specific area improved by this change (e.g., context switch timings).

Needs Improvement:

  • Impact: Simply stating "riscv" is insufficient. You need to elaborate on how this change impacts RISC-V:
    • Specific architectures/boards affected: Does this impact all RISC-V targets or just specific ones?
    • User impact: Will users experience any noticeable differences? Faster context switching might be an improvement worth mentioning.
    • Build impact: While the code size reduction is noted, are there any changes to build configuration or dependencies?
    • Documentation: Does any documentation need updating to reflect this change in g_current_regs behavior?

Example of Improved Impact Section

## Impact

* **Is new feature added? Is existing feature changed?**  Existing feature (interrupt handling) optimized.
* **Impact on user:** Potentially improved performance due to faster interrupt context switching on RISC-V targets. This impact may be most noticeable in applications with frequent interrupts.
* **Impact on build:** Code size reduction of 322 bytes on RISC-V targets. No changes to build process or dependencies.
* **Impact on hardware:** Affects all RISC-V architectures supported by NuttX. No impact on specific boards or drivers.
* **Impact on documentation:** The behavior of `g_current_regs` on RISC-V has changed, so documentation may require updates to reflect its reduced functionality. 
* **Impact on security, compatibility:**  None foreseen. 

By providing specific details in the Impact section, you make it easier for reviewers to assess the risks and benefits of your change and ensure it aligns with NuttX standards.

@xiaoxiang781216 xiaoxiang781216 merged commit c5ecc49 into apache:master Sep 20, 2024
29 checks passed
@masayuki2009
Copy link
Contributor

@hujun260 @xiaoxiang781216

I noticed that rv-virt:knsh crashes now.

$ /home/ishikawa/opensource/QEMU/qemu-9.0.0/build/qemu-system-riscv32 -semihosting -nographic -cpu rv32 -M virt,aclint=on -kernel nuttx

OpenSBI v1.4
   ____                    _____ ____ _____
  / __ \                  / ____|  _ \_   _|
 | |  | |_ __   ___ _ __ | (___ | |_) || |
 | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
 | |__| | |_) |  __/ | | |____) | |_) || |_
  \____/| .__/ \___|_| |_|_____/|____/_____|
        | |
        |_|

Platform Name             : riscv-virtio,qemu
Platform Features         : medeleg
Platform HART Count       : 1
Platform IPI Device       : aclint-mswi
Platform Timer Device     : aclint-mtimer @ 10000000Hz
Platform Console Device   : semihosting
Platform HSM Device       : ---
Platform PMU Device       : ---
Platform Reboot Device    : syscon-reboot
Platform Shutdown Device  : syscon-poweroff
Platform Suspend Device   : ---
Platform CPPC Device      : ---
Firmware Base             : 0x80000000
Firmware Size             : 319 KB
Firmware RW Offset        : 0x40000
Firmware RW Size          : 63 KB
Firmware Heap Offset      : 0x47000
Firmware Heap Size        : 35 KB (total), 2 KB (reserved), 9 KB (used), 24 KB (free)
Firmware Scratch Size     : 4096 B (total), 184 B (used), 3912 B (free)
Runtime SBI Version       : 2.0

Domain0 Name              : root
Domain0 Boot HART         : 0
Domain0 HARTs             : 0*
Domain0 Region00          : 0x00100000-0x00100fff M: (I,R,W) S/U: (R,W)
Domain0 Region01          : 0x02000000-0x0200ffff M: (I,R,W) S/U: ()
Domain0 Region02          : 0x80040000-0x8004ffff M: (R,W) S/U: ()
Domain0 Region03          : 0x80000000-0x8003ffff M: (R,X) S/U: ()
Domain0 Region04          : 0x0c400000-0x0c5fffff M: (I,R,W) S/U: (R,W)
Domain0 Region05          : 0x0c000000-0x0c3fffff M: (I,R,W) S/U: (R,W)
Domain0 Region06          : 0x00000000-0xffffffff M: () S/U: (R,W,X)
Domain0 Next Address      : 0x80200000
Domain0 Next Arg1         : 0x87e00000
Domain0 Next Mode         : S-mode
Domain0 SysReset          : yes
Domain0 SysSuspend        : yes

Boot HART ID              : 0
Boot HART Domain          : root
Boot HART Priv Version    : v1.12
Boot HART Base ISA        : rv32imafdch
Boot HART ISA Extensions  : sstc,zicntr,zihpm,zicboz,zicbom
Boot HART PMP Count       : 16
Boot HART PMP Granularity : 2 bits
Boot HART PMP Address Bits: 32
Boot HART MHPM Info       : 16 (0x0007fff8)
Boot HART MIDELEG         : 0x00001666
Boot HART MEDELEG         : 0x00f0b509
ABC[    0.018000] riscv_exception: EXCEPTION: Store/AMO page fault. MCAUSE: 0000000f, EPC: 80200120, MTVAL: c08810f1
[    0.018000] riscv_exception: Segmentation fault in PID 2: /system/bin/init

@lupyuen
Copy link
Member

lupyuen commented Sep 21, 2024

Yep I see the same error in the Daily Auto Test for rv-virt:knsh:
https://github.com/lupyuen/nuttx-riscv64/actions/runs/10967966985/job/30458585413

$ tools/configure.sh rv-virt:knsh
...
ABC[    0.013000] riscv_exception: EXCEPTION: Store/AMO page fault. MCAUSE: 0000000f, EPC: 80200120, MTVAL: c08810f1
[    0.013000] riscv_exception: Segmentation fault in PID 2: /system/bin/init

rv-virt:knsh64 fails to boot:
https://github.com/lupyuen/nuttx-riscv64/releases/tag/qemu-riscv-knsh64-2024-09-21

ABC

Milk-V Duo S SBC (SG2000) also fails to boot:
https://github.com/lupyuen/nuttx-sg2000/releases/tag/nuttx-sg2000-2024-09-21

Starting kernel ...
ABC

@xiaoxiang781216
Copy link
Contributor

@hujun260 is fixing this issue. @lupyuen could you migrate your automation test to official repo?

hujun260 added a commit to hujun260/nuttx that referenced this pull request Sep 21, 2024
… switch is required

This commit fixes the regression from apache#13561

Change-Id: Ifb0623c0c60f7da55c762926e93cd1708c421ce6
hujun260 added a commit to hujun260/nuttx that referenced this pull request Sep 21, 2024
… switch is required

This commit fixes the regression from apache#13561

Signed-off-by: hujun5 <[email protected]>
@hujun260
Copy link
Contributor Author

@hujun260 is fixing this issue. @lupyuen could you migrate your automation test to official repo?

#13564

hujun260 added a commit to hujun260/nuttx that referenced this pull request Sep 21, 2024
… switch is required

This commit fixes the regression from apache#13561

Signed-off-by: hujun5 <[email protected]>
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.

6 participants