[PATCH] block: Use try_cmpxchg some more

[PATCH] block: Use try_cmpxchg some more

[Uros Bizjak]

Use try_cmpxchg family of functions instead of cmpxchg (*ptr, old, new) == old.
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, try_cmpxchg implicitly assigns old *ptr value to "old" when
cmpxchg fails, enabling further code simplifications.

No functional change intended.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-cgroup.c    |  2 +-
 block/blk-cgroup.h    | 12 ++++--------
 block/blk-core.c      |  2 +-
 block/blk-iolatency.c | 12 +++++-------
 block/blk-rq-qos.c    | 10 ++--------
 5 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 764e740b0c0f..ca99260348ff 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1696,7 +1696,7 @@ static void blkcg_scale_delay(struct blkcg_gq *blkg, u64 now)
 	 * everybody is happy with their IO latencies.
 	 */
 	if (time_before64(old + NSEC_PER_SEC, now) &&
-	    atomic64_cmpxchg(&blkg->delay_start, old, now) == old) {
+	    atomic64_try_cmpxchg(&blkg->delay_start, &old, now)) {
 		u64 cur = atomic64_read(&blkg->delay_nsec);
 		u64 sub = min_t(u64, blkg->last_delay, now - old);
 		int cur_use = atomic_read(&blkg->use_delay);
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index d4de0a35e066..0fc5009be5f9 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -430,12 +430,8 @@ static inline int blkcg_unuse_delay(struct blkcg_gq *blkg)
 	 * then check to see if we were the last delay so we can drop the
 	 * congestion count on the cgroup.
 	 */
-	while (old) {
-		int cur = atomic_cmpxchg(&blkg->use_delay, old, old - 1);
-		if (cur == old)
-			break;
-		old = cur;
-	}
+	do {
+	} while (old && !atomic_try_cmpxchg(&blkg->use_delay, &old, old - 1));
 
 	if (old == 0)
 		return 0;
@@ -458,7 +454,7 @@ static inline void blkcg_set_delay(struct blkcg_gq *blkg, u64 delay)
 	int old = atomic_read(&blkg->use_delay);
 
 	/* We only want 1 person setting the congestion count for this blkg. */
-	if (!old && atomic_cmpxchg(&blkg->use_delay, old, -1) == old)
+	if (!old && atomic_try_cmpxchg(&blkg->use_delay, &old, -1))
 		atomic_inc(&blkg->blkcg->css.cgroup->congestion_count);
 
 	atomic64_set(&blkg->delay_nsec, delay);
@@ -475,7 +471,7 @@ static inline void blkcg_clear_delay(struct blkcg_gq *blkg)
 	int old = atomic_read(&blkg->use_delay);
 
 	/* We only want 1 person clearing the congestion count for this blkg. */
-	if (old && atomic_cmpxchg(&blkg->use_delay, old, 0) == old)
+	if (old && atomic_try_cmpxchg(&blkg->use_delay, &old, 0))
 		atomic_dec(&blkg->blkcg->css.cgroup->congestion_count);
 }
 
