From 0f1862ac477ed8eaf7c34fa4be5e90ba5e6eb9cf Mon Sep 17 00:00:00 2001 From: Benjamin Bruun Date: Wed, 26 Aug 2026 16:45:04 +0200 Subject: [PATCH 1/2] perf: replace the per-file cache entries with a compact dir table Every file cost a 28 B list entry plus a malloc'd path string although only ten files can hold content at once; an 8 B hashed dir entry plus a fixed slot table cuts the 98-file profile's table from 6.7 KB of heap to 0.8 KB and makes the ten-buffer cap real. Signed-off-by: Benjamin Bruun --- lib/ss_cache.c | 193 ++++++++++++-------- lib/ss_cache.h | 91 +++++++--- lib/ss_fs.c | 391 +++++++++++++++++++++-------------------- tests/cache/src/main.c | 268 +++++++++++++--------------- tests/fs/src/main.c | 4 +- 5 files changed, 516 insertions(+), 431 deletions(-) diff --git a/lib/ss_cache.c b/lib/ss_cache.c index 418c9ca0..c5ea1985 100644 --- a/lib/ss_cache.c +++ b/lib/ss_cache.c @@ -7,119 +7,168 @@ #include #include +#include #include "ss_cache.h" #include LOG_MODULE_DECLARE(softsim, CONFIG_SOFTSIM_NRF_LOG_LEVEL); -#define SS_MAX_ENTRIES 10 - /* Fixed header preceding the variable-length name in each DIR record: * a 1-byte name length followed by a 2-byte (big-endian) NVS key. */ #define DIR_RECORD_HEADER_LEN 3 +/* The 8-byte entry is the point of this table; catch a padding surprise. */ +BUILD_ASSERT(sizeof(struct ss_dir_entry) == 8, "struct ss_dir_entry must stay 8 bytes"); + +/* FNV-1a. Case-sensitive on purpose, matching the strcmp lookup it replaced; + * the submodule's ss_profile_crc32() lowercases its input and would not. */ +static uint32_t fnv1a(const uint8_t *data, size_t len) +{ + uint32_t hash = 2166136261u; + + for (size_t i = 0; i < len; i++) { + hash = (hash ^ data[i]) * 16777619u; + } + + return hash; +} + /* See in ss_cache.h */ -struct cache_entry *f_cache_find_buffer(struct cache_entry *entry, struct ss_list *cache) +int ss_dir_table_from_blob(const uint8_t *blob, size_t size, struct ss_dir_entry **out) { - struct cache_entry *cursor; - struct cache_entry *no_hits_no_write_existing_buff = - NULL; /* Best case: no write needed, existing buffer with size >= min_buf_size */ - struct cache_entry *no_hits_no_write = - NULL; /* No write needed, existing buffer with size < min_buf_size */ - struct cache_entry *no_hits = NULL; /* Write needed but low hit count */ - - size_t cached_entries = 0; - size_t min_buf_size = entry->_l; - size_t min_hits_1 = 100, min_hits_2 = 100, min_hits_3 = 100; - - SS_LIST_FOR_EACH(cache, cursor, struct cache_entry, list) - { - if (cursor->buf) { - if (!cursor->_b_dirty && cursor->_b_size >= min_buf_size && - cursor->_cache_hits < min_hits_1) { - min_hits_1 = cursor->_cache_hits; - no_hits_no_write_existing_buff = cursor; - } - if (!cursor->_b_dirty && cursor->_cache_hits < min_hits_2) { - min_hits_2 = cursor->_cache_hits; - no_hits_no_write = cursor; - } - if (cursor->_cache_hits < min_hits_3) { - min_hits_3 = cursor->_cache_hits; - no_hits = cursor; - } - cached_entries++; + size_t cursor = 0; + size_t count = 0; + + *out = NULL; + + /* First pass: count the well-formed records, applying the same + * truncation rule as the fill below (a declared name that runs past + * the end of the blob must not be read). */ + while (cursor < size) { + uint8_t len = blob[cursor]; /* peek the name length */ + + if (cursor + DIR_RECORD_HEADER_LEN + len > size) { + LOG_WRN("DIR blob truncated; ignoring trailing %u byte(s)", + (unsigned)(size - cursor)); + break; } + cursor += DIR_RECORD_HEADER_LEN + len; + count++; } - /* Let cache grow to SS_MAX_ENTRIES */ - if (cached_entries < SS_MAX_ENTRIES) { - return NULL; + if (count == 0) { + return 0; } - if (no_hits_no_write_existing_buff) { - return no_hits_no_write_existing_buff; + struct ss_dir_entry *dir = SS_ALLOC_N(count * sizeof(struct ss_dir_entry)); + + if (!dir) { + LOG_ERR("Failed to allocate the directory table (%u entries)", (unsigned)count); + return -1; } - if (no_hits_no_write) { - return no_hits_no_write; + cursor = 0; + for (size_t i = 0; i < count; i++) { + uint8_t len = blob[cursor]; + uint16_t id = (blob[cursor + 1] << 8) | blob[cursor + 2]; + + dir[i].hash = fnv1a(&blob[cursor + DIR_RECORD_HEADER_LEN], len); + dir[i].key = id; + dir[i].flags = (id & 0xFF00) >> 8; + dir[i].hits = 0; + cursor += DIR_RECORD_HEADER_LEN + len; } - if (no_hits) { - return no_hits; + /* Two paths with the same hash would make lookups serve the wrong + * file; refuse the whole table instead. Quadratic, but only at init + * and only over a few hundred entries at most. */ + for (size_t i = 1; i < count; i++) { + for (size_t j = 0; j < i; j++) { + if (dir[i].hash == dir[j].hash) { + LOG_ERR("DIR paths %u and %u share hash 0x%08x; " + "refusing the table", + (unsigned)j, (unsigned)i, dir[i].hash); + SS_FREE(dir); + return -1; + } + } } - return NULL; + *out = dir; + return (int)count; } /* See in ss_cache.h */ -struct cache_entry *f_cache_find_by_name(const char *name, struct ss_list *cache) +int ss_dir_find(const struct ss_dir_entry *dir, size_t count, const char *name) { - struct cache_entry *cursor; + uint32_t hash = fnv1a((const uint8_t *)name, strlen(name)); - SS_LIST_FOR_EACH(cache, cursor, struct cache_entry, list) - { - if (strcmp(cursor->name, name) == 0) { - return cursor; + for (size_t i = 0; i < count; i++) { + if (dir[i].hash == hash) { + return (int)i; } } - return NULL; + return -1; } /* See in ss_cache.h */ -void generate_dir_table_from_blob(struct ss_list *dirs, uint8_t *blob, size_t size) +int ss_slot_find(const struct ss_cache_slot *slots, size_t count, uint16_t dir_idx) { - size_t cursor = 0; - - while (cursor < size) { - uint8_t len = blob[cursor]; /* peek the name length */ - /* Check if the record header and name fit in the remaining blob */ - if (cursor + DIR_RECORD_HEADER_LEN + len > size) { - LOG_WRN("DIR blob truncated; ignoring trailing %u byte(s)", - (unsigned)(size - cursor)); - break; + for (size_t i = 0; i < count; i++) { + if (slots[i].buf && slots[i].dir_idx == dir_idx) { + return (int)i; } - cursor++; + } - uint16_t id = (blob[cursor] << 8) | blob[cursor + 1]; + return -1; +} - cursor += 2; +/* See in ss_cache.h */ +int ss_slot_acquire(const struct ss_dir_entry *dir, const struct ss_cache_slot *slots, size_t count, + size_t want_len) +{ + int no_hits_no_write_existing_buff = + -1; /* Best case: no write needed, buffer size >= want_len */ + int no_hits_no_write = -1; /* No write needed, buffer too small */ + int no_hits = -1; /* Write needed but low hit count */ + + /* Above the hit counter's ceiling, so a fully-hit cache still yields a + * victim: the slot table is the capacity, there is no growing past it. */ + size_t min_hits_1 = 0x100, min_hits_2 = 0x100, min_hits_3 = 0x100; + + /* Let the cache grow to capacity before evicting anything. */ + for (size_t i = 0; i < count; i++) { + if (!slots[i].buf) { + return (int)i; + } + } - char *name = SS_ALLOC_N(len + 1); - memcpy(name, &blob[cursor], len); - name[len] = '\0'; - cursor += len; + for (size_t i = 0; i < count; i++) { + uint8_t hits = dir[slots[i].dir_idx].hits; - struct cache_entry *entry = SS_ALLOC(struct cache_entry); - memset(entry, 0, sizeof(struct cache_entry)); + if (!slots[i]._b_dirty && slots[i]._b_size >= want_len && hits < min_hits_1) { + min_hits_1 = hits; + no_hits_no_write_existing_buff = (int)i; + } + if (!slots[i]._b_dirty && hits < min_hits_2) { + min_hits_2 = hits; + no_hits_no_write = (int)i; + } + if (hits < min_hits_3) { + min_hits_3 = hits; + no_hits = (int)i; + } + } - entry->key = id; - entry->name = name; - entry->_flags = (id & 0xFF00) >> 8; - entry->buf = NULL; + if (no_hits_no_write_existing_buff >= 0) { + return no_hits_no_write_existing_buff; + } - ss_list_put(dirs, &entry->list); + if (no_hits_no_write >= 0) { + return no_hits_no_write; } + + return no_hits; } diff --git a/lib/ss_cache.h b/lib/ss_cache.h index 7549fb5b..d311da95 100644 --- a/lib/ss_cache.h +++ b/lib/ss_cache.h @@ -9,54 +9,93 @@ #include #include -#include - #define FS_READ_ONLY (1UL << 8) #define FS_COMMIT_ON_CLOSE (1UL << 7) /* Commit changes to NVS on close */ -struct cache_entry { - struct ss_list list; - uint16_t key; /* NVS key */ - uint8_t _flags; /* Part of ID is used for flags */ +/* How many files may hold a content buffer at once. */ +#define SS_MAX_ENTRIES 10 + +/* One directory record per file. The path itself is not kept: lookups compare + * a 32-bit FNV-1a hash of the path instead, which is what makes the entry 8 + * bytes rather than 28-plus-a-string. ss_dir_table_from_blob() refuses a table + * with colliding hashes, so within a table a hash identifies exactly one file; + * the residual risk is a path that is NOT in the table hashing onto one that + * is (~n/2^32 per lookup), which would be served instead of failing. */ +struct ss_dir_entry { + uint32_t hash; /* FNV-1a of the path string */ + uint16_t key; /* NVS key (all 16 bits, including the flag byte) */ + uint8_t flags; /* Derived from (key >> 8); mutable at runtime */ + uint8_t hits; /* Open count, saturating; biases eviction */ +}; + +/* One buffered file. A slot with buf == NULL is free and its other fields are + * meaningless. A file handle (ss_FILE) is a pointer to its slot and does not + * survive eviction of that file; the storage backend opens one file at a time + * and closes it before the next open, so an open file is never evicted. */ +struct ss_cache_slot { + uint8_t *buf; /* Cached content; NULL = slot free */ + uint16_t dir_idx; /* Owning entry in the directory table */ uint16_t _p; /* Local 'file' pointer (ftell, fseek, etc.) */ uint16_t _l; /* Local 'file' length */ - uint8_t *buf; /* In case content is cached */ uint16_t _b_size; /* Memory allocated for buf */ uint8_t _b_dirty; /* Buf is divergent from NVS */ - uint8_t _cache_hits; - char *name; /* Path/key for lookup */ }; /** - * @brief Find a suitable cache entry with a buffer that can be re-used + * @brief Build the directory table from the "DIR" file content. * - * @param entry Pointer to a cache entry - * @param cache Pointer to a cache + * Each blob record is [name_len | id_hi | id_lo | name[name_len]]. A record + * that runs past the end of the blob ends the parse (truncated flash content + * must not be read past). * - * @return Pointer to a suitable cache entry, or NULL if none found + * @param blob Pointer to blob of data + * @param size Size of blob + * @param out Receives the allocated table (NULL when the return is <= 0) + * + * @return Number of entries, or -1 on allocation failure or when two paths + * hash identically (the table would serve the wrong file; fail loudly) */ -struct cache_entry *f_cache_find_buffer(struct cache_entry *entry, struct ss_list *cache); +int ss_dir_table_from_blob(const uint8_t *blob, size_t size, struct ss_dir_entry **out); /** - * @brief Find a cache entry by name + * @brief Find a directory entry by path. * - * @param name Name of the cache entry to find - * @param cache Pointer to a cache + * @param dir Directory table + * @param count Number of entries in the table + * @param name Path to look up * - * @return Pointer to the cache entry with the given name, or NULL if not found + * @return Index of the entry, or -1 if not found */ -struct cache_entry *f_cache_find_by_name(const char *name, struct ss_list *cache); +int ss_dir_find(const struct ss_dir_entry *dir, size_t count, const char *name); /** - * @brief Generate the directory structure based on the content in the "DIR" file. + * @brief Find the slot buffering a given file, if any. * - * The DIR file encodes ID (used to locate the actual file in flash) and the name - * of the file. + * @param slots Slot table + * @param count Number of slots + * @param dir_idx Directory index of the file * - * @param dirs Linked list to populate - * @param blob Pointer to blob of data - * @param size Size of blob + * @return Index of the slot, or -1 if the file is not buffered + */ +int ss_slot_find(const struct ss_cache_slot *slots, size_t count, uint16_t dir_idx); + +/** + * @brief Pick the slot to load a file into. + * + * A free slot is returned first (the cache grows to its capacity before + * anything is evicted). Once full, the victim preference is: clean with a + * buffer already big enough for want_len and fewest hits, then clean with + * fewest hits, then simply fewest hits. The caller writes a dirty victim + * back and reuses or frees its buffer. + * + * @param dir Directory table (source of the per-file hit counts) + * @param slots Slot table + * @param count Number of slots + * @param want_len Length of the file about to be loaded + * + * @return Index of the slot to use, or -1 when count is 0 */ -void generate_dir_table_from_blob(struct ss_list *dirs, uint8_t *blob, size_t size); +int ss_slot_acquire(const struct ss_dir_entry *dir, const struct ss_cache_slot *slots, size_t count, + size_t want_len); #endif /* _F_CACHE_H_ */ diff --git a/lib/ss_fs.c b/lib/ss_fs.c index 4ce26b96..0eb2e6cb 100644 --- a/lib/ss_fs.c +++ b/lib/ss_fs.c @@ -13,7 +13,6 @@ #include "ss_cache.h" #include -#include #include #include #include @@ -63,7 +62,13 @@ LOG_MODULE_DECLARE(softsim, CONFIG_SOFTSIM_NRF_LOG_LEVEL); #endif static struct nvs_fs fs; -static struct ss_list fs_cache; + +/* One 8-byte entry per file, in one allocation; the paths themselves live + * nowhere (lookups go through the hash). Content buffers are attached to the + * fixed slot table, never to the directory. */ +static struct ss_dir_entry *fs_dir; +static size_t fs_dir_count; +static struct ss_cache_slot fs_slots[SS_MAX_ENTRIES]; static uint8_t fs_is_initialized = 0; static uint8_t default_imsi[] = {0x08, 0x09, 0x10, 0x10, 0x00, 0x00, 0x00, 0x00, 0x10}; @@ -76,17 +81,18 @@ static uint8_t default_imsi[] = {0x08, 0x09, 0x10, 0x10, 0x00, 0x00, 0x00, 0x00, char storage_path[SS_STORAGE_PATH_MAX] = ""; /** - * @brief Internal function to read NVS data into cache + * @brief Internal function to read NVS data into a cache slot * - * @param entry Pointer to the cache entry to read data into + * @param dir_idx Directory index of the file to load + * @param len Length of the file in NVS * - * This function reads the content of a cache entry from NVS and stores it in the - * cache's buffer. If the buffer is already allocated, it reuses it if possible. - * If not, it allocates a new buffer of the required size. + * Acquires a slot (evicting by the ss_slot_acquire() preference once the + * cache is full), writes a dirty victim back to NVS, reuses the victim's + * buffer when it is large enough, and reads the file content from NVS. * - * It also handles writing dirty buffers back to NVS if necessary. - * */ -static void ss_read_nvs_to_cache(struct cache_entry *entry); + * @return The slot holding the content, or NULL on allocation/NVS failure + */ +static struct ss_cache_slot *ss_load_to_slot(uint16_t dir_idx, uint16_t len); /* See in the onomondo-uicc submodule */ int ss_init_fs(void) @@ -95,7 +101,6 @@ int ss_init_fs(void) return 0; /* Already initialized */ } - ss_list_init(&fs_cache); uint8_t *data = NULL; size_t len = 0; @@ -122,8 +127,8 @@ int ss_init_fs(void) len = rc; /* Read DIR_ENTRY from NVS - * This is used to construct a linked list that - * serves as a cache and lookup table for the filesystem + * This is used to construct the directory table that serves as the + * lookup table for the filesystem */ if (!data && rc) { data = SS_ALLOC_N(len * sizeof(uint8_t)); @@ -131,47 +136,42 @@ int ss_init_fs(void) __ASSERT_NO_MSG(rc == len); } - ss_list_init(&fs_cache); - generate_dir_table_from_blob(&fs_cache, data, len); + memset(fs_slots, 0, sizeof(fs_slots)); + rc = ss_dir_table_from_blob(data, data ? len : 0, &fs_dir); + fs_dir_count = rc > 0 ? (size_t)rc : 0; - if (ss_list_empty(&fs_cache)) { - goto out; + if (fs_dir_count > 0) { + fs_is_initialized++; } - fs_is_initialized++; - -out: SS_FREE(data); - return ss_list_empty(&fs_cache); + return fs_dir_count == 0; } /* See in the onomondo-uicc submodule */ int ss_deinit_fs(void) { - /* TODO: check if DIR entry is still valid. If not recreate and write. - * Will NVS only commit if there are changes? If so, we can just recreate - * and let NVS do the compare. - */ - - struct cache_entry *cursor, *pre_cursor; + /* Commit changes to NVS and free the cache buffers and the table */ + for (size_t i = 0; i < SS_MAX_ENTRIES; i++) { + struct ss_cache_slot *slot = &fs_slots[i]; - /* Free all memory allocated by cache and commit changes to NVS */ - SS_LIST_FOR_EACH_SAVE(&fs_cache, cursor, pre_cursor, struct cache_entry, list) - { - if (cursor->_b_dirty) { - LOG_INF("SoftSIM stop - committing %s to NVS", cursor->name); - nvs_write(&fs, cursor->key, cursor->buf, cursor->_l); + if (!slot->buf) { + continue; } - ss_list_remove(&cursor->list); - - if (cursor->buf) { - SS_FREE(cursor->buf); + if (slot->_b_dirty) { + LOG_INF("SoftSIM stop - committing key 0x%04x to NVS", + fs_dir[slot->dir_idx].key); + nvs_write(&fs, fs_dir[slot->dir_idx].key, slot->buf, slot->_l); } - SS_FREE(cursor->name); - SS_FREE(cursor); + + SS_FREE(slot->buf); } + memset(fs_slots, 0, sizeof(fs_slots)); + SS_FREE(fs_dir); + fs_dir = NULL; + fs_dir_count = 0; fs_is_initialized = 0; return 0; @@ -180,38 +180,34 @@ int ss_deinit_fs(void) /* See in the onomondo-uicc submodule */ ss_FILE ss_fopen(char *path, char *mode) { - struct cache_entry *cursor = NULL; - int rc = 0; + int dir_idx = ss_dir_find(fs_dir, fs_dir_count, path); - cursor = f_cache_find_by_name(path, &fs_cache); - if (!cursor) { + if (dir_idx < 0) { return NULL; } - /* Currently not used. - * Could potentially be used in the future to re-arrange order. - * Initial order is ordered by frequency already so not big optimizations can - * be achieved. - */ - if (cursor->_cache_hits < 0xFF) { - cursor->_cache_hits++; + /* Currently only used to bias eviction towards rarely-opened files. */ + if (fs_dir[dir_idx].hits < 0xFF) { + fs_dir[dir_idx].hits++; } - if (!cursor->_l) { - rc = nvs_read(&fs, cursor->key, NULL, 0); - if (rc < 0) { - return NULL; /* TODO: This can not happen */ - } else { - cursor->_l = rc; - } + int slot_idx = ss_slot_find(fs_slots, SS_MAX_ENTRIES, (uint16_t)dir_idx); + + if (slot_idx >= 0) { + /* Reset internal read/write pointer */ + fs_slots[slot_idx]._p = 0; + return &fs_slots[slot_idx]; } - /* Reset internal read/write pointer */ - cursor->_p = 0; + int rc = nvs_read(&fs, fs_dir[dir_idx].key, NULL, 0); - /* Guarantee buffer contains valid data */ - ss_read_nvs_to_cache(cursor); - return (void *)cursor; + if (rc < 0) { + return NULL; + } + + /* NULL on allocation or NVS failure: fail closed rather than hand out + * a handle without content behind it. */ + return ss_load_to_slot((uint16_t)dir_idx, (uint16_t)rc); } /* Strong override of the weak ss_file_size declared in @@ -221,22 +217,21 @@ ss_FILE ss_fopen(char *path, char *mode) * nrf-softsim cache layer instead. */ int ss_file_size(const char *path) { - struct cache_entry *entry = f_cache_find_by_name(path, &fs_cache); + int dir_idx = ss_dir_find(fs_dir, fs_dir_count, path); - if (!entry) { + if (dir_idx < 0) { return -1; } - if (!entry->_l) { - int rc = nvs_read(&fs, entry->key, NULL, 0); + int slot_idx = ss_slot_find(fs_slots, SS_MAX_ENTRIES, (uint16_t)dir_idx); - if (rc < 0) { - return -1; - } - entry->_l = rc; + if (slot_idx >= 0) { + return (int)fs_slots[slot_idx]._l; } - return (int)entry->_l; + int rc = nvs_read(&fs, fs_dir[dir_idx].key, NULL, 0); + + return rc < 0 ? -1 : rc; } /* See in the onomondo-uicc submodule */ @@ -246,92 +241,97 @@ size_t ss_fread(void *ptr, size_t size, size_t nmemb, ss_FILE fp) return 0; } - struct cache_entry *entry = (struct cache_entry *)fp; - size_t max_element_to_return = (entry->_l - entry->_p) / size; + struct ss_cache_slot *slot = (struct ss_cache_slot *)fp; + size_t max_element_to_return = (slot->_l - slot->_p) / size; size_t element_to_return = nmemb > max_element_to_return ? max_element_to_return : nmemb; /* Copy data from cache to user buffer */ - memcpy(ptr, entry->buf + entry->_p, element_to_return * size); + memcpy(ptr, slot->buf + slot->_p, element_to_return * size); /* Update internal read/write pointer */ - entry->_p += element_to_return * size; + slot->_p += element_to_return * size; return element_to_return; } -void ss_read_nvs_to_cache(struct cache_entry *entry) +struct ss_cache_slot *ss_load_to_slot(uint16_t dir_idx, uint16_t len) { - struct cache_entry *tmp; + int idx = ss_slot_acquire(fs_dir, fs_slots, SS_MAX_ENTRIES, len); - if (entry->buf) { - return; + if (idx < 0) { + return NULL; } - tmp = f_cache_find_buffer(entry, &fs_cache); - + struct ss_cache_slot *slot = &fs_slots[idx]; uint8_t *buffer_to_use = NULL; size_t buffer_size = 0; - if (tmp) { - if (tmp->_b_dirty) { - LOG_DBG("Cache entry %s is dirty, writing to NVS", tmp->name); - nvs_write(&fs, tmp->key, tmp->buf, tmp->_l); + if (slot->buf) { + if (slot->_b_dirty) { + LOG_DBG("Cache slot for key 0x%04x is dirty, writing to NVS", + fs_dir[slot->dir_idx].key); + nvs_write(&fs, fs_dir[slot->dir_idx].key, slot->buf, slot->_l); } - if (entry->_l > tmp->_b_size) { - SS_FREE(tmp->buf); + if (len > slot->_b_size) { + SS_FREE(slot->buf); } else { - buffer_size = tmp->_b_size; - buffer_to_use = tmp->buf; + buffer_size = slot->_b_size; + buffer_to_use = slot->buf; memset(buffer_to_use, 0, buffer_size); } - tmp->buf = NULL; - tmp->_b_size = 0; - tmp->_b_dirty = 0; + slot->buf = NULL; + slot->_b_size = 0; + slot->_b_dirty = 0; } if (!buffer_to_use) { - buffer_size = entry->_l; + buffer_size = len; LOG_DBG("Allocating buffer of size %d", buffer_size); buffer_to_use = SS_ALLOC_N(buffer_size * sizeof(uint8_t)); } if (!buffer_to_use) { LOG_ERR("Failed to allocate buffer of size %d", buffer_size); - return; + return NULL; } - int rc = nvs_read(&fs, entry->key, buffer_to_use, entry->_l); + int rc = nvs_read(&fs, fs_dir[dir_idx].key, buffer_to_use, len); if (rc < 0) { LOG_ERR("NVS read failed: %d", rc); SS_FREE(buffer_to_use); - return; + return NULL; } - entry->buf = buffer_to_use; - entry->_b_size = buffer_size; - entry->_b_dirty = 0; + slot->buf = buffer_to_use; + slot->dir_idx = dir_idx; + slot->_p = 0; + slot->_l = len; + slot->_b_size = buffer_size; + slot->_b_dirty = 0; + + return slot; } char *ss_fgets(char *str, int n, ss_FILE fp) { - struct cache_entry *entry = (struct cache_entry *)fp; + struct ss_cache_slot *slot = (struct ss_cache_slot *)fp; - if (!entry) { + if (!slot) { LOG_ERR("Invalid file pointer, ss_fgets failed"); return NULL; } - if (entry->_p >= entry->_l) { + if (slot->_p >= slot->_l) { /* No more data to read */ return NULL; } int idx = 0; /* Destination buffer index */ - while (entry->_p < entry->_l && idx < n - 1 && entry->buf[entry->_p] != '\0' && - entry->buf[entry->_p] != '\n') { - str[idx++] = entry->buf[entry->_p++]; + while (slot->_p < slot->_l && idx < n - 1 && slot->buf[slot->_p] != '\0' && + slot->buf[slot->_p] != '\n') { + str[idx++] = slot->buf[slot->_p++]; } str[idx] = '\0'; @@ -340,47 +340,49 @@ char *ss_fgets(char *str, int n, ss_FILE fp) int ss_fclose(ss_FILE fp) { - struct cache_entry *entry = (struct cache_entry *)fp; + struct ss_cache_slot *slot = (struct ss_cache_slot *)fp; - if (!entry) { + if (!slot) { LOG_ERR("Invalid file pointer, ss_fclose failed"); return -1; } - if (entry->_flags & FS_READ_ONLY) { + uint8_t flags = fs_dir[slot->dir_idx].flags; + + if (flags & FS_READ_ONLY) { goto out; } - if (entry->_flags & FS_COMMIT_ON_CLOSE) { - if (entry->_b_dirty) { - nvs_write(&fs, entry->key, entry->buf, entry->_l); + if (flags & FS_COMMIT_ON_CLOSE) { + if (slot->_b_dirty) { + nvs_write(&fs, fs_dir[slot->dir_idx].key, slot->buf, slot->_l); } - entry->_b_dirty = 0; + slot->_b_dirty = 0; } out: - entry->_p = 0; /* TODO: Resetting internal read/write pointer not needed? */ + slot->_p = 0; /* TODO: Resetting internal read/write pointer not needed? */ return 0; } int ss_fseek(ss_FILE fp, long offset, int whence) { - struct cache_entry *entry = (struct cache_entry *)fp; + struct ss_cache_slot *slot = (struct ss_cache_slot *)fp; - if (!entry) { + if (!slot) { LOG_ERR("Invalid file pointer, ss_fseek failed"); return -1; } if (whence == SEEK_SET) { - entry->_p = offset; + slot->_p = offset; } else if (whence == SEEK_CUR) { - entry->_p += offset; - if (entry->_p >= entry->_l) { - entry->_p = entry->_l - 1; + slot->_p += offset; + if (slot->_p >= slot->_l) { + slot->_p = slot->_l - 1; } } else if (whence == SEEK_END) { - entry->_p = entry->_l - offset; + slot->_p = slot->_l - offset; } return 0; @@ -388,41 +390,42 @@ int ss_fseek(ss_FILE fp, long offset, int whence) long ss_ftell(ss_FILE fp) { - struct cache_entry *entry = (struct cache_entry *)fp; + struct ss_cache_slot *slot = (struct ss_cache_slot *)fp; - if (!entry) { + if (!slot) { return -1; } - return entry->_p; + return slot->_p; } int ss_fputc(int c, ss_FILE fp) { - struct cache_entry *entry = (struct cache_entry *)fp; + struct ss_cache_slot *slot = (struct ss_cache_slot *)fp; - if (!entry) { + if (!slot) { return -1; } - if (entry->_p >= entry->_b_size) { - uint8_t *old_buffer = entry->buf; - size_t old_size = entry->_b_size; - entry->buf = SS_ALLOC_N(entry->_b_size + 20); + if (slot->_p >= slot->_b_size) { + uint8_t *old_buffer = slot->buf; + size_t old_size = slot->_b_size; + + slot->buf = SS_ALLOC_N(slot->_b_size + 20); - if (!entry->buf) { - entry->buf = old_buffer; + if (!slot->buf) { + slot->buf = old_buffer; return -1; } - memcpy(entry->buf, old_buffer, old_size); + memcpy(slot->buf, old_buffer, old_size); SS_FREE(old_buffer); - entry->_b_size += 20; + slot->_b_size += 20; } - entry->buf[entry->_p++] = (uint8_t)c; - entry->_b_dirty = 1; - entry->_l = entry->_l >= entry->_p ? entry->_l : entry->_p; + slot->buf[slot->_p++] = (uint8_t)c; + slot->_b_dirty = 1; + slot->_l = slot->_l >= slot->_p ? slot->_l : slot->_p; return c; } @@ -454,63 +457,77 @@ int ss_rmdir(const char *path) int ss_remove(const char *path) { - struct cache_entry *entry = f_cache_find_by_name(path, &fs_cache); + int dir_idx = ss_dir_find(fs_dir, fs_dir_count, path); - if (!entry) { + if (dir_idx < 0) { return -1; } - ss_list_remove(&entry->list); - nvs_delete(&fs, entry->key); + int slot_idx = ss_slot_find(fs_slots, SS_MAX_ENTRIES, (uint16_t)dir_idx); - if (entry->buf) { - SS_FREE(entry->buf); + if (slot_idx >= 0) { + /* The file is going away; its buffer dies with it, dirty or not. */ + SS_FREE(fs_slots[slot_idx].buf); + memset(&fs_slots[slot_idx], 0, sizeof(fs_slots[slot_idx])); } - SS_FREE(entry->name); - SS_FREE(entry); + nvs_delete(&fs, fs_dir[dir_idx].key); + + /* Close the hole with the last entry, re-pointing its slot if buffered. */ + size_t last = fs_dir_count - 1; + + if ((size_t)dir_idx != last) { + fs_dir[dir_idx] = fs_dir[last]; + + int moved = ss_slot_find(fs_slots, SS_MAX_ENTRIES, (uint16_t)last); + + if (moved >= 0) { + fs_slots[moved].dir_idx = (uint16_t)dir_idx; + } + } + fs_dir_count--; return 0; } size_t ss_fwrite(const void *ptr, size_t size, size_t count, ss_FILE fp) { - struct cache_entry *entry = (struct cache_entry *)fp; + struct ss_cache_slot *slot = (struct ss_cache_slot *)fp; - if (!entry) { + if (!slot) { return -1; } - const size_t requiredBufferSize = entry->_p + size * count; + const size_t requiredBufferSize = slot->_p + size * count; - if (requiredBufferSize > entry->_b_size) { - uint8_t *oldBuffer = entry->buf; - const size_t oldSize = entry->_b_size; + if (requiredBufferSize > slot->_b_size) { + uint8_t *oldBuffer = slot->buf; + const size_t oldSize = slot->_b_size; - entry->buf = SS_ALLOC_N(requiredBufferSize); + slot->buf = SS_ALLOC_N(requiredBufferSize); - if (!entry->buf) { - entry->buf = oldBuffer; + if (!slot->buf) { + slot->buf = oldBuffer; return -1; } else { - entry->_b_size = requiredBufferSize; + slot->_b_size = requiredBufferSize; } - memcpy(entry->buf, oldBuffer, oldSize); + memcpy(slot->buf, oldBuffer, oldSize); SS_FREE(oldBuffer); } - const size_t buffer_left = entry->_b_size - entry->_p; + const size_t buffer_left = slot->_b_size - slot->_p; const size_t elements_to_copy = buffer_left > size * count ? count : buffer_left / size; const uint8_t content_is_different = - memcmp(entry->buf + entry->_p, ptr, size * elements_to_copy); + memcmp(slot->buf + slot->_p, ptr, size * elements_to_copy); if (content_is_different) { - memcpy(entry->buf + entry->_p, ptr, size * elements_to_copy); - entry->_b_dirty = 1; + memcpy(slot->buf + slot->_p, ptr, size * elements_to_copy); + slot->_b_dirty = 1; } - entry->_p += size * elements_to_copy; + slot->_p += size * elements_to_copy; return elements_to_copy; } @@ -524,14 +541,14 @@ int port_check_provisioned(void) { int ret; uint8_t buffer[IMSI_BIN_LEN] = {0}; - struct cache_entry *entry = - (struct cache_entry *)f_cache_find_by_name(IMSI_PATH, &fs_cache); - if (!entry) { + int dir_idx = ss_dir_find(fs_dir, fs_dir_count, IMSI_PATH); + + if (dir_idx < 0) { LOG_DBG("IMSI EF not in filesystem cache => not provisioned"); return 0; } - ret = nvs_read(&fs, entry->key, buffer, IMSI_BIN_LEN); + ret = nvs_read(&fs, fs_dir[dir_idx].key, buffer, IMSI_BIN_LEN); if (ret < 0) { return 0; } @@ -587,51 +604,51 @@ int port_provision(struct ss_profile *profile) a004[header_size] = KIC_TAG; a004[header_size + KEY_BIN_LEN] = KID_TAG; - struct cache_entry *entry = - (struct cache_entry *)f_cache_find_by_name(IMSI_PATH, &fs_cache); - if (!entry) { + int dir_idx = ss_dir_find(fs_dir, fs_dir_count, IMSI_PATH); + + if (dir_idx < 0) { LOG_ERR("EF IMSI not in filesystem cache"); goto out_err; } LOG_INF("Provisioning SoftSIM 1/4"); - if (nvs_write(&fs, entry->key, imsi, IMSI_BIN_LEN) < 0) { + if (nvs_write(&fs, fs_dir[dir_idx].key, imsi, IMSI_BIN_LEN) < 0) { goto out_err; } - entry->_flags = 0; + fs_dir[dir_idx].flags = 0; LOG_INF("Provisioning SoftSIM 2/4"); - entry = (struct cache_entry *)f_cache_find_by_name(ICCID_PATH, &fs_cache); - if (!entry) { + dir_idx = ss_dir_find(fs_dir, fs_dir_count, ICCID_PATH); + if (dir_idx < 0) { LOG_ERR("EF ICCID not in filesystem cache"); goto out_err; } - if (nvs_write(&fs, entry->key, iccid, ICCID_BIN_LEN) < 0) { + if (nvs_write(&fs, fs_dir[dir_idx].key, iccid, ICCID_BIN_LEN) < 0) { goto out_err; } - entry->_flags = 0; + fs_dir[dir_idx].flags = 0; LOG_INF("Provisioning SoftSIM 3/4"); - entry = (struct cache_entry *)f_cache_find_by_name(A001_PATH, &fs_cache); - if (!entry) { + dir_idx = ss_dir_find(fs_dir, fs_dir_count, A001_PATH); + if (dir_idx < 0) { LOG_ERR("EF A001 not in filesystem cache"); goto out_err; } - if (nvs_write(&fs, entry->key, a001, sizeof(a001)) < 0) { + if (nvs_write(&fs, fs_dir[dir_idx].key, a001, sizeof(a001)) < 0) { goto out_err; } - entry->_flags = 0; + fs_dir[dir_idx].flags = 0; LOG_INF("Provisioning SoftSIM 4/4"); - entry = (struct cache_entry *)f_cache_find_by_name(A004_PATH, &fs_cache); - if (!entry) { + dir_idx = ss_dir_find(fs_dir, fs_dir_count, A004_PATH); + if (dir_idx < 0) { LOG_ERR("EF A004 not in filesystem cache"); goto out_err; } - if (nvs_write(&fs, entry->key, a004, sizeof(a004)) < 0) { + if (nvs_write(&fs, fs_dir[dir_idx].key, a004, sizeof(a004)) < 0) { goto out_err; } - entry->_flags = 0; + fs_dir[dir_idx].flags = 0; /* Optionally provision EF.SMSP. The profile may carry the SMS-parameter * record (profile->SMSP) and/or just the service-centre address @@ -645,19 +662,19 @@ int port_provision(struct ss_profile *profile) if (have_smsp || have_smsc) { LOG_INF("Provisioning SoftSIM EF.SMSP"); - entry = (struct cache_entry *)f_cache_find_by_name(SMSP_PATH, &fs_cache); - if (!entry) { + dir_idx = ss_dir_find(fs_dir, fs_dir_count, SMSP_PATH); + if (dir_idx < 0) { LOG_ERR("EF.SMSP not in filesystem cache"); goto out_err; } uint8_t smsp[SMSP_RECORD_SIZE * 2]; /* 104: holds a 2-record EF.SMSP */ - int ef_len = nvs_read(&fs, entry->key, NULL, 0); + int ef_len = nvs_read(&fs, fs_dir[dir_idx].key, NULL, 0); if (ef_len < SMSC_REC_OFFSET + (int)(SMSC_LEN / 2) || ef_len > (int)sizeof(smsp)) { LOG_ERR("Unexpected EF.SMSP length: %d", ef_len); goto out_err; } - if (nvs_read(&fs, entry->key, smsp, ef_len) != ef_len) { + if (nvs_read(&fs, fs_dir[dir_idx].key, smsp, ef_len) != ef_len) { LOG_ERR("Failed to read EF.SMSP"); goto out_err; } @@ -672,10 +689,10 @@ int port_provision(struct ss_profile *profile) sizeof(smsp) - SMSC_REC_OFFSET); } - if (nvs_write(&fs, entry->key, smsp, ef_len) < 0) { + if (nvs_write(&fs, fs_dir[dir_idx].key, smsp, ef_len) < 0) { goto out_err; } - entry->_flags = 0; + fs_dir[dir_idx].flags = 0; } LOG_INF("SoftSIM provisioned"); diff --git a/tests/cache/src/main.c b/tests/cache/src/main.c index 1670a726..df1d2ecb 100644 --- a/tests/cache/src/main.c +++ b/tests/cache/src/main.c @@ -4,11 +4,11 @@ * * Unit tests for lib/ss_cache.c. * - * generate_dir_table_from_blob() decodes the directory table read back from - * flash, so it is an input-validation surface fed by potentially corrupt data; - * it already had an out-of-bounds read fixed once (#145). The lookup helpers - * decide which cached buffer gets evicted, which is where several of the - * filesystem's memory bugs have originated. + * ss_dir_table_from_blob() decodes the directory table read back from flash, + * so it is an input-validation surface fed by potentially corrupt data; its + * predecessor already had an out-of-bounds read fixed once (#145). The lookup + * helpers decide which cached buffer gets evicted, which is where several of + * the filesystem's memory bugs have originated. */ #include @@ -36,55 +36,28 @@ static size_t put_record(uint8_t *buf, size_t pos, uint16_t id, const char *name return pos + len; } -static size_t dir_count(struct ss_list *dirs) -{ - struct cache_entry *cursor; - size_t n = 0; - - SS_LIST_FOR_EACH(dirs, cursor, struct cache_entry, list) - { - n++; - } - - return n; -} - -static void dir_free(struct ss_list *dirs) -{ - while (!ss_list_empty(dirs)) { - struct cache_entry *entry = SS_LIST_GET_NEXT(dirs, struct cache_entry, list); - - ss_list_remove(&entry->list); - SS_FREE(entry->name); - SS_FREE(entry); - } -} - ZTEST_SUITE(softsim_cache, NULL, NULL, NULL, NULL, NULL); -/* --- generate_dir_table_from_blob ------------------------------------------ */ +/* --- ss_dir_table_from_blob ------------------------------------------------- */ ZTEST(softsim_cache, test_decode_two_records) { uint8_t blob[64]; - struct ss_list dirs; + struct ss_dir_entry *dir; size_t len; - ss_list_init(&dirs); len = put_record(blob, 0, 0x0002, "/3f00/2fe2"); len = put_record(blob, len, 0x0003, "/3f00/a001"); - generate_dir_table_from_blob(&dirs, blob, len); - - zassert_equal(dir_count(&dirs), 2, "both records should decode"); + int n = ss_dir_table_from_blob(blob, len, &dir); - struct cache_entry *first = SS_LIST_GET_NEXT(&dirs, struct cache_entry, list); + zassert_equal(n, 2, "both records should decode"); + zassert_equal(dir[0].key, 0x0002); + zassert_equal(ss_dir_find(dir, n, "/3f00/2fe2"), 0, "the first path must map to entry 0"); + zassert_equal(ss_dir_find(dir, n, "/3f00/a001"), 1, "the second path must map to entry 1"); + zassert_equal(dir[0].hits, 0, "a freshly decoded entry must start unopened"); - zassert_equal(first->key, 0x0002); - zassert_str_equal(first->name, "/3f00/2fe2"); - zassert_is_null(first->buf, "a freshly decoded entry must not claim a buffer"); - - dir_free(&dirs); + SS_FREE(dir); } /* @@ -96,10 +69,9 @@ ZTEST(softsim_cache, test_decode_two_records) ZTEST(softsim_cache, test_truncated_tail_is_ignored) { uint8_t blob[64]; - struct ss_list dirs; + struct ss_dir_entry *dir; size_t len; - ss_list_init(&dirs); len = put_record(blob, 0, 0x0002, "/3f00/2fe2"); /* Second record claims a 32-byte name but only 4 bytes remain. */ @@ -108,63 +80,56 @@ ZTEST(softsim_cache, test_truncated_tail_is_ignored) blob[len++] = 0x07; blob[len++] = 'x'; - generate_dir_table_from_blob(&dirs, blob, len); + int n = ss_dir_table_from_blob(blob, len, &dir); - zassert_equal(dir_count(&dirs), 1, "truncated trailing record must be dropped"); + zassert_equal(n, 1, "truncated trailing record must be dropped"); - dir_free(&dirs); + SS_FREE(dir); } ZTEST(softsim_cache, test_declared_name_longer_than_whole_blob) { uint8_t blob[8] = {0xff, 0x00, 0x02, 'a', 'b', 'c', 'd', 'e'}; - struct ss_list dirs; - - ss_list_init(&dirs); - generate_dir_table_from_blob(&dirs, blob, sizeof(blob)); + struct ss_dir_entry *dir; - zassert_equal(dir_count(&dirs), 0, "a single over-long record must yield nothing"); + int n = ss_dir_table_from_blob(blob, sizeof(blob), &dir); - dir_free(&dirs); + zassert_equal(n, 0, "a single over-long record must yield nothing"); + zassert_is_null(dir, "an empty result must not hand out a table"); } /* Erased flash reads back as 0xFF; it must not decode into anything. */ ZTEST(softsim_cache, test_erased_flash_blob_decodes_to_nothing) { uint8_t blob[128]; - struct ss_list dirs; + struct ss_dir_entry *dir; memset(blob, 0xff, sizeof(blob)); - ss_list_init(&dirs); - generate_dir_table_from_blob(&dirs, blob, sizeof(blob)); + int n = ss_dir_table_from_blob(blob, sizeof(blob), &dir); - zassert_equal(dir_count(&dirs), 0, "0xFF fill must not produce entries"); - - dir_free(&dirs); + zassert_equal(n, 0, "0xFF fill must not produce entries"); + zassert_is_null(dir); } ZTEST(softsim_cache, test_zero_length_name) { uint8_t blob[16]; - struct ss_list dirs; + struct ss_dir_entry *dir; size_t len; - ss_list_init(&dirs); len = put_record(blob, 0, 0x0005, ""); len = put_record(blob, len, 0x0006, "/3f00"); - generate_dir_table_from_blob(&dirs, blob, len); + int n = ss_dir_table_from_blob(blob, len, &dir); /* A zero-length record is well-formed: it consumes exactly the 3-byte * header and must not desynchronise the records that follow. */ - zassert_equal(dir_count(&dirs), 2); - - struct cache_entry *first = SS_LIST_GET_NEXT(&dirs, struct cache_entry, list); + zassert_equal(n, 2); + zassert_equal(ss_dir_find(dir, n, ""), 0); + zassert_equal(ss_dir_find(dir, n, "/3f00"), 1); - zassert_str_equal(first->name, ""); - - dir_free(&dirs); + SS_FREE(dir); } /* @@ -177,32 +142,27 @@ ZTEST(softsim_cache, test_flags_are_derived_but_key_keeps_all_16_bits) const uint16_t ids[] = {0x0002, 0x8010, 0x0110}; const uint8_t expected_flags[] = {0x00, 0x80, 0x01}; uint8_t blob[64]; - struct ss_list dirs; + struct ss_dir_entry *dir; size_t len = 0; - size_t i = 0; - ss_list_init(&dirs); len = put_record(blob, len, ids[0], "/a"); len = put_record(blob, len, ids[1], "/b"); len = put_record(blob, len, ids[2], "/c"); - generate_dir_table_from_blob(&dirs, blob, len); - zassert_equal(dir_count(&dirs), 3); + int n = ss_dir_table_from_blob(blob, len, &dir); - struct cache_entry *cursor; + zassert_equal(n, 3); - SS_LIST_FOR_EACH(&dirs, cursor, struct cache_entry, list) - { - zassert_equal(cursor->key, ids[i], "NVS key must keep the flag bits"); - zassert_equal(cursor->_flags, expected_flags[i], "flags are the high byte"); - i++; + for (size_t i = 0; i < 3; i++) { + zassert_equal(dir[i].key, ids[i], "NVS key must keep the flag bits"); + zassert_equal(dir[i].flags, expected_flags[i], "flags are the high byte"); } - dir_free(&dirs); + SS_FREE(dir); } /* - * Known defect: _flags is a uint8_t holding (id >> 8), so a flag macro above + * Known defect: flags is a uint8_t holding (id >> 8), so a flag macro above * 0xFF can never match. FS_COMMIT_ON_CLOSE (1<<7) is expressed in post-shift * space and works; FS_READ_ONLY (1<<8) is expressed in raw-id space and does * not, which makes the read-only guard in ss_fs.c dead code. @@ -214,147 +174,167 @@ ZTEST(softsim_cache, test_flags_are_derived_but_key_keeps_all_16_bits) ZTEST(softsim_cache, test_flag_macros_fit_the_flags_field) { zassert_true(FS_COMMIT_ON_CLOSE <= UINT8_MAX, "FS_COMMIT_ON_CLOSE cannot match"); - zassert_true(FS_READ_ONLY <= UINT8_MAX, "FS_READ_ONLY cannot match a uint8_t _flags"); + zassert_true(FS_READ_ONLY <= UINT8_MAX, "FS_READ_ONLY cannot match a uint8_t flags"); } ZTEST_EXPECT_FAIL(softsim_cache, test_flag_macros_fit_the_flags_field); -/* --- f_cache_find_by_name -------------------------------------------------- */ +/* + * Lookups compare path hashes, so two paths hashing identically would silently + * serve one file for the other. The table build must refuse that outright. + * The pair below really collides under FNV-1a (both hash to 0x38902ebd); + * found by brute force, any future hash change invalidates it loudly. + */ +ZTEST(softsim_cache, test_colliding_paths_refuse_the_table) +{ + uint8_t blob[64]; + struct ss_dir_entry *dir; + size_t len; + + len = put_record(blob, 0, 0x0002, "/3f00/07a0f5"); + len = put_record(blob, len, 0x0003, "/3f00/0aec20"); + + int n = ss_dir_table_from_blob(blob, len, &dir); + + zassert_equal(n, -1, "colliding paths must fail the whole table"); + zassert_is_null(dir, "a refused table must not be handed out"); +} + +/* --- ss_dir_find ------------------------------------------------------------- */ ZTEST(softsim_cache, test_find_by_name_hit_and_miss) { uint8_t blob[64]; - struct ss_list dirs; + struct ss_dir_entry *dir; size_t len; - ss_list_init(&dirs); len = put_record(blob, 0, 0x0002, "/3f00/2fe2"); len = put_record(blob, len, 0x0003, "/3f00/a001"); - generate_dir_table_from_blob(&dirs, blob, len); - struct cache_entry *hit = f_cache_find_by_name("/3f00/a001", &dirs); + int n = ss_dir_table_from_blob(blob, len, &dir); + int hit = ss_dir_find(dir, n, "/3f00/a001"); - zassert_not_null(hit); - zassert_equal(hit->key, 0x0003); + zassert_true(hit >= 0); + zassert_equal(dir[hit].key, 0x0003); - zassert_is_null(f_cache_find_by_name("/3f00/6f07", &dirs), "miss must return NULL"); - zassert_is_null(f_cache_find_by_name("", &dirs), "empty path must not match"); + zassert_equal(ss_dir_find(dir, n, "/3f00/6f07"), -1, "miss must return -1"); + zassert_equal(ss_dir_find(dir, n, ""), -1, "empty path must not match"); - dir_free(&dirs); + SS_FREE(dir); } -/* --- f_cache_find_buffer --------------------------------------------------- */ +/* --- ss_slot_acquire --------------------------------------------------------- */ -/* SS_MAX_ENTRIES in ss_cache.c; the cache is allowed to grow to this many - * buffered entries before anything is evicted. */ +/* SS_MAX_ENTRIES slots: the cache is allowed to grow to this many buffered + * files before anything is evicted. */ #define EXPECTED_MAX_ENTRIES 10 -static struct cache_entry pool[EXPECTED_MAX_ENTRIES + 2]; +static struct ss_dir_entry dir_pool[EXPECTED_MAX_ENTRIES + 2]; +static struct ss_cache_slot pool[EXPECTED_MAX_ENTRIES + 2]; static uint8_t pool_buf[EXPECTED_MAX_ENTRIES + 2][8]; -static void pool_reset(struct ss_list *cache, size_t buffered) +static void pool_reset(size_t buffered) { - ss_list_init(cache); + memset(dir_pool, 0, sizeof(dir_pool)); memset(pool, 0, sizeof(pool)); for (size_t i = 0; i < buffered; i++) { pool[i].buf = pool_buf[i]; pool[i]._b_size = sizeof(pool_buf[i]); - pool[i]._cache_hits = 5; - ss_list_put(cache, &pool[i].list); + pool[i].dir_idx = (uint16_t)i; + dir_pool[i].hits = 5; } } -ZTEST(softsim_cache, test_find_buffer_lets_the_cache_grow_first) +ZTEST(softsim_cache, test_acquire_lets_the_cache_grow_first) { - struct ss_list cache; - struct cache_entry want = {._l = 4}; + pool_reset(EXPECTED_MAX_ENTRIES - 1); - pool_reset(&cache, EXPECTED_MAX_ENTRIES - 1); + int idx = ss_slot_acquire(dir_pool, pool, EXPECTED_MAX_ENTRIES, 4); - zassert_is_null(f_cache_find_buffer(&want, &cache), - "below the cap nothing should be evicted"); + zassert_true(idx >= 0); + zassert_is_null(pool[idx].buf, "below the cap nothing should be evicted"); } -ZTEST(softsim_cache, test_find_buffer_prefers_clean_big_enough_least_used) +ZTEST(softsim_cache, test_acquire_prefers_clean_big_enough_least_used) { - struct ss_list cache; - struct cache_entry want = {._l = 4}; + pool_reset(EXPECTED_MAX_ENTRIES); - pool_reset(&cache, EXPECTED_MAX_ENTRIES); - - /* Make one entry the obvious victim: clean, big enough, fewest hits. */ + /* Make one slot the obvious victim: clean, big enough, fewest hits. */ pool[3]._b_dirty = 0; - pool[3]._cache_hits = 0; + dir_pool[3].hits = 0; - /* A dirty entry with even fewer hits must NOT win -- reusing it would + /* A dirty slot with even fewer hits must NOT win -- reusing it would * cost a flash write. */ pool[7]._b_dirty = 1; - pool[7]._cache_hits = 0; + dir_pool[7].hits = 0; - struct cache_entry *victim = f_cache_find_buffer(&want, &cache); + int idx = ss_slot_acquire(dir_pool, pool, EXPECTED_MAX_ENTRIES, 4); - zassert_equal_ptr(victim, &pool[3], "clean entry should win over dirty"); + zassert_equal(idx, 3, "clean slot should win over dirty"); } /* - * The middle preference: a clean entry whose buffer is too small still beats a + * The middle preference: a clean slot whose buffer is too small still beats a * dirty one, because handing it over only costs an allocation while reusing a - * dirty entry costs a flash write. Every entry in the pool is big enough for the + * dirty slot costs a flash write. Every slot in the pool is big enough for the * requests above, so without this case the first two preferences are * indistinguishable and only one of the three branches is ever taken. */ -ZTEST(softsim_cache, test_find_buffer_takes_a_clean_small_buffer_over_a_dirty_one) +ZTEST(softsim_cache, test_acquire_takes_a_clean_small_buffer_over_a_dirty_one) { - struct ss_list cache; - struct cache_entry want = {._l = sizeof(pool_buf[0]) + 8}; + const size_t want = sizeof(pool_buf[0]) + 8; - pool_reset(&cache, EXPECTED_MAX_ENTRIES); + pool_reset(EXPECTED_MAX_ENTRIES); /* Clean, fewest hits, but its buffer cannot hold the request. */ pool[2]._b_dirty = 0; - pool[2]._cache_hits = 1; + dir_pool[2].hits = 1; /* Dirty and completely unused: still loses, a write is the worse cost. */ pool[5]._b_dirty = 1; - pool[5]._cache_hits = 0; + dir_pool[5].hits = 0; /* Nobody can satisfy the size, so the best-case branch stays empty. */ for (size_t i = 0; i < EXPECTED_MAX_ENTRIES; i++) { - zassert_true(pool[i]._b_size < want._l, "entry %zu was unexpectedly big enough", i); + zassert_true(pool[i]._b_size < want, "slot %zu was unexpectedly big enough", i); } - struct cache_entry *victim = f_cache_find_buffer(&want, &cache); + int idx = ss_slot_acquire(dir_pool, pool, EXPECTED_MAX_ENTRIES, want); - zassert_equal_ptr(victim, &pool[2], "a clean undersized buffer should win over dirty"); + zassert_equal(idx, 2, "a clean undersized buffer should win over dirty"); } -ZTEST(softsim_cache, test_find_buffer_falls_back_to_dirty_when_all_dirty) +ZTEST(softsim_cache, test_acquire_falls_back_to_dirty_when_all_dirty) { - struct ss_list cache; - struct cache_entry want = {._l = 4}; - - pool_reset(&cache, EXPECTED_MAX_ENTRIES); + pool_reset(EXPECTED_MAX_ENTRIES); for (size_t i = 0; i < EXPECTED_MAX_ENTRIES; i++) { pool[i]._b_dirty = 1; } - pool[6]._cache_hits = 0; + dir_pool[6].hits = 0; - struct cache_entry *victim = f_cache_find_buffer(&want, &cache); + int idx = ss_slot_acquire(dir_pool, pool, EXPECTED_MAX_ENTRIES, 4); - zassert_equal_ptr(victim, &pool[6], "least-used dirty entry is the fallback"); + zassert_equal(idx, 6, "least-used dirty slot is the fallback"); } -/* --- ABI invariant --------------------------------------------------------- */ - /* - * SS_LIST_GET_NEXT does pointer arithmetic on a struct ss_list *, so the - * offsetof() it subtracts is scaled by sizeof(struct ss_list). That only - * happens to work because the list member sits first in struct cache_entry. - * Reorder the struct and the whole cache corrupts silently. + * The hit counter saturates at 255. The list-based predecessor compared + * against a sentinel of 100, so once every buffered file had been opened 100 + * times no victim was ever found and the cache silently grew past its + * capacity, one buffer per new file, for the life of the session. The slot + * table IS the capacity: a fully-hit cache must still yield a victim. */ -ZTEST(softsim_cache, test_list_member_is_first_in_cache_entry) +ZTEST(softsim_cache, test_acquire_still_evicts_when_every_file_is_hot) { - zassert_equal(offsetof(struct cache_entry, list), 0, - "SS_LIST_GET_NEXT relies on this being zero"); + pool_reset(EXPECTED_MAX_ENTRIES); + + for (size_t i = 0; i < EXPECTED_MAX_ENTRIES; i++) { + dir_pool[i].hits = 200; + } + dir_pool[4].hits = 150; + + int idx = ss_slot_acquire(dir_pool, pool, EXPECTED_MAX_ENTRIES, 4); + + zassert_equal(idx, 4, "a saturated cache must still evict its least-used slot"); } diff --git a/tests/fs/src/main.c b/tests/fs/src/main.c index da802893..a1f024b0 100644 --- a/tests/fs/src/main.c +++ b/tests/fs/src/main.c @@ -91,8 +91,8 @@ static const uint8_t deinit_content[] = {0xd0, 0xd1, 0xd2, 0xd3, 0xd4, 0xd5, 0xd * port_check_provisioned() compares the stored IMSI against. */ static const uint8_t unprovisioned_imsi[] = {0x08, 0x09, 0x10, 0x10, 0x00, 0x00, 0x00, 0x00, 0x10}; -/* SS_MAX_ENTRIES in ss_cache.c: the cache grows to this many buffered entries - * before f_cache_find_buffer() starts evicting. */ +/* SS_MAX_ENTRIES in ss_cache.h: the cache grows to this many buffered slots + * before ss_slot_acquire() starts evicting. */ #define CACHE_CAPACITY 10 /* One file bigger than every other, so the open that evicts cannot reuse the From d4cb3c88de8cb613246d18501492077ecef6127d Mon Sep 17 00:00:00 2001 From: Benjamin Bruun Date: Wed, 2 Sep 2026 08:24:49 +0200 Subject: [PATCH 2/2] chore(fs): log the NVS keys of colliding DIR entries Signed-off-by: Benjamin Bruun --- lib/ss_cache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/ss_cache.c b/lib/ss_cache.c index c5ea1985..51874928 100644 --- a/lib/ss_cache.c +++ b/lib/ss_cache.c @@ -86,9 +86,9 @@ int ss_dir_table_from_blob(const uint8_t *blob, size_t size, struct ss_dir_entry for (size_t i = 1; i < count; i++) { for (size_t j = 0; j < i; j++) { if (dir[i].hash == dir[j].hash) { - LOG_ERR("DIR paths %u and %u share hash 0x%08x; " - "refusing the table", - (unsigned)j, (unsigned)i, dir[i].hash); + LOG_ERR("DIR entries with NVS keys 0x%04x and 0x%04x share hash " + "0x%08x; refusing the table", + dir[j].key, dir[i].key, dir[i].hash); SS_FREE(dir); return -1; }