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.

Sign up for GitHub

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

Jump to bottom

arch: inline up_switch_context,in arm arm64 #13644

Merged
merged 2 commits into from
Sep 28, 2024
Merged

arch: inline up_switch_context,in arm arm64 #13644

merged 2 commits into from
Sep 28, 2024

Conversation

hujun260
Copy link
Contributor

Summary

inline up_switch_context,in arm arm64
when a context switch occurs, up_switch_context is executed. In order to reduce the time taken for context switching, we inline the up_switch_context function.

Impact

arm,arm64

Testing

ostest

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small labels Sep 26, 2024
anchao
anchao reviewed Sep 26, 2024
View reviewed changes
arch/arm/include/irq.h
@@ -76,6 +77,17 @@

#ifndef __ASSEMBLY__

#ifndef up_switch_context
#define up_switch_context(tcb, rtcb) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use inline function?

Copy link
Contributor Author

@hujun260 hujun260 Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use inline, we need to see the definition of struct tcb_s, but irq.h can't include nuttx/sched.h

arch/arm/src/common/arm_switchcontext.c Outdated Show resolved Hide resolved
@nuttxpr
Copy link

nuttxpr commented Sep 26, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX Requirements.

Missing Information:

  • Summary:
    • Lacks a clear explanation of why inlining up_switch_context is necessary. What specific performance benefits are expected?
    • Doesn't specify which functional part of the code is being modified (e.g., scheduler, context switching mechanism).
  • Impact:
    • Impact on user: While you mention "arm, arm64", it's unclear if users need to make any code changes or configuration adjustments.
    • Impact on build, hardware, documentation, security, compatibility: All these sections are marked as "arm, arm64" which doesn't provide any meaningful information. You need to state "YES" or "NO" and provide descriptions if applicable.
  • Testing:
    • Build Host(s): Specify the operating system, CPU architecture, and compiler used for building NuttX.
    • Target(s): Be specific about the architecture (e.g., ARM Cortex-M4) and the board configuration used for testing.
    • Testing logs: Provide actual testing logs before and after the change. "ostest" is not sufficient. Include relevant log output that demonstrates the performance improvement or any issues.

Recommendations:

  1. Explain the Rationale: Clearly articulate the reasons for inlining up_switch_context. Provide specific benchmarks or performance data to justify the change.
  2. Detailed Impact Assessment: Analyze and describe the potential impact on all aspects mentioned (build, hardware, etc.). If there's no impact, state "NO" explicitly.
  3. Thorough Testing: Provide detailed testing information, including build host and target specifications, and relevant log snippets that showcase the change's effects.

By addressing these points, your PR will be better aligned with NuttX's requirements and easier for reviewers to evaluate.

@github-actions github-actions bot added Arch: ceva Issues related to CEVA architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Sep 26, 2024
@hujun260 hujun260 force-pushed the apache_16 branch 2 times, most recently from 4534662 to 656a877 Compare September 26, 2024 13:22
@acassis
Copy link
Contributor

acassis commented Sep 26, 2024

@hujun260 modifications in the context switch requires deep testing, only ostest (you didn't say which arch was used) is not enough. History shows it always introduces regression.

@acassis
Copy link
Contributor

acassis commented Sep 26, 2024

Please remove reference to VELAPLATFO-38287

@hujun260
Copy link
Contributor Author

@hujun260 modifications in the context switch requires deep testing, only ostest (you didn't say which arch was used) is not enough. History shows it always introduces regression.

I understand your concerns. We've tested this PR on multiple products internally.

@github-actions github-actions bot removed Arch: ceva Issues related to CEVA architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Sep 27, 2024
@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Sep 27, 2024
@hujun260
arch: inline up_switch_context,in arm arm64
d9f0e30 
reason:
when a context switch occurs, up_switch_context is executed.
In order to reduce the time taken for context switching,
we inline the up_switch_context function.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: S The size of the change in this PR is small labels Sep 27, 2024
@hujun260
syscall: Use a more compatible writing style
1d2eeab 
compile error:
Register: ostest
Register: nsh
Register: sh
Register: hello
Register: getprime
In file included from /home/hujun5/downloads1/vela_sim/nuttx/include/arch/irq.h:35,
                 from /home/hujun5/downloads1/vela_sim/nuttx/include/nuttx/irq.h:37,
                 from /home/hujun5/downloads1/vela_sim/nuttx/include/nuttx/sched.h:40,
                 from /home/hujun5/downloads1/vela_sim/nuttx/include/nuttx/arch.h:87,
                 from common/arm_signal_dispatch.c:26:
common/arm_signal_dispatch.c: In function 'up_signal_dispatch':
common/arm_signal_dispatch.c:72:3: error: 'asm' operand has impossible constraints
   72 |   sys_call4(SYS_signal_handler, (uintptr_t)sighand, (uintptr_t)signo,
      |   ^~~~~~~~~
make[1]: *** [Makefile:168：arm_signal_dispatch.o] error 1

Signed-off-by: hujun5 <hujun5@xiaomi.com>
xiaoxiang781216
xiaoxiang781216 reviewed Sep 28, 2024
View reviewed changes
arch/arm64/include/irq.h
@@ -423,6 +424,15 @@ static inline_function void up_set_current_regs(uint64_t *regs)
__asm__ volatile ("msr " "tpidr_el1" ", %0" : : "r" (regs));
}

#define up_switch_context(tcb, rtcb) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's create a new patch to inline other arch too

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

@anchao anchao anchao left review comments

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

@masayuki2009 masayuki2009 Awaiting requested review from masayuki2009

@pkarashchenko pkarashchenko Awaiting requested review from pkarashchenko

@yf13 yf13 Awaiting requested review from yf13

@davids5 davids5 Awaiting requested review from davids5

Assignees
No one assigned
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: L The size of the change in this PR is large
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hujun260 @nuttxpr @acassis @anchao @xiaoxiang781216