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

arm: g_current_regs is only used to determine if we are in irq, #13444

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Sep 14, 2024

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
size nuttx
text data bss dec hex filename
225920 409 30925 257254 3ece6 nuttx

after
text data bss dec hex filename
225604 409 30925 256938 3ebaa nuttx

szie change -316

Testing

Build Host:

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

Target:

  • Arch: ARM ARM64

logs:
NO change

@hujun260 hujun260 force-pushed the apache_7 branch 2 times, most recently from f63fe3e to b996852 Compare September 18, 2024 09:52
@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

While the provided summary gives a high-level idea of the change, it lacks some important details.

Here's a breakdown of what's missing and how to improve the PR:

Summary:

  • Clarity on the "Why": Instead of just stating functionalities were removed, explain the benefit of removing them. Why is reducing context switch time important? What kind of performance improvement does it bring?
  • Specificity on "What": Which functionalities were removed exactly? This needs to be crystal clear.
  • Missing Information:
    • Related Issues: Are there any related NuttX issues this PR addresses? If so, provide links.
    • NuttX Apps Impact: Does this change impact any NuttX Apps? Be specific.

Impact:

  • Incomplete: You need to address all the impact points listed in the requirements. Even if the answer is "NO," state it explicitly. Don't leave any section blank. For example:
    • Is a new feature added? NO
    • Is an existing feature changed? YES (Please describe how the removal of g_current_regs functionality impacts existing features)
    • Impact on the user: NO (or YES if users need to be aware of anything)
    • ...and so on for all the impact points.

Testing:

  • Insufficient Information:
    • Build Hosts: Specify the operating system, CPU architecture, and compiler used for testing.
    • Targets: Be specific about the architecture (e.g., ARM Cortex-M4) and board configuration used.
  • Missing Logs:
    • Provide actual testing logs from before and after the change. This helps reviewers understand how the code behaved before and after your changes. Include relevant logs that demonstrate the performance improvement (context switch time).

Overall:

The PR is incomplete and needs more details to meet the NuttX requirements. Provide the missing information and address all the impact points clearly. Detailed testing information, including logs, is crucial for reviewers to evaluate your changes effectively.

@hujun260
Copy link
Contributor Author

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

While the provided summary gives a high-level idea of the change, it lacks some important details.

Here's a breakdown of what's missing and how to improve the PR:

Summary:

  • Clarity on the "Why": Instead of just stating functionalities were removed, explain the benefit of removing them. Why is reducing context switch time important? What kind of performance improvement does it bring?

  • Specificity on "What": Which functionalities were removed exactly? This needs to be crystal clear.

  • Missing Information:

    • Related Issues: Are there any related NuttX issues this PR addresses? If so, provide links.
    • NuttX Apps Impact: Does this change impact any NuttX Apps? Be specific.

Impact:

  • Incomplete: You need to address all the impact points listed in the requirements. Even if the answer is "NO," state it explicitly. Don't leave any section blank. For example:

    • Is a new feature added? NO
    • Is an existing feature changed? YES (Please describe how the removal of g_current_regs functionality impacts existing features)
    • Impact on the user: NO (or YES if users need to be aware of anything)
    • ...and so on for all the impact points.

Testing:

  • Insufficient Information:

    • Build Hosts: Specify the operating system, CPU architecture, and compiler used for testing.
    • Targets: Be specific about the architecture (e.g., ARM Cortex-M4) and board configuration used.
  • Missing Logs:

    • Provide actual testing logs from before and after the change. This helps reviewers understand how the code behaved before and after your changes. Include relevant logs that demonstrate the performance improvement (context switch time).

Overall:

The PR is incomplete and needs more details to meet the NuttX requirements. Provide the missing information and address all the impact points clearly. Detailed testing information, including logs, is crucial for reviewers to evaluate your changes effectively.

i update the pr

@github-actions github-actions bot added the Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. label Sep 19, 2024
@hujun260 hujun260 force-pushed the apache_7 branch 4 times, most recently from ab6070b to 3a66943 Compare September 23, 2024 01:27
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: x86_64 Issues related to the x86_64 architecture Area: Board support Board support issues labels Sep 23, 2024
@xiaoxiang781216
Copy link
Contributor

please fix conflict, @hujun260

Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

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
size nuttx
   text    data     bss     dec     hex filename
 225920     409   30925  257254   3ece6 nuttx

after
   text    data     bss     dec     hex filename
 225604     409   30925  256938   3ebaa nuttx

 szie change -316

Signed-off-by: hujun5 <[email protected]>
with other functionalities removed.

Signed-off-by: hujun5 <[email protected]>
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

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

@hujun260
I noticed that spresense:smp and raspberrypi-pico:smp do not boot with this PR.

@hujun260
Copy link
Contributor Author

@hujun260 I noticed that spresense:smp and raspberrypi-pico:smp do not boot with this PR.

ok i'll look into this issue

hujun260 added a commit to hujun260/nuttx that referenced this pull request Sep 25, 2024
…basis for judging whether to call restore_critical_section.

This commit fixes the regression from apache#13444

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

#13598 fix this issue

masayuki2009 pushed a commit that referenced this pull request Sep 25, 2024
…basis for judging whether to call restore_critical_section.

This commit fixes the regression from #13444

Signed-off-by: hujun5 <[email protected]>
masayuki2009 pushed a commit to masayuki2009/incubator-nuttx that referenced this pull request Sep 25, 2024
…basis for judging whether to call restore_critical_section.

This commit fixes the regression from apache#13444

Signed-off-by: hujun5 <[email protected]>
@hujun260 hujun260 deleted the apache_7 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: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: x86_64 Issues related to the x86_64 architecture Area: Board support Board support issues Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants