Skip to content

Conversation

@hujun260
Copy link
Contributor

@hujun260 hujun260 commented Nov 5, 2024

Summary

reason:
The old implementation of the SMP call, even when using the "no wait" parameter, could still result in waiting, if invoking it within a critical section may lead to deadlocks. Therefore, in order to implement a truly asynchronous SMP call strategy, we have added nxsched_smp_call_async.

#2822

apache/nuttx#14656

These two pull requests (PRs) must be merged together.

Impact

smp call

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

reason:
The old implementation of the SMP call, even when using the "no wait" parameter,
could still result in waiting, if invoking it within a critical section
may lead to deadlocks. Therefore, in order to implement a truly asynchronous SMP
call strategy, we have added nxsched_smp_call_async.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@nuttxpr
Copy link

nuttxpr commented Nov 5, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why:

  • Summary: While it states the reason for the change, it lacks crucial details. It doesn't specify what functional part of the code is changed (e.g., the scheduler, a specific driver, etc.) or how the new nxsched_smp_call_async function works differently internally. It needs to explain the mechanism used to achieve asynchronicity. It also lacks issue references if applicable.

  • Impact: This section is severely lacking. It only mentions "smp call" which is insufficient. It needs to explicitly answer all the questions with YES/NO and provide descriptions where necessary. For example:

    • Is new feature added? YES (nxsched_smp_call_async)
    • Impact on user? Potentially YES (if users were relying on the old behavior, they might need to adapt). This needs further elaboration.
    • Impact on build? Possibly NO, but needs confirmation.
    • Impact on hardware? Likely NO, but needs confirmation. Does it impact specific architectures?
    • Impact on documentation? YES (new function needs documentation).
    • Impact on security? Potentially YES/NO - needs analysis and explanation. Could asynchronous calls introduce new vulnerabilities?
    • Impact on compatibility? Potentially YES/NO - Does it break existing code? Needs explanation.
  • Testing: This section is also inadequate. "ci ostest" is not sufficient. It needs to specify:

    • The specific host OS, CPU architecture, compiler, and versions used for testing.
    • The specific target architecture, board, and configuration used for testing.
    • Actual meaningful log output before and after the change, demonstrating the difference in behavior and that the issue is resolved. Simply stating "ci ostest" doesn't provide any evidence of successful testing. Show the relevant parts of the log that prove the fix works.

In short, the PR description needs to be significantly more detailed and address all the points listed in the NuttX requirements template. It currently provides only a very high-level overview and lacks essential information for proper review.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @hujun260 :-) Some build errors please fix :-)

@hujun260
Copy link
Contributor Author

hujun260 commented Nov 5, 2024

#2822

apache/nuttx#14656

These two pull requests (PRs) must be merged together.

@cederom
Copy link
Contributor

cederom commented Nov 6, 2024

Looks like this broke some other things as seen in CI builds following the merge?

https://github.com/apache/nuttx-apps/actions/runs/11699250290/job/32580956611?pr=2826

I have restarted the failed CI job just to make sure :-)

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Nov 6, 2024

already fixed by: apache/nuttx#14664 and #2822

@hujun260 hujun260 deleted the apache_3 branch November 7, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants