Merge pull request #300 from arutk/revert_cl_lock_opt

Revert "Optimize cacheline locking in ocf_engine_prepare_clines"

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
+ * Read-lock request cache lines. Must be called under cacheline concurrency
- * concurrency write lock.
+ * 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
+ * Write-lock request cache lines. Must be called under cacheline concurrency
- * concurrency write lock.
+ * 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;
+}
+
+
 /*
  *
  */

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
+ * @brief Lock OCF request for write access (Lock all cache lines in map info)
- * 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
+ * added into waiting list. When lock will be acquired @cb cllback be called
- *		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
+ * @brief Lock OCF request for read access (Lock all cache lines in map info)
- * 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.
  *
  * @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
+ * added into waiting list. When lock will be acquired @cb callback be called
- *		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
+ * @brief Unlock OCF request from write access
- * 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
  *
  * @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

src/engine/engine_common.c

@@ -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,78 +444,55 @@ 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
+		/* Request cachelines are already mapped, acquire cacheline
-		 * per-cacheline locking fast path, which would fail either if
+		 * lock */
-		 * cachelines are already locked without putting request to a
-		 * waiter list */
 		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);
-		}
 	} 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;
 }

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);
-	}
 
 	if (lock >= 0) {
 		if (OCF_LOCK_ACQUIRED == lock) {

src/engine/engine_fast.c

@@ -195,17 +195,10 @@ 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);
-	}
 
 	if (mapped) {
 		if (lock >= 0) {

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);
-	}
 
 	if (use_cache) {
 		/*

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_unlock_rd(req); /*- END Metadata READ access----------------*/
-		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);
-	}
 
 	if (lock >= 0) {
 		if (lock == OCF_LOCK_ACQUIRED) {

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_unlock_rd(req); /*- END Metadata RD access -----------------*/
-		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);
-	}
 
 	if (lock >= 0) {
 		if (lock != OCF_LOCK_ACQUIRED) {