From 7184f7787c29fda4270390eb06c8e805f4047347 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Fri, 19 Jul 2019 12:58:07 -0400 Subject: [PATCH 1/3] Cleanup map_info struct 1. Rename hash_key -> hash 2. Better comments for hash and coll_idx Signed-off-by: Adam Rutkowski --- src/engine/engine_common.c | 10 +++++----- src/ocf_request.h | 8 +++++--- src/utils/utils_cleaner.c | 10 +++++----- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/engine/engine_common.c b/src/engine/engine_common.c index 2bd6544..328aff6 100644 --- a/src/engine/engine_common.c +++ b/src/engine/engine_common.c @@ -37,19 +37,19 @@ void ocf_engine_lookup_map_entry(struct ocf_cache *cache, uint64_t core_line) { ocf_cache_line_t line; - ocf_cache_line_t hash_key; + ocf_cache_line_t hash; - hash_key = ocf_metadata_hash_func(cache, core_line, core_id); + hash = ocf_metadata_hash_func(cache, core_line, core_id); /* Initially assume that we have cache miss. * Hash points to proper bucket. */ - entry->hash_key = hash_key; + entry->hash = hash; entry->status = LOOKUP_MISS; entry->coll_idx = cache->device->collision_table_entries; entry->core_line = core_line; - line = ocf_metadata_get_hash(cache, hash_key); + line = ocf_metadata_get_hash(cache, hash); while (line != cache->device->collision_table_entries) { ocf_core_id_t curr_core_id; @@ -340,7 +340,7 @@ void ocf_engine_map(struct ocf_request *req) if (entry->status != LOOKUP_HIT) { ocf_engine_map_cache_line(req, entry->core_line, - entry->hash_key, &entry->coll_idx); + entry->hash, &entry->coll_idx); if (req->info.eviction_error) { /* diff --git a/src/ocf_request.h b/src/ocf_request.h index 44590b2..3847e35 100644 --- a/src/ocf_request.h +++ b/src/ocf_request.h @@ -49,9 +49,11 @@ struct ocf_req_info { }; struct ocf_map_info { - /* If HIT -> pointer to hash_key and coll_idx */ - unsigned int hash_key; - unsigned int coll_idx; + ocf_cache_line_t hash; + /*!< target LBA & core id hash */ + + ocf_cache_line_t coll_idx; + /*!< Index in collision table (in case of hit) */ uint64_t core_line; diff --git a/src/utils/utils_cleaner.c b/src/utils/utils_cleaner.c index 8c20d02..41c6b60 100644 --- a/src/utils/utils_cleaner.c +++ b/src/utils/utils_cleaner.c @@ -493,7 +493,7 @@ static void _ocf_cleaner_core_io_for_dirty_range(struct ocf_request *req, addr = (ocf_line_size(cache) * iter->core_line) + SECTORS_TO_BYTES(begin); - offset = (ocf_line_size(cache) * iter->hash_key) + offset = (ocf_line_size(cache) * iter->hash) + SECTORS_TO_BYTES(begin); ocf_io_configure(io, addr, SECTORS_TO_BYTES(end - begin), OCF_WRITE, @@ -676,7 +676,7 @@ static int _ocf_cleaner_fire_cache(struct ocf_request *req) addr *= ocf_line_size(cache); addr += cache->device->metadata_offset; - offset = ocf_line_size(cache) * iter->hash_key; + offset = ocf_line_size(cache) * iter->hash; part_id = ocf_metadata_get_partition_id(cache, iter->coll_idx); @@ -776,7 +776,7 @@ static int _ocf_cleaner_do_fire(struct ocf_request *req, uint32_t i_out, req->map[i_out].core_id = OCF_CORE_MAX; req->map[i_out].core_line = ULLONG_MAX; req->map[i_out].status = LOOKUP_MISS; - req->map[i_out].hash_key = i_out; + req->map[i_out].hash = i_out; } if (do_sort) { @@ -784,7 +784,7 @@ static int _ocf_cleaner_do_fire(struct ocf_request *req, uint32_t i_out, env_sort(req->map, req->core_line_count, sizeof(req->map[0]), _ocf_cleaner_cmp_private, NULL); for (i = 0; i < req->core_line_count; i++) - req->map[i].hash_key = i; + req->map[i].hash = i; } /* issue actual request */ @@ -919,7 +919,7 @@ void ocf_cleaner_fire(struct ocf_cache *cache, req->map[i_out].core_line = core_sector; req->map[i_out].coll_idx = cache_line; req->map[i_out].status = LOOKUP_HIT; - req->map[i_out].hash_key = i_out; + req->map[i_out].hash = i_out; i_out++; if (max == i_out) { From 3dd4263bc8c005e08f94ab17641bf2e91e93f23b Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Fri, 19 Jul 2019 17:15:54 -0400 Subject: [PATCH 2/3] Change hash function Modify ocf_metadata_hash_func to return consecutive (modulo @hash_table_entries) values for consecutive @core_line_num. This way it is trivial to sort all core lines within a single request according to their hash value. This kind of sorting will be required to assure that future hash bucket metadata locks are always acquired in fixed order, eliminating the risk of dead locks. This change is part of fine granularity metadata lock implementation. Signed-off-by: Adam Rutkowski --- src/metadata/metadata_hash.c | 3 +-- src/metadata/metadata_misc.h | 20 +++++++++----------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/metadata/metadata_hash.c b/src/metadata/metadata_hash.c index 0788ee1..7c0d130 100644 --- a/src/metadata/metadata_hash.c +++ b/src/metadata/metadata_hash.c @@ -83,8 +83,7 @@ static ocf_cache_line_t ocf_metadata_hash_get_entries( return cache_lines; case metadata_segment_hash: - return OCF_DIV_ROUND_UP(cache_lines / 4, OCF_HASH_PRIME) * - OCF_HASH_PRIME - 1; + return OCF_DIV_ROUND_UP(cache_lines, 4); case metadata_segment_sb_config: return OCF_DIV_ROUND_UP(sizeof(struct ocf_superblock_config), diff --git a/src/metadata/metadata_misc.h b/src/metadata/metadata_misc.h index 74ec801..51cdad7 100644 --- a/src/metadata/metadata_misc.h +++ b/src/metadata/metadata_misc.h @@ -6,21 +6,19 @@ #ifndef __METADATA_MISC_H__ #define __METADATA_MISC_H__ -/* - * Hash function needs number that has no common factors with both number - * of cores and number of entries in metadata hash container (hash lists). - * This can be easily achived by picking prime which is bigger than maximum - * number of cores and ensuring that count of hash table entries is not - * divisible by this number. Let's choose 4099, which is smallest prime - * greater than OCF_CORE_MAX (which is 4096). +/* Hash function intentionally returns consecutive (modulo @hash_table_entries) + * values for consecutive @core_line_num. This way it is trivial to sort all + * core lines within a single request in ascending hash value order. This kind + * of sorting is required to assure that (future) hash bucket metadata locks are + * always acquired in fixed order, eliminating the risk of dead locks. */ -#define OCF_HASH_PRIME 4099 - static inline ocf_cache_line_t ocf_metadata_hash_func(ocf_cache_t cache, uint64_t core_line_num, ocf_core_id_t core_id) { - return (ocf_cache_line_t) ((core_line_num * OCF_HASH_PRIME + core_id) % - cache->device->hash_table_entries); + const unsigned int entries = cache->device->hash_table_entries; + + return (ocf_cache_line_t) ((core_line_num + (core_id * (entries / 32))) + % entries); } void ocf_metadata_sparse_cache_line(struct ocf_cache *cache, From 9b8815935a23a81c440b1d892469b772184e7250 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 22 Jul 2019 14:04:56 -0400 Subject: [PATCH 3/3] Unit test for hashing function Signed-off-by: Adam Rutkowski --- .../metadata_collision.c/metadata_collision.c | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 tests/unit/tests/metadata/metadata_collision.c/metadata_collision.c diff --git a/tests/unit/tests/metadata/metadata_collision.c/metadata_collision.c b/tests/unit/tests/metadata/metadata_collision.c/metadata_collision.c new file mode 100644 index 0000000..a07dce4 --- /dev/null +++ b/tests/unit/tests/metadata/metadata_collision.c/metadata_collision.c @@ -0,0 +1,65 @@ +/* + * src/metadata/metadata_collision.c + * ocf_metadata_hash_func + * + * ocf_metadata_get_hash + * + */ + +#undef static + +#undef inline + + +#include +#include +#include +#include +#include "print_desc.h" + +#include "ocf/ocf.h" +#include "metadata.h" +#include "../utils/utils_cache_line.h" + +#include "metadata/metadata_collision.c/metadata_collision_generated_warps.c" + +static void metadata_hash_func_test01(void **state) +{ + struct ocf_cache cache; + bool wrap = false; + ocf_cache_line_t i; + ocf_cache_line_t hash_cur, hash_next; + unsigned c; + ocf_core_id_t core_ids[] = {0, 1, 2, 100, OCF_CORE_MAX}; + ocf_core_id_t core_id; + + print_test_description("Verify that hash function increments by 1 and generates" + "collision after 'hash_table_entries' successive core lines"); + + cache.device = malloc(sizeof(*cache.device)); + cache.device->hash_table_entries = 10; + + for (c = 0; c < sizeof(core_ids) / sizeof(core_ids[0]); c++) { + core_id = core_ids[c]; + for (i = 0; i < cache.device->hash_table_entries + 1; i++) { + hash_cur = ocf_metadata_hash_func(&cache, i, core_id); + hash_next = ocf_metadata_hash_func(&cache, i + 1, core_id); + /* make sure hash value is within expected range */ + assert(hash_cur < cache.device->hash_table_entries); + assert(hash_next < cache.device->hash_table_entries); + /* hash should either increment by 1 or overflow to 0 */ + assert(hash_next == (hash_cur + 1) % cache.device->hash_table_entries); + } + } +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(metadata_hash_func_test01) + }; + + print_message("Unit test of src/metadata/metadata_collision.c"); + + return cmocka_run_group_tests(tests, NULL, NULL); +}