Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

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
178368 876 130604 309848 4ba58 nuttx
after
text data bss dec hex filename
178120 876 130212 309208 4b7d8 nuttx

szie change -248

Testing

Build Host:

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

Arch: xtensa(./tools/configure.sh -E esp32s3-devkit:smp)

@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Sep 26, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 26, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

Overall: The PR description mostly meets NuttX requirements but has some areas for improvement.

Strengths:

  • Clear problem statement: The "Why" section clearly explains the problem with using g_current_regs and the benefits of removing it.
  • Concise impact description: The "Impact" section outlines the changes and their implications on functionality, build size, and interrupt handling.
  • Testing information: Provides build host and target details.

Areas for Improvement:

  • Missing Information:
    • Related Issues: Even if there are no directly related issues, mentioning this explicitly helps reviewers.
    • Testing Logs: The template requests "before" and "after" logs, which are missing. Provide evidence of successful testing.
  • "Impact" Section Specificity:
    • User Impact: While you state "NO," it's best to explicitly confirm users won't need to adapt (e.g., "No user-facing changes").
    • Build Process: Similarly, rephrase "NO" to "No changes to the build process itself."
    • Other Impacts: Since you're changing core context switching, consider if there might be subtle impacts on compatibility or edge-case behavior worth mentioning.
  • Code Clarity:
    • The description mentions "optimizes signal handling logic." Briefly elaborate on how signal handling is improved by this change.

Recommendation:

Address the missing information and clarify the points mentioned above. Providing concrete testing logs demonstrating the improvement (e.g., reduced context switch time) would significantly strengthen the PR.

@tmedicci
Copy link
Contributor

I will test it in our internal CI. Please don't merge it yet.

@xiaoxiang781216
Copy link
Contributor

@hujun260 please rebase the change to fix the conflict.

@github-actions github-actions bot added the Arch: arm Issues related to ARM (32-bit) architecture label Sep 27, 2024
@github-actions github-actions bot removed the Arch: arm Issues related to ARM (32-bit) architecture label Sep 27, 2024
@xiaoxiang781216
Copy link
Contributor

I will test it in our internal CI. Please don't merge it yet.

@tmedicci do you have test result?

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
178368     876  130604  309848   4ba58 nuttx
after
text    data     bss     dec     hex filename
178120     876  130212  309208   4b7d8 nuttx

szie change -248

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@tmedicci
Copy link
Contributor

I will test it in our internal CI. Please don't merge it yet.

@tmedicci do you have test result?

I'm sorry. I let it testing during the night. Everything seems to be fine. Please, go ahead!

@xiaoxiang781216 xiaoxiang781216 merged commit e98dd37 into apache:master Sep 27, 2024
@hujun260 hujun260 deleted the apache_14 branch September 29, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: xtensa Issues related to the Xtensa 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