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

sched: replace up_cpu_index with this_cpu #14954

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

hujun260
Copy link
Contributor

Summary

sched: replace up_cpu_index with this_cpu
Make this_cpu is arch independent and up_cpu_index do that. In AMP mode, up_cpu_index() may return the index of the physical core.

Impact

sched

Testing

ci 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 Arch: x86_64 Issues related to the x86_64 architecture Size: S The size of the change in this PR is small labels Nov 26, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 26, 2024

[Experimental Bot, please feedback here]

No. This PR description lacks crucial information required by the NuttX guidelines.

Here's why:

  • Insufficient Summary: While it mentions the change (up_cpu_index to this_cpu), it doesn't clearly explain why this change is necessary. What problem does it solve? What are the benefits of using this_cpu? It also doesn't explain how the change works beyond a very high-level description.
  • Incomplete Impact Assessment: Simply stating "sched" isn't enough. It needs YES/NO answers for all impact categories, with explanations where applicable. For example:
    • Impact on user: Likely NO, but needs confirmation.
    • Impact on build: Possibly NO, but needs explicit confirmation.
    • Impact on hardware: This is the most crucial part! The summary mentions AMP, implying a hardware impact. This needs a detailed explanation of which architectures/boards are affected.
    • Impact on documentation: Does this change require documentation updates?
    • Impact on security, compatibility: These need explicit NO/YES answers with justification.
  • Inadequate Testing: "ci ostest" is not sufficient. It needs:
    • Specific build host details (OS, CPU, compiler versions).
    • Specific target details (architecture, board, configuration).
    • Actual testing logs before and after the change, demonstrating the issue and the fix. Empty code blocks are unacceptable.

The submitter needs to significantly revise this PR description to meet NuttX requirements before it can be properly reviewed.

arch/arm/src/armv8-r/arm_gicv3.c Outdated Show resolved Hide resolved
Make this_cpu is arch independent and up_cpu_index do that.
In AMP mode, up_cpu_index() may return the index of the physical core.

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Size: XS The size of the change in this PR is very small and removed Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small labels Nov 26, 2024
@xiaoxiang781216 xiaoxiang781216 merged commit ef31375 into apache:master Nov 26, 2024
27 checks passed
@hujun260 hujun260 deleted the apache_31 branch December 1, 2024 02:54
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: x86_64 Issues related to the x86_64 architecture Size: XS The size of the change in this PR is very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants