From 09b68297b2e632fcdfb906ffdeb23c044242b2d1 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 30 Sep 2019 23:15:35 -0400 Subject: [PATCH] Revert "Optimize cacheline locking in ocf_engine_prepare_clines" This change introduced a race condition. In some code paths after cacheline trylock failed, hash bucket lock needed bo be upgraded in order to obtain asynchronous lock. During hash bucket lock upgrade, hash read locks were released followed by obtaining hash write locks. After read locks were released, concurrent thread could obtain hash bucket locks and modify cacheline state. The thread upgrading hash bucket lock would need to repeat traversation in order to safely continue. This reverts commit 30f22d4f471af525671e19595023534f29e287e2. Signed-off-by: Adam Rutkowski --- src/concurrency/ocf_cache_line_concurrency.c | 60 +++++++++++-- src/concurrency/ocf_cache_line_concurrency.h | 46 ++-------- src/engine/engine_common.c | 88 ++++++-------------- src/engine/engine_discard.c | 10 +-- src/engine/engine_fast.c | 13 +-- src/engine/engine_pt.c | 15 ++-- src/engine/engine_wi.c | 10 +-- src/engine/engine_wo.c | 10 +-- 8 files changed, 101 insertions(+), 151 deletions(-) diff --git a/src/concurrency/ocf_cache_line_concurrency.c b/src/concurrency/ocf_cache_line_concurrency.c index 3ed0b54..de2f20e 100644 --- a/src/concurrency/ocf_cache_line_concurrency.c +++ b/src/concurrency/ocf_cache_line_concurrency.c @@ -50,6 +50,7 @@ struct __waiters_list { }; struct ocf_cache_line_concurrency { + env_rwlock lock; env_atomic *access; env_atomic waiting; size_t access_limit; @@ -115,6 +116,8 @@ int ocf_cache_line_concurrency_init(struct ocf_cache *cache) goto spinlock_err; } + env_rwlock_init(&c->lock); + return 0; spinlock_err: @@ -144,6 +147,8 @@ void ocf_cache_line_concurrency_deinit(struct ocf_cache *cache) concurrency = cache->device->concurrency.cache_line; + env_rwlock_destroy(&concurrency->lock); + for (i = 0; i < _WAITERS_LIST_ENTRIES; i++) env_spinlock_destroy(&concurrency->waiters_lsts[i].lock); @@ -695,7 +700,7 @@ static inline void __remove_line_from_waiters_list(struct ocf_cache_line_concurr /* Try to read-lock request without adding waiters. Function should be called * under read lock, multiple threads may attempt to acquire the lock * concurrently. */ -int ocf_req_trylock_rd(struct ocf_request *req) +static int _ocf_req_trylock_rd(struct ocf_request *req) { int32_t i; struct ocf_cache_line_concurrency *c = req->cache->device->concurrency. @@ -746,10 +751,10 @@ int ocf_req_trylock_rd(struct ocf_request *req) } /* - * Asynchronously read-lock request cache lines. Must be called under cacheline - * concurrency write lock. + * Read-lock request cache lines. Must be called under cacheline concurrency + * write lock. */ -int ocf_req_async_lock_rd(struct ocf_request *req, ocf_req_async_lock_cb cb) +static int _ocf_req_lock_rd(struct ocf_request *req, ocf_req_async_lock_cb cb) { int32_t i; struct ocf_cache_line_concurrency *c = req->cache->device->concurrency. @@ -802,10 +807,29 @@ err: } +int ocf_req_async_lock_rd(struct ocf_request *req, ocf_req_async_lock_cb cb) +{ + struct ocf_cache_line_concurrency *c = + req->cache->device->concurrency.cache_line; + int lock; + + env_rwlock_read_lock(&c->lock); + lock = _ocf_req_trylock_rd(req); + env_rwlock_read_unlock(&c->lock); + + if (lock != OCF_LOCK_ACQUIRED) { + env_rwlock_write_lock(&c->lock); + lock = _ocf_req_lock_rd(req, cb); + env_rwlock_write_unlock(&c->lock); + } + + return lock; +} + /* Try to write-lock request without adding waiters. Function should be called * under read lock, multiple threads may attempt to acquire the lock * concurrently. */ -int ocf_req_trylock_wr(struct ocf_request *req) +static int _ocf_req_trylock_wr(struct ocf_request *req) { int32_t i; struct ocf_cache_line_concurrency *c = req->cache->device->concurrency. @@ -854,10 +878,10 @@ int ocf_req_trylock_wr(struct ocf_request *req) } /* - * Asynchronously write-lock request cache lines. Must be called under cacheline - * concurrency write lock. + * Write-lock request cache lines. Must be called under cacheline concurrency + * write lock. */ -int ocf_req_async_lock_wr(struct ocf_request *req, ocf_req_async_lock_cb cb) +static int _ocf_req_lock_wr(struct ocf_request *req, ocf_req_async_lock_cb cb) { int32_t i; struct ocf_cache_line_concurrency *c = req->cache->device->concurrency. @@ -910,6 +934,26 @@ err: return ret; } +int ocf_req_async_lock_wr(struct ocf_request *req, ocf_req_async_lock_cb cb) +{ + struct ocf_cache_line_concurrency *c = + req->cache->device->concurrency.cache_line; + int lock; + + env_rwlock_read_lock(&c->lock); + lock = _ocf_req_trylock_wr(req); + env_rwlock_read_unlock(&c->lock); + + if (lock != OCF_LOCK_ACQUIRED) { + env_rwlock_write_lock(&c->lock); + lock = _ocf_req_lock_wr(req, cb); + env_rwlock_write_unlock(&c->lock); + } + + return lock; +} + + /* * */ diff --git a/src/concurrency/ocf_cache_line_concurrency.h b/src/concurrency/ocf_cache_line_concurrency.h index 6c84ec4..fc71d1c 100644 --- a/src/concurrency/ocf_cache_line_concurrency.h +++ b/src/concurrency/ocf_cache_line_concurrency.h @@ -54,8 +54,7 @@ size_t ocf_cache_line_concurrency_size_of(struct ocf_cache *cache); typedef void (*ocf_req_async_lock_cb)(struct ocf_request *req); /** - * @brief Lock OCF request for write access asynchronously. Attempts to lock all - * cache lines in map info. + * @brief Lock OCF request for write access (Lock all cache lines in map info) * * @param req - OCF request * @param cb - async lock acquisition callback @@ -64,27 +63,12 @@ typedef void (*ocf_req_async_lock_cb)(struct ocf_request *req); * error * @retval OCF_LOCK_ACQUIRED - OCF request has been locked and can be processed * @retval OCF_LOCK_NOT_ACQUIRED - OCF request lock not acquired, request was - * added into waiting list. When lock is acquired, @cb will be - * called. + * added into waiting list. When lock will be acquired @cb cllback be called */ int ocf_req_async_lock_wr(struct ocf_request *req, ocf_req_async_lock_cb cb); /** - * @brief Try to lock OCF request for write access. Serves the same purpose as - * ocf_req_async_lock_wr, except that this function fails if lock is already - * held by someone else. - * - * @param req - OCF request - * - * @returns lock acquisition status - * @retval OCF_LOCK_ACQUIRED - OCF request has been locked and can be processed - * @retval OCF_LOCK_NOT_ACQUIRED - OCF request lock not acquired - */ -int ocf_req_trylock_wr(struct ocf_request *req); - -/** - * @brief Lock OCF request for read access asynchronously. Attempts to lock all - * cache lines in map info. + * @brief Lock OCF request for read access (Lock all cache lines in map info) * * @param req - OCF request * @param cb - async lock acquisition callback @@ -93,40 +77,26 @@ int ocf_req_trylock_wr(struct ocf_request *req); * error * @retval OCF_LOCK_ACQUIRED - OCF request has been locked and can be processed * @retval OCF_LOCK_NOT_ACQUIRED - OCF request lock not acquired, request was - * added into waiting list. When lock is acquired, @cb will be - * called. + * added into waiting list. When lock will be acquired @cb callback be called */ int ocf_req_async_lock_rd(struct ocf_request *req, ocf_req_async_lock_cb cb); /** - * @brief Try to lock OCF request forread access. Serves the same purpose as - * ocf_req_async_lock_rd, except that this function fails if lock is already - * held by someone else. - * - * @param req - OCF request - * - * @returns lock acquisition status - * @retval OCF_LOCK_ACQUIRED - OCF request has been locked and can be processed - * @retval OCF_LOCK_NOT_ACQUIRED - OCF request lock not acquired - */ -int ocf_req_trylock_rd(struct ocf_request *req); - -/** - * @brief Unlock OCF request from WRITE access + * @brief Unlock OCF request from write access * * @param req - OCF request */ void ocf_req_unlock_wr(struct ocf_request *req); /** - * @brief Unlock OCF request from READ access + * @brief Unlock OCF request from read access * * @param req - OCF request */ void ocf_req_unlock_rd(struct ocf_request *req); /** - * @brief Unlock OCF request from READ or WRITE access + * @brief Unlock OCF request from read or write access * * @param req - OCF request */ @@ -163,7 +133,7 @@ bool ocf_cache_line_are_waiters(struct ocf_cache *cache, ocf_cache_line_t line); /** - * @brief un_lock request map info entry from from WRITE or READ access. + * @brief un_lock request map info entry from from write or read access. * * @param cache - OCF cache instance * @param req - OCF request diff --git a/src/engine/engine_common.c b/src/engine/engine_common.c index 55e6624..cd1c978 100644 --- a/src/engine/engine_common.c +++ b/src/engine/engine_common.c @@ -405,7 +405,7 @@ static void _ocf_engine_clean_end(void *private_data, int error) } } -static int ocf_engine_evict(struct ocf_request *req) +static int ocf_engine_evict(struct ocf_request *req) { if (!ocf_engine_unmapped_count(req)) return 0; @@ -427,19 +427,6 @@ static int lock_clines(struct ocf_request *req, enum ocf_engine_lock_type lock, } } -static int trylock_clines(struct ocf_request *req, - enum ocf_engine_lock_type lock) -{ - switch (lock) { - case ocf_engine_lock_write: - return ocf_req_trylock_wr(req); - case ocf_engine_lock_read: - return ocf_req_trylock_rd(req); - default: - return OCF_LOCK_ACQUIRED; - } -} - int ocf_engine_prepare_clines(struct ocf_request *req, const struct ocf_engine_callbacks *engine_cbs) { @@ -457,79 +444,56 @@ int ocf_engine_prepare_clines(struct ocf_request *req, * not change during traversation */ ocf_req_hash_lock_rd(req); - /* Traverse request to cache if there is hit */ + /* Traverse to check if request is mapped fully */ ocf_engine_traverse(req); + mapped = ocf_engine_is_mapped(req); - if (mapped) { - /* We are holding hash buckets read lock, so we can attempt - * per-cacheline locking fast path, which would fail either if - * cachelines are already locked without putting request to a - * waiter list */ + /* Request cachelines are already mapped, acquire cacheline + * lock */ lock_type = engine_cbs->get_lock_type(req); - lock = trylock_clines(req, lock_type); - - if (lock == OCF_LOCK_ACQUIRED) { - /* Cachelines are mapped and locked, we don't need the - * hash bucket lock any more */ - ocf_req_hash_unlock_rd(req); - } else { - /* Failed to acquire cachelines lock in fast path, - * acquire hash-buckets write lock and attempt the lock - * again, allowing slow path and async assignment of - * the lock. */ - ocf_req_hash_lock_upgrade(req); - lock = lock_clines(req, lock_type, engine_cbs->resume); - ocf_req_hash_unlock_wr(req); - } + lock = lock_clines(req, lock_type, engine_cbs->resume); } else { /* check if request should promote cachelines */ promote = ocf_promotion_req_should_promote( req->cache->promotion_policy, req); - if (!promote) { + if (!promote) req->info.mapping_error = 1; - ocf_req_hash_unlock_rd(req); - } } - if (!mapped && promote) { + if (mapped || !promote) { + /* Will not attempt mapping - release hash bucket lock */ + ocf_req_hash_unlock_rd(req); + } else { /* Need to map (potentially evict) cachelines. Mapping must be * performed holding (at least) hash-bucket write lock */ ocf_req_hash_lock_upgrade(req); - ocf_engine_map(req); - if (!req->info.mapping_error) { - /* Lock cachelines, potentially putting the request on - * waiter list */ - lock_type = engine_cbs->get_lock_type(req); - lock = trylock_clines(req, lock_type); - if (lock != OCF_LOCK_ACQUIRED) { - lock = lock_clines(req, lock_type, - engine_cbs->resume); - } - } - - /* At this point the request is mapped or we need to evict, - * which is done under global metadata lock */ ocf_req_hash_unlock_wr(req); if (req->info.mapping_error) { - /* Not mapped - evict cachelines */ + /* Not mapped - evict cachelines under global exclusive + * lock*/ ocf_metadata_start_exclusive_access(metadata_lock); + + /* Now there is exclusive access for metadata. May + * traverse once again and evict cachelines if needed. + */ if (ocf_engine_evict(req) == LOOKUP_MAPPED) ocf_engine_map(req); - if (!req->info.mapping_error) { - lock_type = engine_cbs->get_lock_type(req); - lock = trylock_clines(req, lock_type); - if (lock != OCF_LOCK_ACQUIRED) { - lock = lock_clines(req, lock_type, - engine_cbs->resume); - } - } + ocf_metadata_end_exclusive_access(metadata_lock); } + + if (!req->info.mapping_error) { + /* Request mapped successfully - acquire cacheline + * lock */ + lock_type = engine_cbs->get_lock_type(req); + lock = lock_clines(req, lock_type, engine_cbs->resume); + } } + return lock; } diff --git a/src/engine/engine_discard.c b/src/engine/engine_discard.c index e8cca8c..81c5112 100644 --- a/src/engine/engine_discard.c +++ b/src/engine/engine_discard.c @@ -235,18 +235,12 @@ static int _ocf_discard_step(struct ocf_request *req) if (ocf_engine_mapped_count(req)) { /* Some cache line are mapped, lock request for WRITE access */ - lock = ocf_req_trylock_wr(req); + lock = ocf_req_async_lock_wr(req, _ocf_discard_on_resume); } else { lock = OCF_LOCK_ACQUIRED; } - if (lock != OCF_LOCK_ACQUIRED) { - ocf_req_hash_lock_upgrade(req); - lock = ocf_req_async_lock_wr(req, _ocf_discard_on_resume); - ocf_req_hash_unlock_wr(req); - } else { - ocf_req_hash_unlock_rd(req); - } + ocf_req_hash_unlock_rd(req); if (lock >= 0) { if (OCF_LOCK_ACQUIRED == lock) { diff --git a/src/engine/engine_fast.c b/src/engine/engine_fast.c index 11c3b7f..8cfe96c 100644 --- a/src/engine/engine_fast.c +++ b/src/engine/engine_fast.c @@ -195,18 +195,11 @@ int ocf_write_fast(struct ocf_request *req) mapped = ocf_engine_is_mapped(req); if (mapped) { ocf_io_start(&req->ioi.io); - lock = ocf_req_trylock_wr(req); - if (lock != OCF_LOCK_ACQUIRED) { - ocf_req_hash_lock_upgrade(req); - lock = ocf_req_async_lock_wr(req, ocf_engine_on_resume); - ocf_req_hash_unlock_wr(req); - } else { - ocf_req_hash_unlock_rd(req); - } - } else { - ocf_req_hash_unlock_rd(req); + lock = ocf_req_async_lock_wr(req, ocf_engine_on_resume); } + ocf_req_hash_unlock_rd(req); + if (mapped) { if (lock >= 0) { OCF_DEBUG_RQ(req, "Fast path success"); diff --git a/src/engine/engine_pt.c b/src/engine/engine_pt.c index 8aabeee..799f7b0 100644 --- a/src/engine/engine_pt.c +++ b/src/engine/engine_pt.c @@ -102,7 +102,7 @@ static const struct ocf_io_if _io_if_pt_resume = { int ocf_read_pt(struct ocf_request *req) { bool use_cache = false; - int lock = OCF_LOCK_ACQUIRED; + int lock = OCF_LOCK_NOT_ACQUIRED; OCF_DEBUG_TRACE(req->cache); @@ -127,17 +127,14 @@ int ocf_read_pt(struct ocf_request *req) /* There are mapped cache line, * lock request for READ access */ - lock = ocf_req_trylock_rd(req); + lock = ocf_req_async_lock_rd(req, ocf_engine_on_resume); + } else { + /* No mapped cache lines, no need to get lock */ + lock = OCF_LOCK_ACQUIRED; } } - if (lock != OCF_LOCK_ACQUIRED) { - ocf_req_hash_lock_upgrade(req); - lock = ocf_req_async_lock_rd(req, ocf_engine_on_resume); - ocf_req_hash_unlock_wr(req); - } else { - ocf_req_hash_unlock_rd(req); - } + ocf_req_hash_unlock_rd(req); if (use_cache) { /* diff --git a/src/engine/engine_wi.c b/src/engine/engine_wi.c index 58b7f83..ff947e4 100644 --- a/src/engine/engine_wi.c +++ b/src/engine/engine_wi.c @@ -154,18 +154,12 @@ int ocf_write_wi(struct ocf_request *req) if (ocf_engine_mapped_count(req)) { /* Some cache line are mapped, lock request for WRITE access */ - lock = ocf_req_trylock_wr(req); + lock = ocf_req_async_lock_wr(req, _ocf_write_wi_on_resume); } else { lock = OCF_LOCK_ACQUIRED; } - if (lock != OCF_LOCK_ACQUIRED) { - ocf_req_hash_lock_upgrade(req); - lock = ocf_req_async_lock_wr(req, _ocf_write_wi_on_resume); - ocf_req_hash_unlock_wr(req); - } else { - ocf_req_hash_unlock_rd(req); - } + ocf_req_hash_unlock_rd(req); /*- END Metadata READ access----------------*/ if (lock >= 0) { if (lock == OCF_LOCK_ACQUIRED) { diff --git a/src/engine/engine_wo.c b/src/engine/engine_wo.c index 289d38b..193198d 100644 --- a/src/engine/engine_wo.c +++ b/src/engine/engine_wo.c @@ -223,16 +223,10 @@ int ocf_read_wo(struct ocf_request *req) /* There are mapped cache lines, * lock request for READ access */ - lock = ocf_req_trylock_rd(req); + lock = ocf_req_async_lock_rd(req, ocf_engine_on_resume); } - if (lock != OCF_LOCK_ACQUIRED) { - ocf_req_hash_lock_upgrade(req); - lock = ocf_req_async_lock_rd(req, ocf_engine_on_resume); - ocf_req_hash_unlock_wr(req); - } else { - ocf_req_hash_unlock_rd(req); - } + ocf_req_hash_unlock_rd(req); /*- END Metadata RD access -----------------*/ if (lock >= 0) { if (lock != OCF_LOCK_ACQUIRED) {