fs_inode:Change the type of i_crefs to atomic_int #13443
+68
−140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crafcat7
force-pushed
the
inode_atomic
branch
from
2eb25f6
to
2f854a4
Compare
|
Please check that all i_crefs are replaced correctly, eg: |
crafcat7
force-pushed
the
inode_atomic
branch
from
2f854a4
to
24170b6
Compare
anchao
reviewed
Sep 14, 2024
crafcat7
force-pushed
the
inode_atomic
branch
from
24170b6
to
ca181c6
Compare
|
Ditto, please make sure to go through the code carefully |
crafcat7
force-pushed
the
inode_atomic
branch
2 times, most recently
from
acadd16
to
e5535a7
Compare
Done
Done |
crafcat7
force-pushed
the
inode_atomic
branch
4 times, most recently
from
75fbe24
to
b291fb2
Compare
include/nuttx/atomic.h
Outdated
| @@ -78,6 +78,9 @@ extern "C++" | |||
| # if !(__clang__) && defined(__cplusplus) | |||
| # define _Atomic | |||
| # endif | |||
| # if (__clang__) | |||
| # define _CLANG_DISABLE_CRT_DEPRECATION_WARNINGS | |||
There was a problem hiding this comment.
let's use the new macro or remove ATOMIC_VAR_INIT from code base
There was a problem hiding this comment.
Done,I used ATOMIC_INIT instead of ATOMIC_VAR_INIT. Normally it is equivalent to ATOMIC_VAR_INIT. In clang it means ATOMIC_INIT(value) (value)
There was a problem hiding this comment.
ATOMIC_VAR_INIT is deprecated, so we should change ATOMIC_VAR_INIT(x) to x.
crafcat7
force-pushed
the
inode_atomic
branch
3 times, most recently
from
c720470
to
4911fee
Compare
|
I found that some platforms have the following warning when using different compilers In the current strategy, if the compiler itself does not support Atomic, Nuttx Base Atomic (low-performance version) will be used. So can we ignore it? |
crafcat7
force-pushed
the
inode_atomic
branch
from
4911fee
to
236d671
Compare
Summary: 1.Modified the i_crefs from int16_t to atomic_int 2.Modified the i_crefs add, delete, read, and initialize interfaces to atomic operations The purpose of this change is to avoid deadlock in cross-core scenarios, where A Core blocks B Core’s request for a write operation to A Core when A Core requests a read operation to B Core. Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
crafcat7
force-pushed
the
inode_atomic
branch
from
236d671
to
3b6c346
Compare
Error: vfs/fs_epoll.c:126:3: error: macro 'ATOMIC_VAR_INIT' has been marked as deprecated [-Werror,-Wdeprecated-pragma]
ATOMIC_VAR_INIT(1), /* i_crefs */
^
/Applications/Xcode_15.2.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/include/stdatomic.h:54:41: note: macro marked 'deprecated' here
^
1 error generated.
make[1]: *** [fs_epoll.o] Error 1
Error: socket/socket.c:78:3: error: macro 'ATOMIC_VAR_INIT' has been marked as deprecated [-Werror,-Wdeprecated-pragma]
ATOMIC_VAR_INIT(1), /* i_crefs */
Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
…upport it Summary: Configuration/Tool: zp214xpa/nsh,CONFIG_ARM_TOOLCHAIN_GNU_EABI 2024-09-14 06:11:00 ------------------------------------------------------------------------------------ Cleaning... Configuring... Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI Building NuttX... arm-none-eabi-ld: /github/workspace/sources/nuttx/staging/libfs.a(fs_inoderemove.o): in function `inode_remove': fs_inoderemove.c:(.text.inode_remove+0x94): undefined reference to `__sync_synchronize' arm-none-eabi-ld: fs_inoderemove.c:(.text.inode_remove+0x9c): undefined reference to `__sync_synchronize' make[1]: *** [Makefile:212: nuttx] Error 1 make: *** [tools/Unix.mk:548: nuttx] Error 2 make: Target 'all' not remade because of errors. Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
crafcat7
force-pushed
the
inode_atomic
branch
7 times, most recently
from
e90852e
to
6b05d09
Compare
crafcat7
force-pushed
the
inode_atomic
branch
from
6b05d09
to
607130d
Compare
When the toolchain does not support atomic, it will use the version implemented by NuttX (low performance version). This scenario is consistent with the original design, so we can ignore it. see bug here: https://bugs.llvm.org/show_bug.cgi?id=43603 Error: inode/fs_inodeaddref.c:50:7: error: large atomic operation may incur significant performance penalty; the access size (4 bytes) exceeds the max lock-free size (0 bytes) [-Werror,-Watomic-alignment] 50 | atomic_fetch_add(&inode->i_crefs, 1); | ^ /tools/clang-arm-none-eabi/lib/clang/17/include/stdatomic.h:152:43: note: expanded from macro 'atomic_fetch_add' 152 | #define atomic_fetch_add(object, operand) __c11_atomic_fetch_add(object, operand, __ATOMIC_SEQ_CST) | ^ 1 error generated. make[1]: *** [Makefile:83: fs_inodeaddref.o] Error 1 Error: inode/fs_inodefind.c:74:7: error: large atomic operation may incur significant performance penalty; the access size (4 bytes) exceeds the max lock-free size (0 bytes) [-Werror,-Watomic-alignment] 74 | atomic_fetch_add(&node->i_crefs, 1); Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
crafcat7
force-pushed
the
inode_atomic
branch
from
607130d
to
cf85911
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Benefit from PR#13044, we can change the the inode->i_crefs to atomic. That we can avoid problems:
Deadlocks caused by the server requesting reads from the client and the client requesting writes from the server when operating a file system with multiple cores (e.g. RPMSGFS).
The main changes are as follows:
1.Modified the i_crefs from int16_t to atomic_int
2.Modified the i_crefs add, delete, read, and initialize interfaces to atomic operations
The main change of this PR is to change the i_crefs count of inode to atomic type and use atomic operations to maintain i_crefs.
Impact
Impact on user: NO, This is a change within VFS and should be transparent to external developers.
Impact on build: YES, This may affect compilation because the members of the inode structure have changed. Different compilers/platforms have different support for atomic, so some warnings are skipped.
Impact on compatibility: This implementation aims to be backward compatible. No existing functionality is expected to be broken.
Testing
Build Host(s): Linux x86
Target(s): sim/nsh
Basic operations on the file system can work normally