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

percpu reg store this_task #13726

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

percpu reg store this_task #13726

wants to merge 3 commits into from

Conversation

hujun260
Copy link
Contributor

Summary

We can utilize percpu storage to hold information about the
current running task. If we intend to implement this feature, we would
need to define two macros that help us manage this percpu information
effectively.

up_this_task: This macro is designed to read the contents of the percpu
register to retrieve information about the current
running task.This allows us to quickly access
task-specific data without having to disable interrupts,
access global variables and obtain the current cpu index.

up_update_task: This macro is responsible for updating the contents of
the percpu register.It is typically called during
initialization or when a context switch occurs to ensure
that the percpu register reflects the information of the
newly running task.

Impact

arm64

Testing

Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic \
   -machine virt,virtualization=on,gic-version=3 \
   -net none -chardev stdio,id=con,mux=on -serial chardev:con \
   -mon chardev=con,mode=readline -kernel ./nuttx

reason:
We can utilize percpu storage to hold information about the
current running task. If we intend to implement this feature, we would
need to define two macros that help us manage this percpu information
effectively.

up_this_task: This macro is designed to read the contents of the percpu
              register to retrieve information about the current
              running task.This allows us to quickly access
              task-specific data without having to disable interrupts,
              access global variables and obtain the current cpu index.

up_update_task: This macro is responsible for updating the contents of
                the percpu register.It is typically called during
                initialization or when a context switch occurs to ensure
                that the percpu register reflects the information of the
                newly running task.

Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic \
   -machine virt,virtualization=on,gic-version=3 \
   -net none -chardev stdio,id=con,mux=on -serial chardev:con \
   -mon chardev=con,mode=readline -kernel ./nuttx

Signed-off-by: hujun5 <[email protected]>
…task more earlier.

A call stack looks like the following:
sched_idletask
syslog_write
nx_vsyslog
syslog
getreg64
gic_validate_redist_version
arm64_gic_init
arm64_gic_secondary_init
arm64_boot_secondary_c_routine

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Area: Networking Effects networking subsystem Arch: arm64 Issues related to ARM64 (64-bit) architecture Area: File System File System issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Sep 29, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 29, 2024

[Experimental Bot, please feedback here]

The provided PR description is insufficient based on the NuttX requirements.

Here's why and how to improve it:

Missing Information

  • Summary:
    • Why is this change necessary? What specific problems does using percpu storage for task information solve? Is there a performance bottleneck this addresses? Is it a prerequisite for another feature?
    • What functional part of the code is being changed? Be more specific than "arm64". Are you modifying the scheduler? A specific architecture port?
  • Impact:
    • Is this a new feature or changing an existing one? This is important for understanding the scope.
    • Impact on user: Even if you believe the impact on the user is minimal, state that explicitly. If there's no impact on the user API, say so.
    • Impact on build: Will any new configuration options be added?
    • You only mention "arm64". Will this impact other architectures? If so, how? If not, state that this is arm64 specific.
    • Impact on documentation: New features or significant changes require documentation updates. Indicate if you are providing documentation updates as part of this PR.
    • Security, compatibility: Even if there is no impact, state this explicitly.
  • Testing:
    • Insufficient build host details: Specify the operating system (e.g., Ubuntu 20.04), CPU architecture (x86_64), and compiler version used for testing.
    • "Running with qemu" is not enough:
      • Provide the full QEMU command line used.
      • Crucially, you need to show HOW you tested this. What specific commands did you run in the NuttX shell (nsh) to validate that up_this_task and up_update_task work as expected?
    • Show both "before" and "after" logs. What is different in the output that demonstrates the change is working?

Recommendations

  1. Provide concrete motivations for the change.
  2. Be specific about the code being modified.
  3. Explain the user impact even if it is minimal.
  4. Detail build system changes.
  5. Clarify architecture specificity (is it only arm64?).
  6. Describe any new or modified documentation.
  7. Provide comprehensive test cases with expected output. Demonstrate how you validated that the percpu storage is being used correctly.

By addressing these points, your PR will be much clearer and easier for the NuttX maintainers to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm64 Issues related to ARM64 (64-bit) architecture Area: File System File System issues Area: Networking Effects networking subsystem Area: OS Components OS Components issues 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.

2 participants