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

arch_atomic: support nx atomic function #14827

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

Conversation

zyfeier
Copy link
Contributor

@zyfeier zyfeier commented Nov 17, 2024

Summary

Modify the kernel to use nx atomic interfaces, avoiding the use of sizeof or typeof to determine the type of atomic operations, thereby simplifying the kernel's atomic interface operations.

This PR addresses the following issues:

  1. It can resolve the issue of incorrect type retrieval when using sizeof;
  2. It can resolve the issue of incorrectly using unsigned integers with atomic operations.

Impact

atomic function

Testing

sim:citest citest pass
sabre-6qaud:smp ostest pass
avr32dev1:nsh
bl602evb/elf
esp32-devkitc:sotest
esp32-devkitc:wamr_wasi_debug
mps3-an547/clang

@github-actions github-actions bot added Area: Bluetooth Area: Networking Effects networking subsystem Arch: arm Issues related to ARM (32-bit) architecture Arch: simulator Issues related to the SIMulator Area: Drivers Drivers issues Area: File System File System issues Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Nov 17, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 17, 2024

[Experimental Bot, please feedback here]

No, this PR description does not adequately meet the NuttX requirements. While it provides a basic summary of the change, it lacks crucial details and context. Here's a breakdown of what's missing and how it can be improved:

Summary:

  • Insufficient Detail: "Modify the kernel to use only the long type atomic interfaces" is vague. Which atomic interfaces are being modified? Are all atomic operations now using long, or just a specific subset? What was the previous behavior? Be specific. For example: "Replace the use of atomic_add() with size-specific variants (e.g., atomic_add_long()) throughout the kernel to avoid ambiguity and improve type safety."
  • Missing Rationale: Why is this change necessary? What problem does it solve? Does it improve performance, portability, or code clarity? For example: "Using sizeof or typeof to determine atomic operation types can lead to subtle errors when porting to different architectures or when dealing with complex data structures. This change enforces the use of explicit type specifiers, improving code maintainability and preventing potential portability issues."

Impact:

  • Too Brief: "atomic function" is not helpful. Address all impact categories explicitly, even if the answer is "NO." Provide explanations where needed.
    • User Impact: Likely NO. But explain why. "No direct user impact is expected. This change is internal to the kernel and should not affect application behavior."
    • Build Impact: Possibly YES. If the change affects header files or build configurations, describe the impact. If NO, state so explicitly.
    • Hardware Impact: Possibly YES if this affects architecture-specific code. Specify which architectures are affected and how. If NO, explain why.
    • Documentation Impact: Possibly YES if any documentation needs updating to reflect the API changes.
    • Security Impact: Possibly YES or NO. Justify your answer. Does this change address any potential vulnerabilities or introduce new ones?
    • Compatibility Impact: Possibly YES or NO. Does this break any existing code or drivers? Does it affect compatibility with different NuttX versions?

Testing:

  • Insufficient Detail: Provide more context. What does "sim:citest pass" mean? Which tests were run? "sabre-6qaud:smp pass" is also insufficient. Which SMP tests specifically?
  • Missing Before/After Logs: Include actual log snippets demonstrating the change in behavior or performance, if applicable. Even if there's no visible functional change in the logs, provide some evidence that the code is functioning as expected (e.g., kernel boot log snippets).
  • Expand on Test Setup: What specific configuration options were used for the build? What commands were used to run the tests? This information allows others to reproduce your results. Mention the NuttX version you tested against.

Example of an improved summary:

This PR refactors the kernel's atomic operations to exclusively use long type interfaces (e.g., atomic_add_long()). Previously, the code relied on sizeof and typeof to determine the appropriate atomic operation type, which could lead to portability issues and subtle errors when dealing with complex data structures. This change enforces the use of explicit type specifiers, improving code clarity, maintainability, and portability across different architectures. This change addresses issue #[insert issue number].

By providing more detail and context in each section, you make it easier for reviewers to understand the purpose, impact, and correctness of your changes, which significantly increases the chances of your PR being accepted.

arch/sim/src/sim/sim_heap.c Outdated Show resolved Hide resolved
include/nuttx/lib/atomic_long.h Outdated Show resolved Hide resolved
@yamt
Copy link
Contributor

yamt commented Nov 18, 2024

  • it seems a bit wasteful to mechanically replace short with long to me. especially where structure fields are arranged for a short.
  • why long? i suspect it's simpler to use a fixed sized type like int32_t given the current usage in our tree.

