[PATCH RESEND] sbitmap: Use atomic_{,long}_try_cmpxchg in sbitmap.c

[PATCH RESEND] sbitmap: Use atomic_{,long}_try_cmpxchg in sbitmap.c

[Uros Bizjak]

Use atomic_long_try_cmpxchg instead of
atomic_long_cmpxchg (*ptr, old, new) == old in __sbitmap_queue_get_batch
and atomic_try_cmpxchg instead of atomic_cmpxchg (*ptr, old, new) == old
in __sbq_wake_up. x86 CMPXCHG instruction returns success in ZF flag, so
this change saves a compare after cmpxchg (and related move instruction
in front of cmpxchg).

Also, atomic_long_cmpxchg implicitly assigns old *ptr value to "old"
when cmpxchg fails, enabling further code simplifications, e.g.
an extra memory read can be avoided in the loop.

No functional change intended.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 lib/sbitmap.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/lib/sbitmap.c b/lib/sbitmap.c
index 29eb0484215a..32392e65b98f 100644
--- a/lib/sbitmap.c
+++ b/lib/sbitmap.c
@@ -534,16 +534,16 @@ unsigned long __sbitmap_queue_get_batch(struct sbitmap_queue *sbq, int nr_tags,
 		if (nr + nr_tags <= map_depth) {
 			atomic_long_t *ptr = (atomic_long_t *) &map->word;
 			int map_tags = min_t(int, nr_tags, map_depth);
-			unsigned long val, ret;
+			unsigned long val;
 
 			get_mask = ((1UL << map_tags) - 1) << nr;
+			val = READ_ONCE(map->word);
 			do {
-				val = READ_ONCE(map->word);
 				if ((val & ~get_mask) != val)
 					goto next;
-				ret = atomic_long_cmpxchg(ptr, val, get_mask | val);
-			} while (ret != val);
-			get_mask = (get_mask & ~ret) >> nr;
+			} while (!atomic_long_try_cmpxchg(ptr, &val,
+							  get_mask | val));
+			get_mask = (get_mask & ~val) >> nr;
 			if (get_mask) {
 				*offset = nr + (index << sb->shift);
 				update_alloc_hint_after_get(sb, depth, hint,
@@ -612,8 +612,6 @@ static bool __sbq_wake_up(struct sbitmap_queue *sbq)
 
 	wait_cnt = atomic_dec_return(&ws->wait_cnt);
 	if (wait_cnt <= 0) {
-		int ret;
-
 		wake_batch = READ_ONCE(sbq->wake_batch);
 
 		/*
@@ -628,8 +626,7 @@ static bool __sbq_wake_up(struct sbitmap_queue *sbq)
 		 * atomic_cmpxhcg() race should call this function again
 		 * to wakeup a new batch on a different 'ws'.
 		 */
-		ret = atomic_cmpxchg(&ws->wait_cnt, wait_cnt, wake_batch);
-		if (ret == wait_cnt) {
+		if (atomic_try_cmpxchg(&ws->wait_cnt, &wait_cnt, wake_batch)) {
 			sbq_index_atomic_inc(&sbq->wake_index);
 			wake_up_nr(&ws->wait, wake_batch);
 			return false;
-- 
2.37.2

Re: [PATCH RESEND] sbitmap: Use atomic_{,long}_try_cmpxchg in sbitmap.c

[Jens Axboe]

On 9/7/22 12:21 PM, Uros Bizjak wrote:
> Use atomic_long_try_cmpxchg instead of
> atomic_long_cmpxchg (*ptr, old, new) == old in __sbitmap_queue_get_batch
> and atomic_try_cmpxchg instead of atomic_cmpxchg (*ptr, old, new) == old
> in __sbq_wake_up. x86 CMPXCHG instruction returns success in ZF flag, so
> this change saves a compare after cmpxchg (and related move instruction
> in front of cmpxchg).
> 
> Also, atomic_long_cmpxchg implicitly assigns old *ptr value to "old"
> when cmpxchg fails, enabling further code simplifications, e.g.
> an extra memory read can be avoided in the loop.
> 
> No functional change intended.

It doesn't apply to the current tree, can you please resend one against
for-6.1/block?

-- 
Jens Axboe