Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in
nvmet_update_sq_head. 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. There is no need to re-read the value in the loop.
Note that the value from *ptr should be read using READ_ONCE to prevent
the compiler from merging, refetching or reordering the read.
No functional change intended.
Cc: Christoph Hellwig <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Chaitanya Kulkarni <[email protected]>
Signed-off-by: Uros Bizjak <[email protected]>
---
drivers/nvme/target/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 14677145bbba..a384a0927aed 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -695,11 +695,10 @@ static void nvmet_update_sq_head(struct nvmet_req *req)
if (req->sq->size) {
u32 old_sqhd, new_sqhd;
+ old_sqhd = READ_ONCE(req->sq->sqhd);
do {
- old_sqhd = req->sq->sqhd;
new_sqhd = (old_sqhd + 1) % req->sq->size;
- } while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) !=
- old_sqhd);
+ } while (!try_cmpxchg(&req->sq->sqhd, &old_sqhd, new_sqhd));
}
req->cqe->sq_head = cpu_to_le16(req->sq->sqhd & 0x0000FFFF);
}
--
2.37.3
Reviewed-by: Sagi Grimberg <[email protected]>
On 10/20/22 08:35, Uros Bizjak wrote:
> Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in
> nvmet_update_sq_head. x86 CMPXCHG instruction returns success in ZF flag, so
> this change saves a compare after cmpxchg (and related move instruction in
> front of cmpxchg).
>
Is it worth a share delts of assembly instructions of the changes above?
as developers on block mailing list are sharing the delta between before
and after patch including the assembly.
I also hope that you have tested this change with blktests nvme.
Either way:-
Reviewed-by: ChaItanya Kulkarni <[email protected]>
-ck
On Tue, Oct 25, 2022 at 2:47 AM Chaitanya Kulkarni
<[email protected]> wrote:
>
> On 10/20/22 08:35, Uros Bizjak wrote:
> > Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in
> > nvmet_update_sq_head. x86 CMPXCHG instruction returns success in ZF flag, so
> > this change saves a compare after cmpxchg (and related move instruction in
> > front of cmpxchg).
> >
>
> Is it worth a share delts of assembly instructions of the changes above?
> as developers on block mailing list are sharing the delta between before
> and after patch including the assembly.
The difference in the assembly of nvmet_update_sq_head function is:
before:
0000000000001d30 <nvmet_update_sq_head>:
1d30: 48 8b 4f 10 mov 0x10(%rdi),%rcx
1d34: 49 89 f8 mov %rdi,%r8
1d37: 0f b7 71 1a movzwl 0x1a(%rcx),%esi
1d3b: 66 85 f6 test %si,%si
1d3e: 75 14 jne 1d54 <nvmet_update_sq_head+0x24>
1d40: 49 8b 40 08 mov 0x8(%r8),%rax
1d44: 8b 51 1c mov 0x1c(%rcx),%edx
1d47: 66 89 50 08 mov %dx,0x8(%rax)
1d4b: e9 00 00 00 00 jmpq 1d50 <nvmet_update_sq_head+0x20>
1d4c: R_X86_64_PLT32 __x86_return_thunk-0x4
1d50: 0f b7 71 1a movzwl 0x1a(%rcx),%esi
1d54: 8b 79 1c mov 0x1c(%rcx),%edi
1d57: 31 d2 xor %edx,%edx
1d59: 8d 47 01 lea 0x1(%rdi),%eax
1d5c: f7 f6 div %esi
1d5e: 89 f8 mov %edi,%eax
1d60: f0 0f b1 51 1c lock cmpxchg %edx,0x1c(%rcx)
1d65: 49 8b 48 10 mov 0x10(%r8),%rcx
1d69: 39 c7 cmp %eax,%edi
1d6b: 75 e3 jne 1d50 <nvmet_update_sq_head+0x20>
1d6d: 49 8b 40 08 mov 0x8(%r8),%rax
1d71: 8b 51 1c mov 0x1c(%rcx),%edx
1d74: 66 89 50 08 mov %dx,0x8(%rax)
1d78: e9 00 00 00 00 jmpq 1d7d <nvmet_update_sq_head+0x4d>
1d79: R_X86_64_PLT32 __x86_return_thunk-0x4
1d7d:
after:
0000000000001d30 <nvmet_update_sq_head>:
1d30: 48 8b 4f 10 mov 0x10(%rdi),%rcx
1d34: 0f b7 51 1a movzwl 0x1a(%rcx),%edx
1d38: 66 85 d2 test %dx,%dx
1d3b: 74 1e je 1d5b <nvmet_update_sq_head+0x2b>
1d3d: 8b 71 1c mov 0x1c(%rcx),%esi
1d40: 44 0f b7 c2 movzwl %dx,%r8d
1d44: 8d 46 01 lea 0x1(%rsi),%eax
1d47: 31 d2 xor %edx,%edx
1d49: 41 f7 f0 div %r8d
1d4c: 89 f0 mov %esi,%eax
1d4e: f0 0f b1 51 1c lock cmpxchg %edx,0x1c(%rcx)
1d53: 48 8b 4f 10 mov 0x10(%rdi),%rcx
1d57: 89 c6 mov %eax,%esi
1d59: 75 10 jne 1d6b <nvmet_update_sq_head+0x3b>
1d5b: 48 8b 47 08 mov 0x8(%rdi),%rax
1d5f: 8b 51 1c mov 0x1c(%rcx),%edx
1d62: 66 89 50 08 mov %dx,0x8(%rax)
1d66: e9 00 00 00 00 jmpq 1d6b <nvmet_update_sq_head+0x3b>
1d67: R_X86_64_PLT32 __x86_return_thunk-0x4
1d6b: 0f b7 51 1a movzwl 0x1a(%rcx),%edx
1d6f: eb cf jmp 1d40 <nvmet_update_sq_head+0x10>
1d71:
You can see that in addition to the smaller size of the function, the
load of req->sq->size at 1d6b got moved to a cold path. As the main
benefit, the load at 1d3d is now out of the loop, and the value in
%esi is now provided by cmpxchg insn itself at 1d4e (plus move at
1d57). Unfortunately, the division clobbers %eax, so some reg-reg
moves are necessary. Note also that the compare at 1d69 is now gone.
> I also hope that you have tested this change with blktests nvme.
No, I didn't test the patch that thoroughly, but the change is the
same as some similar recent changes in the generic code, so I
confirmed the patch by inspecting asm code. OTOH, the kernel booted
from a NVME SSD.
> Either way:-
>
> Reviewed-by: ChaItanya Kulkarni <[email protected]>
Thanks!
Uros.