michaelh/zephyr
1
0
Fork 0
Code Releases Activity

nvs: replace CRC with better hash function for lookup cache

Browse source 
NVS lookup cache currently uses CRC8/16 as a hash function
to determine the cache position, which is not ideal choice.
For example, when NVS lookup cache size is 512 and 256
subsequent NVS IDs are written (that is, 0, 1.., 255), this
results in 128 cache collisions.

It is better to use a dedicated integer hash function. This
PR uses one of the 16-bit integer hash functions discovered
with https://github.com/skeeto/hash-prospector project. The
hash function was additionally tested in the context of NVS
lookup cache using simple NVS ID allocation patterns as well
as using real device NVS dump.

Also, add a test case to verify that the hash function is
not extremely bad.

Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>
This commit is contained in:
Damian Krolik 2023-08-01 17:07:20 +02:00 • committed by Carles Cufí
commit 49fb5f0a1a
2 changed files with 94 additions and 34 deletions
Download patch file Download diff file Expand all files Collapse all files

22
subsys/fs/nvs/nvs.c
View file

@ -23,21 +23,17 @@ static int nvs_ate_valid(struct nvs_fs *fs, const struct nvs_ate *entry);
static inline size_t nvs_lookup_cache_pos(uint16_t id)
{
size_t pos;
uint16_t hash;
#if CONFIG_NVS_LOOKUP_CACHE_SIZE <= (UINT8_MAX + 1)
/*
* CRC8-CCITT is used for ATE checksums and it also acts well as a hash
* function, so it can be a good choice from the code size perspective.
* However, other hash functions can be used as well if proved better
* performance.
*/
pos = crc8_ccitt(CRC8_CCITT_INITIAL_VALUE, &id, sizeof(id));
#else
pos = crc16_ccitt(0xffff, (const uint8_t *)&id, sizeof(id));
#endif
/* 16-bit integer hash function found by https://github.com/skeeto/hash-prospector. */
hash = id;
hash ^= hash >> 8;
hash *= 0x88b5U;
hash ^= hash >> 7;
hash *= 0xdb2dU;
hash ^= hash >> 9;
return pos % CONFIG_NVS_LOOKUP_CACHE_SIZE;
return hash % CONFIG_NVS_LOOKUP_CACHE_SIZE;
}
static int nvs_lookup_cache_rebuild(struct nvs_fs *fs)

106
tests/subsys/fs/nvs/src/main.c
View file

