Skip to content

Conversation

@pussuw
Copy link
Contributor

@pussuw pussuw commented Nov 19, 2024

Summary

It should be a bit more efficient to do compared to the LR/SC pair.

Impact

Atomic test&set can be implemented with AMOSWAP, this way should be a bit more efficient even.

Testing

rv-virt:ksmp64
MPFS with SMP (not released yet)

It should be a bit more efficient to do compared to the LR/SC pair.
@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 19, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 19, 2024

[Experimental Bot, please feedback here]

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.

@anchao
Copy link
Contributor

anchao commented Nov 19, 2024

I don't understand, that the implementation of AMOSWAP was introduced to the community a long time ago.
#12232
Why did @hujun260 revert it to LR/SC? Is there any problem? @xiaoxiang781216
#12961

@anchao
Copy link
Contributor

anchao commented Nov 19, 2024

Did the code xiaomi submitted actually go through a review? I really feel like swearing right now. @xiaoxiang781216

@hujun260
Copy link
Contributor

Why did @hujun260 revert it to LR/SC? Is there any problem?

Sorry, The reason is as follows:
My initial modification was made in the local Xiaomi repository. However, our local code was not synchronized with the community in a timely manner, leading to the adoption of an old implementation. Later, when merging into the community repository, due to this commit involving file deletions, the new implementation was inadvertently deleted, causing this issue. I will be more cautious in the future.

Copy link
Contributor

@anchao anchao left a comment

Choose a reason for hiding this comment

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

Let's keep moving, please address the CI failure issue.

@pussuw
Copy link
Contributor Author

pussuw commented Nov 20, 2024

Let's keep moving, please address the CI failure issue.

I think the CI error is a known issue and not caused by this patch: #14841 (comment)

@anchao anchao merged commit 3146ea0 into apache:master Nov 20, 2024
@pussuw pussuw deleted the testset_amoswap branch November 20, 2024 05:39
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.

6 participants