sys/vfs: use atomic_utils rather C11 atomics

[maribu]

Contribution description

This has the following advantages:

• faster and leaner when C11 atomics are not efficient (e.g. on LLVM this is almost always the case, as LLVM will only use efficient atomics if it doesn't has to bail out to library calls even for exotic things)
  • Even for GCC e.g. on the nucleo-f429zi this safes 72 B of .text for examples/filesystem despite runtime checks added for over- and underflow
• less pain in the ass for C++ and rust users, as both C++ and c2rust are incompatible with C11 atomics
• adds test for overflow of the open file counter for more robust operation
• adds assumes() so that underflows are detected in non-production code

Testing procedure

No regressions in VFS operations.

Beware: I don't have the time to test this, so this is completely untested.

Issues/PRs references

rust compatibility issues where reported in matrix

[chrysn]

On a high level, looks good to me. I'm uncertain whether assume is warranted -- AIU the difference between assert and assume is that with assume it's UB to be false, thus helping the compiler optimize away any dead branch -- but we don't have any code use the before, so it could just as well be asserts all over. (No harm done, at worst it needs a bit more thinking on the part of the reader).

[chrysn]

Suggested change

	uint16_t before = atomic_fetch_add_u16(&next->open_files, 1);
	/* We cannot use assume() here, an overflow could occur in absence of
	* any bugs and should also be checked for in production code. We use
	* expect() here, which was actually written for unit tests but works
	* here as well */
	uint16_t before = atomic_fetch_add_u16(&next->open_files, 1);
	/* We cannot use assume() here, an overflow could occur when the only
	* bug present is a memory leak, and should also be checked for in
	* production code. We use expect() here, which was actually written for
	* unit tests but works here as well */

Changes after the first lines are just impact of (guessed) reflowing.

Yes, technically there can be an application where open files exceed 2^16 and still not leak memory, but the file handles alone would be 256KiB at the very least, so it's safe to assume that there is a leak.

[maribu]

I think we should be careful about ruling out use cases that appear crazy ;)

Technically, even some 8 bit CPUs can use multiple MiB of RAM. (The ATXmega board's RAM can be extended with PSRAM and does have a way to address 2^24 Bytes or 16 MiB of memory.) And ESPs with some MiBs of PSRAM are quite common, e.g. those with a high megapixel camera module need lots of RAM to be able to take pictures.

So, while the use case of having ten thousands of files open appears pretty crazy, it is technically possible. And everything that is technically possible will eventually some professional lemming.

[chrysn]

Fair enough. I'd still be confused reading this as to how this can happen without a bug (such as a memory leak), but no biggie.

[riot-ci]

Murdock results

✔️ PASSED

45c473d sys/vfs: use atomic_utils rather C11 atomics

Success	Failures	Total	Runtime
10031	0	10031	10m:15s

Artifacts

[maikerlab]

Fixes my issue, where I couldn't USEMODULE += vfs in Rust application. Thanks!
Builds now for native and native64 target as well (I am using Ubuntu 22.04 on x86_64).

Ran unit tests for vfs:

• cd tests/unittests
• BOARD=native make tests-vfs term -> OK (40 tests)
• BOARD=native64 make tests-vfs term -> OK (40 tests)

Also did some manual testing using tests/sys/vfs_default application:

• genfile -o /nvm0/test: 2048 bytes written
• vfs r /nvm0/test 2048 0: output matches content generated by genfile

[chrysn]

1. Fundamentals are good (we generally prefer our atomic wrappers for the reasons you outlined).
2. There is barely any code design needed, and what is there is OK.
3. I trust maikerlab's tests.
4. No obvious code convention violations.
5. Comments are the suitable documentation for the implementation detail, and describe precisely what may surprise a reader.

Thank you both, let's ship it.

[kfessel]

@maribu

https://doc.rust-lang.org/std/sync/atomic/
https://doc.rust-lang.org/nomicon/atomics.html

Says that rust follows the the C/C++11 Model

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60932

says that stdatomic.h is part of C++20

https://en.cppreference.com/w/cpp/thread

says that the missing piece _Atomic() was added with c++20 but most other things are there since c++11

How is a RIOT specific header more compatible (does it relax the requirement (the access is not atomic) when Rust is interacting with it )?

[maribu]

Says that rust follows the the C/C++11 Model

Yeah, but that is about the memory model, not about the memory layout of a given variable in RAM. The memory layout of atomics in C and C++ are different, which is why you cannot pass an atomic variable across language barriers.

In fact, C11 atomics are not even consistant between C compilers. E.g. LLVM will use library calls to implement atomic accesses the moment a single type of operation cannot be implemented without the help of library calls, GCC assumes that it can happily mix between lock-free implementation and library calls on MCUs, forcing the library call implementation to got the IRQ disable route. (IMO the GCC choice is better, as disabling IRQs on MCUs is cheap and the only sensible implementation there anyway.)

The atomic utils do not define any new fancy types. They instead provide helper functions to operate on regular variables in an atomic fashion.