nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard

percpu reg store this_task

Open hujun260 opened this issue 1 year ago • 1 comments

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

hujun260 avatar Sep 29 '24 09:09 hujun260

[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.

nuttxpr avatar Sep 29 '24 09:09 nuttxpr

LGTM

GUIDINGLI avatar Oct 15 '24 07:10 GUIDINGLI