sys/vfs: use atomic_utils rather C11 atomics

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

sys/include/vfs.h

@@ -54,14 +54,6 @@
 #define VFS_H
 
 #include <stdint.h>
-/* The stdatomic.h in GCC gives compilation errors with C++
- * see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60932
- */
-#ifdef __cplusplus
-#include "c11_atomics_compat.hpp"
-#else
-#include <stdatomic.h> /* for atomic_int */
-#endif
 #include <sys/stat.h> /* for struct stat */
 #include <sys/types.h> /* for off_t etc. */
 #include <sys/statvfs.h> /* for struct statvfs */
@@ -383,7 +375,7 @@ struct vfs_mount_struct {
     const vfs_file_system_t *fs; /**< The file system driver for the mount point */
     const char *mount_point;     /**< Mount point, e.g. "/mnt/cdrom" */
     size_t mount_point_len;      /**< Length of mount_point string (set by vfs_mount) */
-    atomic_int open_files;       /**< Number of currently open files and directories */
+    uint16_t open_files;         /**< Number of currently open files and directories */
     void *private_data;          /**< File system driver private data, implementation defined */
 };
 

sys/vfs/vfs.c

@@ -23,13 +23,16 @@
 #include <fcntl.h> /* for O_ACCMODE, ..., fcntl */
 #include <unistd.h> /* for STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO */
 
+#include "atomic_utils.h"
+#include "clist.h"
+#include "compiler_hints.h"
 #include "container.h"
 #include "modules.h"
-#include "vfs.h"
 #include "mutex.h"
-#include "thread.h"
 #include "sched.h"
-#include "clist.h"
+#include "test_utils/expect.h"
+#include "thread.h"
+#include "vfs.h"
 
 #define ENABLE_DEBUG 0
 #include "debug.h"
@@ -295,7 +298,8 @@ int vfs_open(const char *name, int flags, mode_t mode)
     if (fd < 0) {
         DEBUG("vfs_open: _init_fd: ERR %d!\n", fd);
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return fd;
     }
     vfs_file_t *filp = &_vfs_open_files[fd];
@@ -425,7 +429,8 @@ int vfs_opendir(vfs_DIR *dirp, const char *dirname)
         int res = dirp->d_op->opendir(dirp, rel_path);
         if (res < 0) {
             /* remember to decrement the open_files count */
-            atomic_fetch_sub(&mountp->open_files, 1);
+            uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+            assume(before > 0);
             return res;
         }
     }
@@ -463,7 +468,8 @@ int vfs_closedir(vfs_DIR *dirp)
         }
     }
     memset(dirp, 0, sizeof(*dirp));
-    atomic_fetch_sub(&mountp->open_files, 1);
+    uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+    assume(before > 0);
     return res;
 }
 
@@ -563,8 +569,9 @@ int vfs_umount(vfs_mount_t *mountp, bool force)
         DEBUG("vfs_umount: invalid fs\n");
         return -EINVAL;
     }
-    DEBUG("vfs_umount: -> \"%s\" open=%d\n", mountp->mount_point, atomic_load(&mountp->open_files));
-    if (atomic_load(&mountp->open_files) > 0 && !force) {
+    DEBUG("vfs_umount: -> \"%s\" open=%u\n", mountp->mount_point,
+          (unsigned)atomic_load_u16(&mountp->open_files));
+    if (atomic_load_u16(&mountp->open_files) > 0 && !force) {
         mutex_unlock(&_mount_mutex);
         return -EBUSY;
     }
@@ -610,7 +617,8 @@ int vfs_rename(const char *from_path, const char *to_path)
         /* rename not supported */
         DEBUG("vfs_rename: rename not supported by fs!\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return -EROFS;
     }
     const char *rel_to;
@@ -621,15 +629,18 @@ int vfs_rename(const char *from_path, const char *to_path)
         /* No mount point maps to the requested file name */
         DEBUG("vfs_rename: to: no matching mount\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return res;
     }
     if (mountp_to != mountp) {
         /* The paths are on different file systems */
         DEBUG("vfs_rename: from_path and to_path are on different mounts\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
-        atomic_fetch_sub(&mountp_to->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
+        before = atomic_fetch_sub_u16(&mountp_to->open_files, 1);
+        assume(before > 0);
         return -EXDEV;
     }
     res = mountp->fs->fs_op->rename(mountp, rel_from, rel_to);
@@ -642,8 +653,10 @@ int vfs_rename(const char *from_path, const char *to_path)
         DEBUG("\n");
     }
     /* remember to decrement the open_files count */
-    atomic_fetch_sub(&mountp->open_files, 1);
-    atomic_fetch_sub(&mountp_to->open_files, 1);
+    uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+    assume(before > 0);
+    before = atomic_fetch_sub_u16(&mountp_to->open_files, 1);
+    assume(before > 0);
     return res;
 }
 
@@ -669,7 +682,8 @@ int vfs_unlink(const char *name)
         /* unlink not supported */
         DEBUG("vfs_unlink: unlink not supported by fs!\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return -EROFS;
     }
     res = mountp->fs->fs_op->unlink(mountp, rel_path);
@@ -682,7 +696,8 @@ int vfs_unlink(const char *name)
         DEBUG("\n");
     }
     /* remember to decrement the open_files count */
-    atomic_fetch_sub(&mountp->open_files, 1);
+    uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+    assume(before > 0);
     return res;
 }
 
@@ -706,7 +721,8 @@ int vfs_mkdir(const char *name, mode_t mode)
         /* mkdir not supported */
         DEBUG("vfs_mkdir: mkdir not supported by fs!\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return -EROFS;
     }
     res = mountp->fs->fs_op->mkdir(mountp, rel_path, mode);
