From d797c7330e28bdeee74441026bb958dd47059934 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Tue, 15 Oct 2024 12:55:18 +0200 Subject: [PATCH 1/4] Fix logic of lock exchanging variable Signed-off-by: Michal Mielewczyk --- src/utils/utils_alock.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/utils/utils_alock.c b/src/utils/utils_alock.c index dbf8ea6e..fa728fed 100644 --- a/src/utils/utils_alock.c +++ b/src/utils/utils_alock.c @@ -1,6 +1,6 @@ /* * Copyright(c) 2012-2022 Intel Corporation - * Copyright(c) 2024 Huawei Technologies + * Copyright(c) 2024-2025 Huawei Technologies * SPDX-License-Identifier: BSD-3-Clause */ @@ -499,7 +499,7 @@ static inline void ocf_alock_unlock_one_rd_common(struct ocf_alock *alock, const ocf_cache_line_t entry) { bool locked = false; - bool exchanged = true; + bool exchanged = false; uint32_t idx = _WAITERS_LIST_ITEM(entry); struct ocf_alock_waiters_list *lst = &alock->waiters_lsts[idx]; @@ -521,7 +521,7 @@ static inline void ocf_alock_unlock_one_rd_common(struct ocf_alock *alock, if (entry != waiter->entry) continue; - if (exchanged) { + if (!exchanged) { if (waiter->rw == OCF_WRITE) locked = ocf_alock_trylock_entry_rd2wr(alock, entry); else if (waiter->rw == OCF_READ) @@ -538,7 +538,7 @@ static inline void ocf_alock_unlock_one_rd_common(struct ocf_alock *alock, } if (locked) { - exchanged = false; + exchanged = true; list_del(iter); ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); @@ -550,7 +550,7 @@ static inline void ocf_alock_unlock_one_rd_common(struct ocf_alock *alock, } } - if (exchanged) { + if (!exchanged) { /* No exchange, no waiters on the list, unlock and return * WR -> IDLE */ @@ -586,7 +586,7 @@ static inline void ocf_alock_unlock_one_wr_common(struct ocf_alock *alock, const ocf_cache_line_t entry) { bool locked = false; - bool exchanged = true; + bool exchanged = false; uint32_t idx = _WAITERS_LIST_ITEM(entry); struct ocf_alock_waiters_list *lst = &alock->waiters_lsts[idx]; @@ -608,7 +608,7 @@ static inline void ocf_alock_unlock_one_wr_common(struct ocf_alock *alock, if (entry != waiter->entry) continue; - if (exchanged) { + if (!exchanged) { if (waiter->rw == OCF_WRITE) locked = ocf_alock_trylock_entry_wr2wr(alock, entry); else if (waiter->rw == OCF_READ) @@ -625,7 +625,7 @@ static inline void ocf_alock_unlock_one_wr_common(struct ocf_alock *alock, } if (locked) { - exchanged = false; + exchanged = true; list_del(iter); ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); @@ -637,7 +637,7 @@ static inline void ocf_alock_unlock_one_wr_common(struct ocf_alock *alock, } } - if (exchanged) { + if (!exchanged) { /* No exchange, no waiters on the list, unlock and return * WR -> IDLE */ From a44dc4a54781e947f8d8445e12f8c5527bbcb061 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Tue, 15 Oct 2024 13:14:57 +0200 Subject: [PATCH 2/4] Single assert on unlocking cache line Signed-off-by: Michal Mielewczyk --- src/utils/utils_alock.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/utils/utils_alock.c b/src/utils/utils_alock.c index fa728fed..0fda0d9a 100644 --- a/src/utils/utils_alock.c +++ b/src/utils/utils_alock.c @@ -521,20 +521,18 @@ static inline void ocf_alock_unlock_one_rd_common(struct ocf_alock *alock, if (entry != waiter->entry) continue; + ENV_BUG_ON(waiter->rw != OCF_READ && waiter->rw != OCF_WRITE); + if (!exchanged) { if (waiter->rw == OCF_WRITE) locked = ocf_alock_trylock_entry_rd2wr(alock, entry); else if (waiter->rw == OCF_READ) locked = ocf_alock_trylock_entry_rd2rd(alock, entry); - else - ENV_BUG(); } else { if (waiter->rw == OCF_WRITE) locked = ocf_alock_trylock_entry_wr(alock, entry); else if (waiter->rw == OCF_READ) locked = ocf_alock_trylock_entry_rd(alock, entry); - else - ENV_BUG(); } if (locked) { @@ -608,20 +606,18 @@ static inline void ocf_alock_unlock_one_wr_common(struct ocf_alock *alock, if (entry != waiter->entry) continue; + ENV_BUG_ON(waiter->rw != OCF_READ && waiter->rw != OCF_WRITE); + if (!exchanged) { if (waiter->rw == OCF_WRITE) locked = ocf_alock_trylock_entry_wr2wr(alock, entry); else if (waiter->rw == OCF_READ) locked = ocf_alock_trylock_entry_wr2rd(alock, entry); - else - ENV_BUG(); } else { if (waiter->rw == OCF_WRITE) locked = ocf_alock_trylock_entry_wr(alock, entry); else if (waiter->rw == OCF_READ) locked = ocf_alock_trylock_entry_rd(alock, entry); - else - ENV_BUG(); } if (locked) { From 289629528db06054308be22fdd291aa4a6362aae Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Wed, 16 Oct 2024 08:19:24 +0200 Subject: [PATCH 3/4] posix env: list_for_each_entry_safe_from Signed-off-by: Michal Mielewczyk --- env/posix/ocf_env_list.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/env/posix/ocf_env_list.h b/env/posix/ocf_env_list.h index 90348af5..6412e69a 100644 --- a/env/posix/ocf_env_list.h +++ b/env/posix/ocf_env_list.h @@ -1,5 +1,6 @@ /* * Copyright(c) 2019-2021 Intel Corporation + * Copyright(c) 2024-2025 Huawei Technologies * SPDX-License-Identifier: BSD-3-Clause */ @@ -164,5 +165,19 @@ static inline void list_move_tail(struct list_head *it, struct list_head *l1) _list_entry_helper(item, (item)->field_name.next, field_name) != \ _list_entry_helper(item, (plist)->next, field_name); \ item = q, q = _list_entry_helper(q, (q)->field_name.next, field_name)) +/** + * Iterate over a list starting at specified element. Works even if entries are + * deleted during loop. + * @param list pointer to list item (iterator) + * @param q another pointer to list item, used as helper + * @param plist pointer to list_head item + * @param field_name name of list_head field in list entry + */ +#define list_for_each_entry_safe_from(item, q, plist, field_name) \ + for (q = _list_entry_helper(item, (item)->field_name.next, field_name); \ + _list_entry_helper(item, (item)->field_name.next, field_name) != \ + _list_entry_helper(item, (plist)->next, field_name); \ + item = q, q = _list_entry_helper(q, (q)->field_name.next, field_name)) + #endif // __OCF_ENV_LIST__ From 6c78681c920406f26d4ec763aaf95bdf1d220d74 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Wed, 16 Oct 2024 08:35:00 +0200 Subject: [PATCH 4/4] Refactor cache line unlock In the original loop, if a cache line had multiple waiters, there was a redundant trylock() after admitting the write lock to the first waiter. Returning just after write-locking the cache line eliminates the redundant trylock() and reduces the number of accesses to atomic variables. The second issue with the original code was caused by sharing waiter lists between different cache lines. Even if the first waiter was admitted the write lock, the loop was iterating over the waiter list until: a) finding another waiter to the same cache line; in this case it tried to lock the cache line which has just been write-locked so the operation always failed b) reaching the end of the waiter list. In both cases it makes no sense to continue iterating after admitting the write lock to the first watier. Signed-off-by: Michal Mielewczyk --- src/utils/utils_alock.c | 215 ++++++++++++++++++++++++---------------- 1 file changed, 131 insertions(+), 84 deletions(-) diff --git a/src/utils/utils_alock.c b/src/utils/utils_alock.c index 0fda0d9a..36066177 100644 --- a/src/utils/utils_alock.c +++ b/src/utils/utils_alock.c @@ -313,17 +313,16 @@ static inline void ocf_alock_unlock_entry_rd(struct ocf_alock *alock, env_atomic_dec(access); } -static inline bool ocf_alock_trylock_entry_wr2wr(struct ocf_alock *alock, +static inline void ocf_alock_lock_entry_wr2wr(struct ocf_alock *alock, ocf_cache_line_t entry) { env_atomic *access = &alock->access[entry]; int v = env_atomic_read(access); ENV_BUG_ON(v != OCF_CACHE_LINE_ACCESS_WR); - return true; } -static inline bool ocf_alock_trylock_entry_wr2rd(struct ocf_alock *alock, +static inline void ocf_alock_lock_entry_wr2rd(struct ocf_alock *alock, ocf_cache_line_t entry) { env_atomic *access = &alock->access[entry]; @@ -331,7 +330,6 @@ static inline bool ocf_alock_trylock_entry_wr2rd(struct ocf_alock *alock, ENV_BUG_ON(v != OCF_CACHE_LINE_ACCESS_WR); env_atomic_set(access, OCF_CACHE_LINE_ACCESS_ONE_RD); - return true; } static inline bool ocf_alock_trylock_entry_rd2wr(struct ocf_alock *alock, @@ -491,68 +489,95 @@ bool ocf_alock_lock_one_rd(struct ocf_alock *alock, } /* - * Unlocks the given read lock. If any waiters are registered for the same - * cacheline, one is awakened and the lock is either upgraded to a write lock - * or kept as a readlock. If there are no waiters, it's just unlocked. + * Unlocks the given read lock. + * + * The lock can be upgraded to write lock and passed to the first waiter if the + * current owner is the last one holding the read lock. + * + * If there are requests waiting for read access to the cache line, the lock + * will be granted to all such waiters that are not preceded by a write request */ static inline void ocf_alock_unlock_one_rd_common(struct ocf_alock *alock, const ocf_cache_line_t entry) { - bool locked = false; - bool exchanged = false; - + bool locked = false, no_waiters = true; + int rw; uint32_t idx = _WAITERS_LIST_ITEM(entry); struct ocf_alock_waiters_list *lst = &alock->waiters_lsts[idx]; - struct ocf_alock_waiter *waiter; - - struct list_head *iter, *next; + struct ocf_alock_waiter *waiter, *waiter_tmp; /* * Lock exchange scenario * 1. RD -> IDLE - * 2. RD -> RD - * 3. RD -> WR + * 2. RD -> WR + * 3. RD -> RD + * 4. RD -> Multiple RD */ - /* Check is requested page is on the list */ - list_for_each_safe(iter, next, &lst->head) { - waiter = list_entry(iter, struct ocf_alock_waiter, item); + /* Check if any request is waiting for the cache line */ + list_for_each_entry_safe(waiter, waiter_tmp, &lst->head, item) { + if (entry == waiter->entry) { + no_waiters = false; + break; + } + } + + /* RD -> IDLE */ + if (no_waiters) { + ocf_alock_unlock_entry_rd(alock, entry); + return; + } + + ENV_BUG_ON(waiter->rw != OCF_READ && waiter->rw != OCF_WRITE); + + /* RD -> WR/RD */ + if (waiter->rw == OCF_WRITE) + locked = ocf_alock_trylock_entry_rd2wr(alock, entry); + else if (waiter->rw == OCF_READ) + locked = ocf_alock_trylock_entry_rd2rd(alock, entry); + if (unlikely(!locked)) { + ocf_alock_unlock_entry_rd(alock, entry); + return; + } + + rw = waiter->rw; + + list_del(&waiter->item); + + ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); + ocf_alock_entry_locked(alock, waiter->req, waiter->cmpl); + + env_allocator_del(alock->allocator, waiter); + + /* If we upgraded to write lock, the read reaquests won't be able to + * lock the cache line anyways + */ + if (rw == OCF_WRITE) + return; + + waiter = waiter_tmp; + + /* RD -> Multiple RD */ + list_for_each_entry_safe_from(waiter, waiter_tmp, &lst->head, item) { if (entry != waiter->entry) continue; ENV_BUG_ON(waiter->rw != OCF_READ && waiter->rw != OCF_WRITE); - if (!exchanged) { - if (waiter->rw == OCF_WRITE) - locked = ocf_alock_trylock_entry_rd2wr(alock, entry); - else if (waiter->rw == OCF_READ) - locked = ocf_alock_trylock_entry_rd2rd(alock, entry); - } else { - if (waiter->rw == OCF_WRITE) - locked = ocf_alock_trylock_entry_wr(alock, entry); - else if (waiter->rw == OCF_READ) - locked = ocf_alock_trylock_entry_rd(alock, entry); - } + if (waiter->rw == OCF_WRITE) + return; - if (locked) { - exchanged = true; - list_del(iter); + locked = ocf_alock_trylock_entry_rd(alock, entry); + /* There is no limit for number of readers */ + ENV_BUG_ON(!locked); - ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); - ocf_alock_entry_locked(alock, waiter->req, waiter->cmpl); + list_del(&waiter->item); - env_allocator_del(alock->allocator, waiter); - } else { - break; - } - } + ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); + ocf_alock_entry_locked(alock, waiter->req, waiter->cmpl); - if (!exchanged) { - /* No exchange, no waiters on the list, unlock and return - * WR -> IDLE - */ - ocf_alock_unlock_entry_rd(alock, entry); + env_allocator_del(alock->allocator, waiter); } } @@ -576,68 +601,90 @@ void ocf_alock_unlock_one_rd(struct ocf_alock *alock, } /* - * Unlocks the given write lock. If any waiters are registered for the same - * cacheline, one is awakened and the lock is either downgraded to a readlock - * or kept as a writelock. If there are no waiters, it's just unlocked. + * Unlocks the given write lock. + * + * The lock can be passed to the first waiter + * + * If there are requests waiting for read access to the cache line, the lock + * will be downgraded and granted to all such waiters that are not preceded by + * a write request */ static inline void ocf_alock_unlock_one_wr_common(struct ocf_alock *alock, const ocf_cache_line_t entry) { - bool locked = false; - bool exchanged = false; - + bool locked = false, no_waiters = true; + int rw; uint32_t idx = _WAITERS_LIST_ITEM(entry); struct ocf_alock_waiters_list *lst = &alock->waiters_lsts[idx]; - struct ocf_alock_waiter *waiter; - - struct list_head *iter, *next; + struct ocf_alock_waiter *waiter, *waiter_tmp; /* * Lock exchange scenario * 1. WR -> IDLE - * 2. WR -> RD - * 3. WR -> WR + * 2. WR -> WR + * 3. WR -> RD + * 4. WR -> Multiple RD */ - /* Check is requested page is on the list */ - list_for_each_safe(iter, next, &lst->head) { - waiter = list_entry(iter, struct ocf_alock_waiter, item); + /* Check if any request is waiting for the cache line */ + list_for_each_entry_safe(waiter, waiter_tmp, &lst->head, item) { + if (entry == waiter->entry) { + no_waiters = false; + break; + } + } + + /* WR -> IDLE */ + if (no_waiters) { + ocf_alock_unlock_entry_wr(alock, entry); + return; + } + + ENV_BUG_ON(waiter->rw != OCF_READ && waiter->rw != OCF_WRITE); + /* WR -> WR/RD */ + if (waiter->rw == OCF_WRITE) + ocf_alock_lock_entry_wr2wr(alock, entry); + else if (waiter->rw == OCF_READ) + ocf_alock_lock_entry_wr2rd(alock, entry); + + rw = waiter->rw; + + list_del(&waiter->item); + + ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); + ocf_alock_entry_locked(alock, waiter->req, waiter->cmpl); + + env_allocator_del(alock->allocator, waiter); + + /* If we passed the write lock, the read requests won't be able to lock + * the cache line anyways + */ + if (rw == OCF_WRITE) + return; + + waiter = waiter_tmp; + + /* WR -> Multiple RD */ + list_for_each_entry_safe_from(waiter, waiter_tmp, &lst->head, item) { if (entry != waiter->entry) continue; ENV_BUG_ON(waiter->rw != OCF_READ && waiter->rw != OCF_WRITE); - if (!exchanged) { - if (waiter->rw == OCF_WRITE) - locked = ocf_alock_trylock_entry_wr2wr(alock, entry); - else if (waiter->rw == OCF_READ) - locked = ocf_alock_trylock_entry_wr2rd(alock, entry); - } else { - if (waiter->rw == OCF_WRITE) - locked = ocf_alock_trylock_entry_wr(alock, entry); - else if (waiter->rw == OCF_READ) - locked = ocf_alock_trylock_entry_rd(alock, entry); - } + if (waiter->rw == OCF_WRITE) + return; - if (locked) { - exchanged = true; - list_del(iter); + locked = ocf_alock_trylock_entry_rd(alock, entry); + /* There is no limit for number of readers */ + ENV_BUG_ON(!locked); - ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); - ocf_alock_entry_locked(alock, waiter->req, waiter->cmpl); + list_del(&waiter->item); - env_allocator_del(alock->allocator, waiter); - } else { - break; - } - } + ocf_alock_mark_index_locked(alock, waiter->req, waiter->idx, true); + ocf_alock_entry_locked(alock, waiter->req, waiter->cmpl); - if (!exchanged) { - /* No exchange, no waiters on the list, unlock and return - * WR -> IDLE - */ - ocf_alock_unlock_entry_wr(alock, entry); + env_allocator_del(alock->allocator, waiter); } }