[PATCH] nvmet: use try_cmpxchg in nvmet_update_sq_head

[PATCH] nvmet: use try_cmpxchg in nvmet_update_sq_head

[Uros Bizjak]

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 <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 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

Re: [PATCH] nvmet: use try_cmpxchg in nvmet_update_sq_head

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvmet: use try_cmpxchg in nvmet_update_sq_head

[Chaitanya Kulkarni]

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 <kch@nvidia.com>

-ck

Re: [PATCH] nvmet: use try_cmpxchg in nvmet_update_sq_head

[Uros Bizjak]

On Tue, Oct 25, 2022 at 2:47 AM Chaitanya Kulkarni
<chaitanyak@nvidia.com> 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 <kch@nvidia.com>

Thanks!

Uros.

Re: [PATCH] nvmet: use try_cmpxchg in nvmet_update_sq_head

[Christoph Hellwig]

Thanks,

applied to nvme-6.2.