From 930fe6e67cd4644a6ec78e6bda0ac996a25bbcb7 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Fri, 16 May 2025 13:24:25 -0500 Subject: [PATCH] Force lfsr_file_sync_ off the stack hot-path This adds LFS_NOINLINE, and forces lfsr_file_sync_ (the commit logic in lfsr_file_sync) off the stack hot-path. This adds a bit of code, function calls are surprisingly expensive, but saves a nice big chunk of stack: code stack ctx before: 35992 2408 636 after: 36016 (+0.1%) 2296 (-4.7%) 636 (+0.0%) Well, maybe not _real_ stack. The fact that this worked suggests the real stack usage is less than our measured value. The reason is because our stack.py script is relatively simple. It just adds together stack frames based on the callgraph at compile time, which misses shrinkwrapping and similar optimizations. Unfortunately that sort of information is simply not available via GCC short of parsing the disassembly. But this is the number that will be used for statically allocated stacks, and of course the number that will probably end up associated with littlefs, so it still seems like a worthwhile number to "optimize" for. Maybe in the future this will be different as tooling around stack measurements improves. --- The other benefit of moving lfsr_file_sync_ off the hot-path is that we now no longer incorrectly include the sync commit context in the hot-path. This tells a much different story for the cost of 1-commit shrubs: code stack ctx before 1c-shrubs: 35848 2296 636 after 1c-shrubs: 36016 (+0.5%) 2296 (+0.0%) 636 (+0.0%) --- lfs.c | 95 +++++++++++++++++++++++++++++++----------------------- lfs_util.h | 7 ++++ 2 files changed, 62 insertions(+), 40 deletions(-) diff --git a/lfs.c b/lfs.c index be29fd6d..cd10af09 100644 --- a/lfs.c +++ b/lfs.c @@ -12696,37 +12696,9 @@ failed:; return err; } -int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { - LFS_ASSERT(lfsr_omdir_isopen(lfs, &file->b.o)); - // can't write to readonly files, if you want to resync call - // lfsr_file_resync - LFS_ASSERT(!lfsr_o_isrdonly(file->b.o.flags)); - - // removed? sync is just a noop in this case - int err; - if (lfsr_o_iszombie(file->b.o.flags)) { - return 0; - } - - // first flush any data in our cache, this is a noop if already - // flushed - // - // note that flush does not change the actual file data, so if - // flush succeeds but mdir commit fails it's ok to fall back to - // our flushed state - // - // though don't flush quite yet if our file is small and can be - // combined with sync in a single commit - if (file->cache.size < lfsr_file_size_(file) - || file->cache.size > lfs->cfg->inline_size - || file->cache.size > lfs->cfg->fragment_size - || file->cache.size >= lfs->cfg->crystal_thresh) { - err = lfsr_file_flush(lfs, file); - if (err) { - goto failed; - } - } - +// this LFS_NOINLINE is to force lfsr_file_sync_ off the stack hot-path +LFS_NOINLINE +static int lfsr_file_sync_(lfs_t *lfs, lfsr_file_t *file) { // build a commit of any pending file metadata lfsr_rattr_t rattrs[4]; lfs_size_t rattr_count = 0; @@ -12740,13 +12712,13 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { // uncreated files must be unsynced LFS_ASSERT(lfsr_o_isunsync(file->b.o.flags)); - err = lfsr_rbyd_lookup(lfs, &file->b.o.mdir.rbyd, + int err = lfsr_rbyd_lookup(lfs, &file->b.o.mdir.rbyd, lfsr_mrid(lfs, file->b.o.mdir.mid), LFSR_TAG_STICKYNOTE, NULL, &name_data); if (err) { // orphan flag but no stickynote tag? LFS_ASSERT(err != LFS_ERR_NOENT); - goto failed; + return err; } rattrs[rattr_count++] = LFSR_RATTR_DATA( @@ -12782,9 +12754,9 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { // pending file changes? if (lfsr_o_isunsync(file->b.o.flags)) { // make sure data is on-disk before committing metadata - err = lfsr_bd_sync(lfs); + int err = lfsr_bd_sync(lfs); if (err) { - goto failed; + return err; } // zero size files should have no bshrub/btree @@ -12831,19 +12803,18 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { // lookup the attr lfsr_data_t data; - err = lfsr_mdir_lookup(lfs, &file->b.o.mdir, + int err = lfsr_mdir_lookup(lfs, &file->b.o.mdir, LFSR_TAG_ATTR(file->cfg->attrs[i].type), NULL, &data); if (err && err != LFS_ERR_NOENT) { - goto failed; + return err; } // does disk match our attr? lfs_scmp_t cmp = lfsr_attr_cmp(lfs, &file->cfg->attrs[i], (err != LFS_ERR_NOENT) ? &data : NULL); if (cmp < 0) { - err = cmp; - goto failed; + return cmp; } if (cmp != LFS_CMP_EQ) { @@ -12865,13 +12836,57 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { // commit! LFS_ASSERT(rattr_count <= sizeof(rattrs)/sizeof(lfsr_rattr_t)); - err = lfsr_mdir_commit(lfs, &file->b.o.mdir, + int err = lfsr_mdir_commit(lfs, &file->b.o.mdir, rattrs, rattr_count); + if (err) { + return err; + } + } + + return 0; +} + +int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { + LFS_ASSERT(lfsr_omdir_isopen(lfs, &file->b.o)); + // can't write to readonly files, if you want to resync call + // lfsr_file_resync + LFS_ASSERT(!lfsr_o_isrdonly(file->b.o.flags)); + + // removed? sync is just a noop in this case + int err; + if (lfsr_o_iszombie(file->b.o.flags)) { + return 0; + } + + // first flush any data in our cache, this is a noop if already + // flushed + // + // note that flush does not change the actual file data, so if + // flush succeeds but mdir commit fails it's ok to fall back to + // our flushed state + // + // though don't flush quite yet if our file is small and can be + // combined with sync in a single commit + if (file->cache.size < lfsr_file_size_(file) + || file->cache.size > lfs->cfg->inline_size + || file->cache.size > lfs->cfg->fragment_size + || file->cache.size >= lfs->cfg->crystal_thresh) { + err = lfsr_file_flush(lfs, file); if (err) { goto failed; } } + // commit any pending metadata to disk + // + // the use of a second function here is mainly to isolate the stack + // costs of lfsr_file_flush and lfsr_file_sync_ + // + err = lfsr_file_sync_(lfs, file); + if (err) { + goto failed; + } + // update in-device state for (lfsr_omdir_t *o = lfs->omdirs; o; o = o->next) { if (lfsr_o_type(o->flags) == LFS_TYPE_REG diff --git a/lfs_util.h b/lfs_util.h index a3b6d671..07c74875 100644 --- a/lfs_util.h +++ b/lfs_util.h @@ -274,6 +274,13 @@ #define LFS_FORCEINLINE #endif +// Force a function to _not_ be inlined +#if !defined(LFS_NO_BUILTINS) && defined(__GNUC__) +#define LFS_NOINLINE __attribute__((noinline)) +#else +#define LFS_NOINLINE +#endif + // Builtin functions, these may be replaced by more efficient // toolchain-specific implementations. LFS_NO_BUILTINS falls back to a more