@zyfeier
Copy link
Contributor Author

zyfeier commented Nov 18, 2024

  • it seems a bit wasteful to mechanically replace short with long to me. especially where structure fields are arranged for a short.
  • why long? i suspect it's simpler to use a fixed sized type like int32_t given the current usage in our tree.

Hi, yamt, we will implement an interface similar to linux/atomic/atomic-instrumented.h for use by the NuttX kernel. What are your suggestions?

@yamt
Copy link
Contributor

yamt commented Nov 18, 2024

  • it seems a bit wasteful to mechanically replace short with long to me. especially where structure fields are arranged for a short.
  • why long? i suspect it's simpler to use a fixed sized type like int32_t given the current usage in our tree.

Hi, yamt, we will implement an interface similar to linux/atomic/atomic-instrumented.h for use by the NuttX kernel. What are your suggestions?

similar in which sense?
instrumentation?
types? (atomic_t/atomic64_t/atomic_long_t)
something else?

@zyfeier
Copy link
Contributor Author

zyfeier commented Nov 18, 2024

similar in which sense?
instrumentation?
types? (atomic_t/atomic64_t/atomic_long_t)
something else?

@yamt only support int32_t atomic type and interface.

@github-actions github-actions bot added Size: M The size of the change in this PR is medium and removed Size: L The size of the change in this PR is large labels Nov 18, 2024
arch/sim/src/sim/posix/sim_testset.c Outdated Show resolved Hide resolved
arch/sim/src/sim/sim_heap.c Outdated Show resolved Hide resolved
arch/sim/src/sim/sim_heap.c Outdated Show resolved Hide resolved
unsigned short owner;
unsigned short next;
atomic_t owner;
atomic_t next;
} tickets;
unsigned int value;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

can't simple change owner/next to 32bit, the algo aasume them 16bit

Copy link
Contributor

Choose a reason for hiding this comment

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

but, can't do atomic operation on 32bit arch

include/nuttx/spinlock.h Outdated Show resolved Hide resolved
sched/semaphore/sem_trywait.c Outdated Show resolved Hide resolved
sched/semaphore/sem_trywait.c Outdated Show resolved Hide resolved
sched/semaphore/sem_wait.c Outdated Show resolved Hide resolved
sched/semaphore/semaphore.h Outdated Show resolved Hide resolved
libs/libc/machine/arch_atomic.c Show resolved Hide resolved
@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: M The size of the change in this PR is medium labels Nov 19, 2024
@zyfeier zyfeier changed the title arch_atomic: nx only use atomic_long_xxx function arch_atomic: support nx atomic function Nov 19, 2024
@zyfeier zyfeier force-pushed the sem_atomic_2 branch 2 times, most recently from f477882 to 962c8c1 Compare November 19, 2024 13:58
@pussuw
Copy link
Contributor

pussuw commented Nov 19, 2024

  • it seems a bit wasteful to mechanically replace short with long to me. especially where structure fields are arranged for a short.

    • why long? i suspect it's simpler to use a fixed sized type like int32_t given the current usage in our tree.

Some architectures do not support subword atomics without involving libatomic (RISC-V is one). As a fallback, the current code we have invokes the nx_atomic functions which use spin locks internally to guarantee data integrity which causes a huge performance impact. I already see this after this patch #14465.

If the data type size needs to be optimized, please make it possible for the user to force use of lock free versions (i.e. toolchain optimized versions) of the atomic_ procedures, as people who use them most likely want performance rather than size optimization.

EDIT:
I misread a bit and my rant is about using subword size types for atomics. Using int32 as the atomic type is fine.

@yamt
Copy link
Contributor

yamt commented Nov 20, 2024

similar in which sense?
instrumentation?
types? (atomic_t/atomic64_t/atomic_long_t)
something else?

@yamt only support int32_t atomic type and interface.

i have a bit concern about the size increase from "short", especially where it doesn't really need to be atomic. (eg. non-SMP)

Modify the kernel to use only atomic_xx and atomic64_xx interfaces,
avoiding the use of sizeof or typeof to determine the type of
atomic operations, thereby simplifying the kernel's atomic
interface operations.

Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
@zyfeier zyfeier marked this pull request as ready for review November 25, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: simulator Issues related to the SIMulator Area: Bluetooth Area: Drivers Drivers issues Area: File System File System issues Area: Networking Effects networking subsystem Area: OS Components OS Components issues Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants