-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: master
Are you sure you want to change the base?
Conversation
[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:
Impact:
Testing:
Example of an improved summary: This PR refactors the kernel's atomic operations to exclusively use long type interfaces (e.g., 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. |
437bd80
to
4b3655c
Compare
|
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? |
@yamt only support int32_t atomic type and interface. |
4b3655c
to
7e573ba
Compare
unsigned short owner; | ||
unsigned short next; | ||
atomic_t owner; | ||
atomic_t next; | ||
} tickets; | ||
unsigned int value; | ||
}; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
7e573ba
to
4182a7d
Compare
f477882
to
962c8c1
Compare
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: |
962c8c1
to
cff42d9
Compare
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. Signed-off-by: zhangyuan29 <[email protected]>
Because SysV is not used, disable SysV to save RAM space. Signed-off-by: zhangyuan29 <[email protected]>
b49baf8
to
91a989b
Compare
i have a bit concern about the size increase from "short", especially where it doesn't really need to be atomic. (eg. non-SMP) |
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:
Impact
atomic function
Testing
./tools/configure.sh sim:citest pass
sabre-6qaud:smp pass