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/risc-v: Implement up_this_task using Thread Pointer (TP) #15563

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

arch/risc-v: Implement up_this_task using Thread Pointer (TP) #15563

wants to merge 1 commit into from
+41 −2

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Jan 15, 2025

Summary

  • Added up_this_task() and up_update_task() macros to use TP register for fast task pointer access
  • Modified CPU startup and initial state code to initialize TP with task pointer
  • Updated context save/restore macros to handle TP only when TLS is enabled
  • Added irq.h include to riscv_cpustart.c for new macros

Impact

  • Improves performance by using TP for fast task pointer access
  • Reduces overhead of getting current task pointer in scheduler
  • Maintains compatibility with TLS configuration
  • No impact when CONFIG_SCHED_THREAD_LOCAL is enabled
  • Changes context save/restore behavior for TP

Testing

GitHub CI and local board with CONFIG_SCHED_THREAD_LOCAL enabled/disabled

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Jan 15, 2025
@no1wudi
arch/risc-v: Implement up_this_task using Thread Pointer (TP)
c2f6dd3 
Summary:
- Added up_this_task() and up_update_task() macros to use TP register for fast task pointer access
- Modified CPU startup and initial state code to initialize TP with task pointer
- Updated context save/restore macros to handle TP only when TLS is enabled
- Added irq.h include to riscv_cpustart.c for new macros

Impact:
- Improves performance by using TP for fast task pointer access
- Reduces overhead of getting current task pointer in scheduler
- Maintains compatibility with TLS configuration
- No impact when CONFIG_SCHED_THREAD_LOCAL is enabled
- Changes context save/restore behavior for TP

Signed-off-by: Huang Qi <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Jan 15, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although more detail could be provided. Here's a breakdown of why and where improvements could be made:

Strengths:

  • Clear Summary: The summary clearly outlines the what and how of the changes. It mentions specific functions/macros added/modified.
  • Impact Assessment: Addresses most impact categories. Specifically calls out performance improvements, compatibility considerations, and the limited impact when CONFIG_SCHED_THREAD_LOCAL is enabled.
  • Testing Performed: Mentions both CI and local board testing, and importantly, tests with the relevant configuration option both enabled and disabled.

Weaknesses & Suggestions for Improvement:

  • Missing Issue References: If this PR addresses a specific issue in NuttX or NuttX Apps, those should be linked in the Summary. Even if it's a performance improvement without a corresponding issue, stating "No related issue" is helpful.
  • Impact - User: While it mentions no impact if CONFIG_SCHED_THREAD_LOCAL is enabled, it doesn't explicitly state the user impact if it's disabled. Will applications need to be recompiled? Will there be any observable changes in behavior?
  • Impact - Build: Does adding these macros change the build process in any way? Even if the answer is "NO", explicitly stating it is helpful.
  • Impact - Hardware: Specifying the architecture (RISC-V as implied by riscv_cpustart.c) and the specific board used for local testing would be beneficial. "local board" is too vague.
  • Impact - Documentation: Does this change require documentation updates? If not, explicitly state "NO". If yes, provide the updates or a plan to update them.
  • Impact - Security: Even if the impact is "NO", stating it explicitly is good practice.
  • Impact - Compatibility: More detail is needed. What kind of backward/forward compatibility is maintained or broken? Are there interoperability concerns?
  • Testing Logs: The provided template requests testing logs. While mentioning CI and local testing is good, including snippets of relevant log output demonstrating the change (e.g., performance benchmarks, scheduler behavior) would significantly strengthen the PR. If logs are too extensive, consider linking to a separate file or gist.
  • Build Host Details: While GitHub CI likely covers various build hosts, specifying your local build host details (OS, CPU, compiler version) is still important.

By addressing these points, the PR would be even stronger and provide reviewers with all the necessary information to efficiently assess the changes.

lupyuen
lupyuen approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

Tested OK with rv-virt:knsh64. Thanks :-)
https://gist.github.com/lupyuen/4b7811d6dae690e3a61b68cf359826aa

nsh> uname -a
NuttX 10.0.1 c2f6dd3d17 Jan 15 2025 18:29:25 risc-v rv-virt
nsh> ostest
ostest_main: Exiting with status 0

pussuw
pussuw reviewed Jan 15, 2025
View reviewed changes
arch/risc-v/include/irq.h
* thread-local storage.
*/

#ifndef CONFIG_SCHED_THREAD_LOCAL
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use TP when system calls are enabled as well, even when SCHED_THREAD_LOCAL is in use.

The current logic in exception_common just needs to be modified to swap user TP with kernel TP (from the scratch area) always (not just when a system call is taken).

In flat mode where the kernel symbols are directly visible/callable this obviously does not work.

pussuw
pussuw reviewed Jan 15, 2025
View reviewed changes
arch/risc-v/src/common/riscv_initialstate.c
@@ -153,6 +153,8 @@ void up_initial_state(struct tcb_s *tcb)
#ifdef CONFIG_SCHED_THREAD_LOCAL
xcp->regs[REG_TP] = (uintptr_t)tcb->stack_alloc_ptr +
sizeof(struct tls_info_s);
#else
xcp->regs[REG_TP] = (uintptr_t)tcb;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary, just set tcb -> tp directly when a context switch occurs.

pussuw
pussuw reviewed Jan 15, 2025
View reviewed changes
arch/risc-v/src/common/riscv_macros.S
@@ -58,7 +58,9 @@
#ifdef RISCV_SAVE_GP
REGSTORE x3, REG_X3(\in) /* gp */
#endif
#ifdef CONFIG_SCHED_THREAD_LOCAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary as well, TP will remain stable when the user process is running, as well as during a trap. Only time when TP needs to be updated is when a context switch occurs.

pussuw
pussuw reviewed Jan 15, 2025
View reviewed changes
arch/risc-v/include/irq.h
})

/* Update the current task pointer stored in TP register */
#define up_update_task(t) \
Copy link
Contributor

Choose a reason for hiding this comment

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

You should call this when a context switch occurs.

@no1wudi no1wudi marked this pull request as draft January 15, 2025 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pussuw pussuw pussuw left review comments

@lupyuen lupyuen lupyuen approved these changes

Assignees
No one assigned
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@no1wudi @nuttxpr @pussuw @lupyuen