From 736fb2efc00dd4277eb52f20e604da841846be56 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Tue, 16 Mar 2021 19:59:09 -0500 Subject: [PATCH 1/4] Call LRU set_hot() immediately after cache insert This assures that cacheline with LOOKUP_INSERTED status is always present on the LRU list. This fixes an ENV_BUG() caused by an attempt to remove a cacheline from LRU list which was not there. This happened when cacheline was mapped from freelist (LOOKUP_INSERTED) but the entire request mapping failed and generic cleanup routines attempted to invalidate cacheline, including removing it from the LRU list. As engine_set_hot() is called after successfull mapping, the inserted cacheline was not yet present on the LRU list. Signed-off-by: Adam Rutkowski --- src/engine/engine_common.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/engine/engine_common.c b/src/engine/engine_common.c index c5e0c08..d20230a 100644 --- a/src/engine/engine_common.c +++ b/src/engine/engine_common.c @@ -203,11 +203,16 @@ static void ocf_engine_set_hot(struct ocf_request *req) uint8_t status; unsigned i; + if (req->info.hit_no == 0 && req->info.invalid_no == 0) { + /* no previously mapped clines */ + return; + } + for (i = 0; i < req->core_line_count; i++) { entry = &(req->map[i]); status = entry->status; - if (status == LOOKUP_HIT || status == LOOKUP_INSERTED) { + if (status == LOOKUP_HIT) { /* Update eviction (LRU) */ ocf_eviction_set_hot_cache_line(cache, entry->coll_idx); } @@ -347,6 +352,7 @@ static void ocf_engine_map_cache_line(struct ocf_request *req, /* Update LRU:: Move this node to head of lru list. */ ocf_eviction_init_cache_line(cache, cache_line); + ocf_eviction_set_hot_cache_line(cache, cache_line); } static void ocf_engine_map_hndl_error(struct ocf_cache *cache, From e5fa15bdb2808be2ee105fa8d5bb0769cc7ecee9 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 17 Mar 2021 11:21:03 -0500 Subject: [PATCH 2/4] Remove early return from engine_map() in case of hit At this point cacheline status in request map is stale, as lookup was performed before upgrading hash bucket lock. If indeed all cachelines are mapped, this will be determined in the main loop of engine_map(). Signed-off-by: Adam Rutkowski --- src/engine/engine_common.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/engine/engine_common.c b/src/engine/engine_common.c index d20230a..3310800 100644 --- a/src/engine/engine_common.c +++ b/src/engine/engine_common.c @@ -402,9 +402,6 @@ static void ocf_engine_map(struct ocf_request *req) uint64_t core_line; ocf_core_id_t core_id = ocf_core_get_id(req->core); - if (!ocf_engine_unmapped_count(req)) - return; - if (ocf_engine_unmapped_count(req) > ocf_freelist_num_free(cache->freelist)) { ocf_req_set_mapping_error(req); From 98124aa13da4e4e7e7f77912d6fbe249ce749aaa Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 17 Mar 2021 11:23:24 -0500 Subject: [PATCH 3/4] Add missing lookup in engine_map() Early return from engine_map() in case of insufficient free cachelines on the freelist is opportunistic, as both request map info and freelist count are not accurate. Map info is stale as it is to be refreshed in engine_map() after hash bucket lock had been upgraded. Freelist count on other hand is subject to change asynchronously. The implementation assumption however is that after engine_map() request is fully traversed (engine_map() is equivalent to engine_lookup() followed by an attempt to map missing cachelines). So in case of early return we must take care of repeating the lookup. Signed-off-by: Adam Rutkowski --- src/engine/engine_common.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/engine/engine_common.c b/src/engine/engine_common.c index 3310800..ca3d1f0 100644 --- a/src/engine/engine_common.c +++ b/src/engine/engine_common.c @@ -404,6 +404,7 @@ static void ocf_engine_map(struct ocf_request *req) if (ocf_engine_unmapped_count(req) > ocf_freelist_num_free(cache->freelist)) { + ocf_engine_lookup(req); ocf_req_set_mapping_error(req); return; } From c565c5c3f57f37f5202e809044b18f30de07be14 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 17 Mar 2021 11:28:02 -0500 Subject: [PATCH 4/4] Add comments warning about stale request map info Signed-off-by: Adam Rutkowski --- src/engine/engine_common.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/engine/engine_common.c b/src/engine/engine_common.c index ca3d1f0..9b7b885 100644 --- a/src/engine/engine_common.c +++ b/src/engine/engine_common.c @@ -402,6 +402,8 @@ static void ocf_engine_map(struct ocf_request *req) uint64_t core_line; ocf_core_id_t core_id = ocf_core_get_id(req->core); + /* NOTE: request not refreshed after upgrading hash bucket lock. + * ocf_engine_unmapped_count() is potentially not accurate. */ if (ocf_engine_unmapped_count(req) > ocf_freelist_num_free(cache->freelist)) { ocf_engine_lookup(req); @@ -555,6 +557,10 @@ static inline int ocf_prepare_clines_miss(struct ocf_request *req) return lock_status; } + /* NOTE: ocf_part_has_space() below uses potentially stale request + * statistics (collected before hash bucket lock had been upgraded). + * It is ok since this check is opportunistic, as partition occupancy + * is also subject to change. */ if (!ocf_part_has_space(req)) { ocf_engine_lookup(req); return ocf_prepare_clines_evict(req);