@ -20,10 +20,10 @@
#include <zephyr/ztest.h>
#include <zephyr/drivers/flash.h>
#include <zephyr/storage/flash_map.h>
#include <zephyr/stats/stats.h>
#include <zephyr/sys/crc.h>
#include <zephyr/fs/nvs.h>
#include <zephyr/stats/stats.h>
#include <zephyr/storage/flash_map.h>
#include <zephyr/sys/crc.h>
#include "nvs_priv.h"
#define TEST_NVS_FLASH_AREA storage_partition
@ -72,14 +72,6 @@ static void before(void *data)
fixture->sim_stats = stats_group_find("flash_sim_stats");
fixture->sim_thresholds = stats_group_find("flash_sim_thresholds");
/* Verify if NVS is initialized. */
if (fixture->fs.ready) {
int err;
err = nvs_clear(&fixture->fs);
zassert_true(err == 0, "nvs_clear call failure: %d", err);
}
}
static void after(void *data)
@ -93,6 +85,14 @@ static void after(void *data)
stats_reset(fixture->sim_thresholds);
}
/* Clear NVS */
if (fixture->fs.ready) {
int err;
err = nvs_clear(&fixture->fs);
zassert_true(err == 0, "nvs_clear call failure: %d", err);
}
fixture->fs.sector_count = TEST_SECTOR_COUNT;
}
@ -744,6 +744,12 @@ static size_t num_matching_cache_entries(uint32_t addr, bool compare_sector_only
return num;
}
static size_t num_occupied_cache_entries(struct nvs_fs *fs)
{
return CONFIG_NVS_LOOKUP_CACHE_SIZE -
num_matching_cache_entries(NVS_LOOKUP_CACHE_NO_ADDR, false, fs);
}
#endif
/*
@ -762,10 +768,10 @@ ZTEST_F(nvs, test_nvs_cache_init)
fixture->fs.sector_count = 3;
err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_init call failure: %d", err);
zassert_true(err == 0, "nvs_mount call failure: %d", err);
num = num_matching_cache_entries(NVS_LOOKUP_CACHE_NO_ADDR, false, &fixture->fs);
zassert_equal(num, CONFIG_NVS_LOOKUP_CACHE_SIZE, "uninitialized cache");
num = num_occupied_cache_entries(&fixture->fs);
zassert_equal(num, 0, "uninitialized cache");
/* Test cache update after nvs_write() */
@ -773,8 +779,8 @@ ZTEST_F(nvs, test_nvs_cache_init)
err = nvs_write(&fixture->fs, 1, &data, sizeof(data));
zassert_equal(err, sizeof(data), "nvs_write call failure: %d", err);
num = num_matching_cache_entries(NVS_LOOKUP_CACHE_NO_ADDR, false, &fixture->fs);
zassert_equal(num, CONFIG_NVS_LOOKUP_CACHE_SIZE - 1, "cache not updated after write");
num = num_occupied_cache_entries(&fixture->fs);
zassert_equal(num, 1, "cache not updated after write");
num = num_matching_cache_entries(ate_addr, false, &fixture->fs);
zassert_equal(num, 1, "invalid cache entry after write");
@ -783,10 +789,10 @@ ZTEST_F(nvs, test_nvs_cache_init)
memset(fixture->fs.lookup_cache, 0xAA, sizeof(fixture->fs.lookup_cache));
err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_init call failure: %d", err);
zassert_true(err == 0, "nvs_mount call failure: %d", err);
num = num_matching_cache_entries(NVS_LOOKUP_CACHE_NO_ADDR, false, &fixture->fs);
zassert_equal(num, CONFIG_NVS_LOOKUP_CACHE_SIZE - 1, "uninitialized cache after restart");
num = num_occupied_cache_entries(&fixture->fs);
zassert_equal(num, 1, "uninitialized cache after restart");
num = num_matching_cache_entries(ate_addr, false, &fixture->fs);
zassert_equal(num, 1, "invalid cache entry after restart");
@ -806,7 +812,7 @@ ZTEST_F(nvs, test_nvs_cache_collission)
fixture->fs.sector_count = 3;
err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_init call failure: %d", err);
zassert_true(err == 0, "nvs_mount call failure: %d", err);
for (id = 0; id < CONFIG_NVS_LOOKUP_CACHE_SIZE + 1; id++) {
data = id;
@ -834,7 +840,7 @@ ZTEST_F(nvs, test_nvs_cache_gc)
fixture->fs.sector_count = 3;
err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_init call failure: %d", err);
zassert_true(err == 0, "nvs_mount call failure: %d", err);
/* Fill the first sector with writes of ID 1 */
@ -869,3 +875,61 @@ ZTEST_F(nvs, test_nvs_cache_gc)
zassert_equal(num, 2, "invalid cache content after gc");
#endif
}
/*
* Test NVS lookup cache hash quality.
*/
ZTEST_F(nvs, test_nvs_cache_hash_quality)
{
#ifdef CONFIG_NVS_LOOKUP_CACHE
const size_t MIN_CACHE_OCCUPANCY = CONFIG_NVS_LOOKUP_CACHE_SIZE * 6 / 10;
int err;
size_t num;
uint16_t id;
uint16_t data;
err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_mount call failure: %d", err);
/* Write NVS IDs from 0 to CONFIG_NVS_LOOKUP_CACHE_SIZE - 1 */
for (uint16_t i = 0; i < CONFIG_NVS_LOOKUP_CACHE_SIZE; i++) {
id = i;
data = 0;
err = nvs_write(&fixture->fs, id, &data, sizeof(data));
zassert_equal(err, sizeof(data), "nvs_write call failure: %d", err);
}
/* Verify that at least 60% cache entries are occupied */
num = num_occupied_cache_entries(&fixture->fs);
TC_PRINT("Cache occupancy: %u\n", (unsigned int)num);
zassert_between_inclusive(num, MIN_CACHE_OCCUPANCY, CONFIG_NVS_LOOKUP_CACHE_SIZE,
"too low cache occupancy - poor hash quality");
err = nvs_clear(&fixture->fs);
zassert_true(err == 0, "nvs_clear call failure: %d", err);
err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_mount call failure: %d", err);
/* Write CONFIG_NVS_LOOKUP_CACHE_SIZE NVS IDs that form the following series: 0, 4, 8... */
for (uint16_t i = 0; i < CONFIG_NVS_LOOKUP_CACHE_SIZE; i++) {
id = i * 4;
data = 0;
err = nvs_write(&fixture->fs, id, &data, sizeof(data));
zassert_equal(err, sizeof(data), "nvs_write call failure: %d", err);
}
/* Verify that at least 60% cache entries are occupied */
num = num_occupied_cache_entries(&fixture->fs);
TC_PRINT("Cache occupancy: %u\n", (unsigned int)num);
zassert_between_inclusive(num, MIN_CACHE_OCCUPANCY, CONFIG_NVS_LOOKUP_CACHE_SIZE,
"too low cache occupancy - poor hash quality");
#endif
}