@@ -719,7 +735,8 @@ int vfs_mkdir(const char *name, mode_t mode)
         DEBUG("\n");
     }
     /* remember to decrement the open_files count */
-    atomic_fetch_sub(&mountp->open_files, 1);
+    uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+    assume(before > 0);
     return res;
 }
 
@@ -743,7 +760,8 @@ int vfs_rmdir(const char *name)
         /* rmdir not supported */
         DEBUG("vfs_rmdir: rmdir not supported by fs!\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return -EROFS;
     }
     res = mountp->fs->fs_op->rmdir(mountp, rel_path);
@@ -756,7 +774,8 @@ int vfs_rmdir(const char *name)
         DEBUG("\n");
     }
     /* remember to decrement the open_files count */
-    atomic_fetch_sub(&mountp->open_files, 1);
+    uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+    assume(before > 0);
     return res;
 }
 
@@ -780,13 +799,15 @@ int vfs_stat(const char *restrict path, struct stat *restrict buf)
         /* stat not supported */
         DEBUG("vfs_stat: stat not supported by fs!\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return -EPERM;
     }
     memset(buf, 0, sizeof(*buf));
     res = mountp->fs->fs_op->stat(mountp, rel_path, buf);
     /* remember to decrement the open_files count */
-    atomic_fetch_sub(&mountp->open_files, 1);
+    uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+    assume(before > 0);
     return res;
 }
 
@@ -810,13 +831,15 @@ int vfs_statvfs(const char *restrict path, struct statvfs *restrict buf)
         /* statvfs not supported */
         DEBUG("vfs_statvfs: statvfs not supported by fs!\n");
         /* remember to decrement the open_files count */
-        atomic_fetch_sub(&mountp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+        assume(before > 0);
         return -EPERM;
     }
     memset(buf, 0, sizeof(*buf));
     res = mountp->fs->fs_op->statvfs(mountp, rel_path, buf);
     /* remember to decrement the open_files count */
-    atomic_fetch_sub(&mountp->open_files, 1);
+    uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
+    assume(before > 0);
     return res;
 }
 
@@ -954,14 +977,20 @@ bool vfs_iterate_mount_dirs(vfs_DIR *dir)
      * we'd need to let go of it now to actually open the directory. This
      * temporary count ensures that the file system will stick around for the
      * directory open step that follows immediately */
-    atomic_fetch_add(&next->open_files, 1);
+    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 */
+    expect(before < UINT16_MAX);
 
     /* Ignoring errors, can't help with them */
     vfs_closedir(dir);
 
     int err = vfs_opendir(dir, next->mount_point);
     /* No matter the success, the open_files lock has done its duty */
-    atomic_fetch_sub(&next->open_files, 1);
+    before = atomic_fetch_sub_u16(&next->open_files, 1);
+    assume(before > 0);
 
     if (err != 0) {
         DEBUG("vfs_iterate_mount opendir erred: vfs_opendir(\"%s\") = %d\n", next->mount_point, err);
@@ -1018,7 +1047,8 @@ static inline void _free_fd(int fd)
 {
     DEBUG("_free_fd: %d, pid=%d\n", fd, _vfs_open_files[fd].pid);
     if (_vfs_open_files[fd].mp != NULL) {
-        atomic_fetch_sub(&_vfs_open_files[fd].mp->open_files, 1);
+        uint16_t before = atomic_fetch_sub_u16(&_vfs_open_files[fd].mp->open_files, 1);
+        assume(before > 0);
     }
     _vfs_open_files[fd].pid = KERNEL_PID_UNDEF;
 }
@@ -1082,7 +1112,12 @@ static inline int _find_mount(vfs_mount_t **mountpp, const char *name, const cha
         return -ENOENT;
     }
     /* Increment open files counter for this mount */
-    atomic_fetch_add(&mountp->open_files, 1);
+    uint16_t before = atomic_fetch_add_u16(&mountp->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 */
+    expect(before < UINT16_MAX);
     mutex_unlock(&_mount_mutex);
     *mountpp = mountp;
 

tests/unittests/tests-vfs/tests-vfs-file-system-ops.c

@@ -15,12 +15,12 @@
  */
 #include <errno.h>
 #include <stddef.h>
-#include <stdint.h>
 #include <string.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
 
+#include "atomic_utils.h"
 #include "embUnit/embUnit.h"
 
 #include "vfs.h"
@@ -72,7 +72,7 @@ static void setup(void)
 static void teardown(void)
 {
     vfs_umount(&_test_vfs_mount_null, false);
-    atomic_store(&_test_vfs_mount_null.open_files, 0);
+    atomic_store_u16(&_test_vfs_mount_null.open_files, 0);
 }
 
 static void test_vfs_null_fs_ops_mount(void)
@@ -96,7 +96,8 @@ static void test_vfs_null_fs_ops_umount(void)
 static void test_vfs_null_fs_ops_umount__EBUSY(void)
 {
     TEST_ASSERT_EQUAL_INT(0, _test_vfs_fs_op_mount_res);
-    atomic_fetch_add(&_test_vfs_mount_null.open_files, 1);
+    uint16_t before = atomic_fetch_add_u16(&_test_vfs_mount_null.open_files, 1);
+    TEST_ASSERT(before < UINT16_MAX);
     int res = vfs_umount(&_test_vfs_mount_null, false);
     TEST_ASSERT_EQUAL_INT(-EBUSY, res);
     /* force unmount */