diff --git a/block/blk-core.c b/block/blk-core.c
index 27fb1357ad4b..628b965356db 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -987,7 +987,7 @@ void update_io_ticks(struct block_device *part, unsigned long now, bool end)
 again:
 	stamp = READ_ONCE(part->bd_stamp);
 	if (unlikely(time_after(now, stamp))) {
-		if (likely(cmpxchg(&part->bd_stamp, stamp, now) == stamp))
+		if (likely(try_cmpxchg(&part->bd_stamp, &stamp, now)))
 			__part_stat_add(part, io_ticks, end ? now - stamp : 1);
 	}
 	if (part->bd_partno) {
diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index 9568bf8dfe82..79745c6d8e15 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -401,7 +401,6 @@ static void check_scale_change(struct iolatency_grp *iolat)
 	unsigned int cur_cookie;
 	unsigned int our_cookie = atomic_read(&iolat->scale_cookie);
 	u64 scale_lat;
-	unsigned int old;
 	int direction = 0;
 
 	if (lat_to_blkg(iolat)->parent == NULL)
@@ -422,11 +421,10 @@ static void check_scale_change(struct iolatency_grp *iolat)
 	else
 		return;
 
-	old = atomic_cmpxchg(&iolat->scale_cookie, our_cookie, cur_cookie);
-
-	/* Somebody beat us to the punch, just bail. */
-	if (old != our_cookie)
+	if (!atomic_try_cmpxchg(&iolat->scale_cookie, &our_cookie, cur_cookie)) {
+		/* Somebody beat us to the punch, just bail. */
 		return;
+	}
 
 	if (direction < 0 && iolat->min_lat_nsec) {
 		u64 samples_thresh;
@@ -633,8 +631,8 @@ static void blkcg_iolatency_done_bio(struct rq_qos *rqos, struct bio *bio)
 			window_start = atomic64_read(&iolat->window_start);
 			if (now > window_start &&
 			    (now - window_start) >= iolat->cur_win_nsec) {
-				if (atomic64_cmpxchg(&iolat->window_start,
-					     window_start, now) == window_start)
+				if (atomic64_try_cmpxchg(&iolat->window_start,
+							 &window_start, now))
 					iolatency_check_latencies(iolat, now);
 			}
 		}
diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
index d3a75693adbf..88f0fe7dcf54 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -10,16 +10,10 @@ static bool atomic_inc_below(atomic_t *v, unsigned int below)
 {
 	unsigned int cur = atomic_read(v);
 
-	for (;;) {
-		unsigned int old;
-
+	do {
 		if (cur >= below)
 			return false;
-		old = atomic_cmpxchg(v, cur, cur + 1);
-		if (old == cur)
-			break;
-		cur = old;
-	}
+	} while (!atomic_try_cmpxchg(v, &cur, cur + 1));
 
 	return true;
 }
-- 
2.35.3

Re: [PATCH] block: Use try_cmpxchg some more

[Christoph Hellwig]

On Mon, Jul 11, 2022 at 05:33:01PM +0200, Uros Bizjak wrote:
> Use try_cmpxchg family of functions instead of cmpxchg (*ptr, old, new) == old.
> 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, try_cmpxchg implicitly assigns old *ptr value to "old" when
> cmpxchg fails, enabling further code simplifications.
> 
> No functional change intended.

You might want to split this into a patch per caller as it might
attact different reviewers.

> +	do {
> +	} while (old && !atomic_try_cmpxchg(&blkg->use_delay, &old, old - 1));

It might just be me, but for loops with an empty body this do { } while
construct looks odd.  Why not:

	while (old && !atomic_try_cmpxchg(&blkg->use_delay, &old, old - 1))
		;

?

The the use of the atomic on ->use_delay looks really whacky to start
with.  To me it sounds like it really wants to use a proper lock
instead of all this magic.

>  	else
>  		return;
>  
> -	old = atomic_cmpxchg(&iolat->scale_cookie, our_cookie, cur_cookie);
> -
> -	/* Somebody beat us to the punch, just bail. */
> -	if (old != our_cookie)
> +	if (!atomic_try_cmpxchg(&iolat->scale_cookie, &our_cookie, cur_cookie)) {
> +		/* Somebody beat us to the punch, just bail. */
>  		return;
> +	}


	/* If somebody beat us to the punch, just bail. */
	if (!atomic_try_cmpxchg(&iolat->scale_cookie, &our_cookie, cur_cookie))
		return;

Re: [PATCH] block: Use try_cmpxchg some more

[Uros Bizjak]

On Tue, Jul 12, 2022 at 7:13 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jul 11, 2022 at 05:33:01PM +0200, Uros Bizjak wrote:
> > Use try_cmpxchg family of functions instead of cmpxchg (*ptr, old, new) == old.
> > 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, try_cmpxchg implicitly assigns old *ptr value to "old" when
> > cmpxchg fails, enabling further code simplifications.
> >
> > No functional change intended.
>
> You might want to split this into a patch per caller as it might
> attact different reviewers.

No problem for me. get_maintainer.pl returned Jens as the sole
maintainer for all these parts, so I put everything together in order
to ease the maintainer's job.

> > +     do {
> > +     } while (old && !atomic_try_cmpxchg(&blkg->use_delay, &old, old - 1));
>
> It might just be me, but for loops with an empty body this do { } while
> construct looks odd.  Why not:
>
>         while (old && !atomic_try_cmpxchg(&blkg->use_delay, &old, old - 1))
>                 ;

The form was taken from e6790e4b5d5e97dc287f3496dd2cf2dbabdfdb35 [1].
Using try_cmpxhchg, almost every use fits in

do {
    // the body of the loop
} while (try_cmpxchg ...)

and when the body of the loop is empty, it is clear that this was
indeed intended. Using

while (try_cmpxchg ...);

looks to me like a semicolon was left there in error, like "if (...);".

> The the use of the atomic on ->use_delay looks really whacky to start
> with.  To me it sounds like it really wants to use a proper lock
> instead of all this magic.

I took a lot of care not to change the functionality of the
surrounding code, and any functional change should be outside of the
scope of the patch.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e6790e4b5d5e97dc287f3496dd2cf2dbabdfdb35

Uros.

Re: [PATCH] block: Use try_cmpxchg some more

[Bart Van Assche]

On 7/12/22 00:04, Uros Bizjak wrote:
> while (try_cmpxchg ...);
> 
> looks to me like a semicolon was left there in error, like "if (...);".

Putting the semicolon at the end is not what Christoph suggested and is 
not allowed by the kernel coding style.

I also prefer the while-loop instead of do-while.

Thanks,

Bart.