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

smpcall: we directly call the function to handle local smpcall #14663

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Nov 6, 2024

Summary

smpcall: we directly call the function to handle local smpcall
some arch do not support issuing interrupts to the local CPU.

This commit fixes the regression from #14656

Impact

smpcall

Testing

ci ostest
Build Host:

  • OS: Ubuntu 20.04
  • CPU: x86_64
  • Compiler: GCC 9.4.0

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

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 6, 2024
hujun260 added a commit to hujun260/nuttx that referenced this pull request Nov 6, 2024
reason:
some arch do not support issuing interrupts to the local CPU.

This commit fixes the regression from apache#14663

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

nuttxpr commented Nov 6, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is missing some key information. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change, and links a related PR. This is good.
  • Testing Information: Includes build host details and target architecture. The configuration and build commands are helpful. Including the qemu command line is excellent.

Weaknesses:

  • Missing Issue Reference: While a related PR is linked, if this fixes a reported issue, that issue should be linked as well.
  • Incomplete Impact Assessment: The impact section is extremely brief. While "smpcall" is mentioned, it needs more detail. Explicitly state YES/NO for each impact category and provide descriptions where necessary. For example:
    • Is new feature added? Is existing feature changed? Existing feature changed (smpcall functionality)
    • Impact on user: NO (unless they rely on the broken behavior)
    • Impact on build: NO
    • Impact on hardware: YES (affects architectures that don't support self-interrupts) Specify which architectures.
    • Impact on documentation: NO/YES - Justify.
    • Impact on security: NO/YES - Justify.
    • Impact on compatibility: Potentially YES. Since this reverts a regression, describe compatibility regained.
  • Insufficient Testing Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. These are crucial to demonstrate the fix. Show specific commands and their output that demonstrate the problem before the change and the correct behavior after. For example, demonstrate that an SMP-related command now works that previously failed.

Recommendation:

To fully meet the NuttX requirements, the PR needs to:

  1. Link any related NuttX issues.
  2. Expand the Impact section significantly. Provide YES/NO answers for each item and justifications. Be specific about affected architectures.
  3. Populate the Testing Logs sections with concrete examples. Show commands and output demonstrating the bug and the fix. Ideally, the logs should demonstrate the regression introduced by the linked PR and how this PR fixes it.

By addressing these points, the PR will be much stronger and easier for reviewers to evaluate.

hujun260 added a commit to hujun260/nuttx that referenced this pull request Nov 6, 2024
reason:
some arch do not support issuing interrupts to the local CPU.

This commit fixes the regression from apache#14663

Signed-off-by: hujun5 <[email protected]>
reason:
some arch do not support issuing interrupts to the local CPU.

This commit fixes the regression from apache#14663

Signed-off-by: hujun5 <[email protected]>
@xiaoxiang781216
Copy link
Contributor

@masayuki2009 could you verify this patch whether fix your problem?

@masayuki2009 masayuki2009 merged commit 7ec1894 into apache:master Nov 7, 2024
27 checks passed
@hujun260 hujun260 deleted the apache_12 branch November 7, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues 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