nuttxpr commented on PR #14857: URL: https://github.com/apache/nuttx/pull/14857#issuecomment-2485928291
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not meet the NuttX requirements. The provided information is far too vague and lacks crucial details. Here's a breakdown of what's missing: * **Insufficient Summary:** "More efficient" is not a helpful explanation. What *exactly* was changed? What function/file/module is affected? What problem does this solve or what feature does it add? Issue references are missing. * **Unclear Impact:** Simply stating "more efficient" is not enough. Quantify the improvement if possible. Address *all* impact points (user, build, hardware, documentation, security, compatibility) with specific "YES/NO" answers and explanations. "Atomic test&set can be implemented with AMOSWAP" belongs in the *Summary* (describing *how* the change works), not the *Impact* section. * **Inadequate Testing:** "rv-virt:ksmp64" and "MPFS with SMP (not released yet)" are insufficient. Specify: * Host OS, CPU architecture, compiler version used for building. * The specific target board and configuration used. * **Crucially,** the "Testing logs before change" and "Testing logs after change" sections are *empty*. There's no evidence provided that the change actually works or improves efficiency. Show concrete results, benchmarks, or other proof. **Example of how to improve this PR description:** ## Summary This PR replaces the existing LR/SC implementation of atomic test&set in the `arch/risc-v/src/atomic.c` file with an AMOSWAP instruction. This change aims to improve the efficiency of atomic operations, particularly in multi-core scenarios where contention on the atomic variable is likely. This addresses [NuttX Issue #123](replace with actual issue link if applicable). ## Impact * **Is new feature added? Is existing feature changed?** Existing feature changed. * **Impact on user:** NO. Users should not experience any functional changes. * **Impact on build:** NO. The build process remains unchanged. * **Impact on hardware:** YES. This change specifically targets RISC-V architectures that support the AMOSWAP instruction. Other architectures are unaffected. * **Impact on documentation:** YES. The `arch/risc-v/` documentation should be updated to reflect the use of AMOSWAP. (Documentation updates are included in this PR). * **Impact on security:** NO. This change should not have any security implications. * **Impact on compatibility:** NO. This change maintains backward compatibility. * **Anything else to consider:** Performance improvements may vary depending on the specific hardware and workload. ## Testing **Build Host:** Linux (Ubuntu 22.04), x86_64, GCC 11.2.0 **Target:** QEMU RISC-V virt machine (rv-virt:ksmp64 config) **Testing logs before change:** ``` # Show output of a benchmark or test demonstrating performance before the change Time taken for atomic test&set operation (LR/SC): 125ms (average over 10000 iterations) ``` **Testing logs after change:** ``` # Show output of the same benchmark/test after applying the change Time taken for atomic test&set operation (AMOSWAP): 75ms (average over 10000 iterations) ``` By providing this level of detail, reviewers can properly assess the PR and determine its suitability for inclusion in NuttX. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
