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

risc-v: remove g_running_tasks[this_cpu()] = NULL #14984

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

Conversation

hujun260
Copy link
Contributor

Summary

risc-v: remove g_running_tasks[this_cpu()] = NULL
reason:
We hope to keep g_running_tasks valid forever.

Impact

riscv

Testing

ci ostest
rv-virt:smp

@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 Nov 29, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 29, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although it could be significantly improved. Here's a breakdown:

Strengths:

  • Provides a summary of the change: It explains what is being changed (removing g_running_tasks[this_cpu()] = NULL) and why (to keep g_running_tasks valid).
  • Indicates impact: It specifies the affected architecture (RISC-V).
  • Includes testing information: It mentions CI, ostest, rv-virt, and smp, suggesting some level of testing has been performed.

Weaknesses (and how to improve them):

  • Insufficient detail in the summary: The "how" is missing. How does removing this line keep g_running_tasks valid? What scenario was this line causing problems in? More context is crucial for reviewers.
  • Vague impact assessment: Simply stating "riscv" is not enough. Which RISC-V boards? Are there any performance implications? All the "Impact" checklist items should be explicitly addressed (even with "NO" if they don't apply). For example:
    • Impact on user: NO
    • Impact on build: NO
    • Impact on hardware: YES (RISC-V architecture, specifically...)
    • etc.
  • Missing testing logs: The PR template specifically requests "Testing logs before change" and "Testing logs after change." These are essential for demonstrating the fix works and doesn't introduce regressions. Placeholder comments are not sufficient; actual logs are required.
  • Lack of issue references: If this addresses a specific issue, linking it (even if it's just a local bug you encountered) helps provide context and traceability.

Example of an improved summary:

"risc-v: prevent invalid g_running_tasks entries during context switches

Currently, g_running_tasks[this_cpu()] is set to NULL during context switches. This can lead to issues in scenarios like [briefly describe the scenario, e.g., nested interrupts, certain scheduling edge cases] where code relies on g_running_tasks being consistently valid. This change removes the line setting g_running_tasks[this_cpu()] to NULL, ensuring it always points to the currently running task, even during context switches. This resolves [mention the specific problem being solved, e.g., a crash, incorrect behavior].

Related Issue: [If applicable]"

By addressing these weaknesses, the PR will be much clearer, easier to review, and more likely to be accepted.

Copy link
Contributor

@pussuw pussuw left a comment

Choose a reason for hiding this comment

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

riscv_syscall_dispatch.c must be fixed too?

@hujun260
Copy link
Contributor Author

riscv_syscall_dispatch.c

done

reason:
We hope to keep g_running_tasks valid forever.

Signed-off-by: hujun5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants