github/ocf
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Revert "Optimize cacheline locking in ocf_engine_prepare_clines"

Browse Source 
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 30f22d4f47.

Signed-off-by: Adam Rutkowski <adam.j.rutkowski@intel.com>
This commit is contained in:
Adam Rutkowski 2019-09-30 23:15:35 -04:00
parent 944d70288e
commit 09b68297b2
8 changed files with 101 additions and 151 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
src/concurrency/ocf_cache_line_concurrency.c
View File

@ -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;
}
/*
*
*/

46
src/concurrency/ocf_cache_line_concurrency.h
View File

@ -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

88
src/engine/engine_common.c
View File

@ -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;
}

10
src/engine/engine_discard.c
View File

@ -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) {

13
src/engine/engine_fast.c
View File

@ -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");

15
src/engine/engine_pt.c
View File

@ -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) {
/*

10
src/engine/engine_wi.c
View File

@ -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) {

10
src/engine/engine_wo.c
View File

@ -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) {