From: Radoslaw Zarzynski <rzarzyns@redhat.com>
Date: Wed, 24 Jun 2020 20:32:16 +0000 (+0200)
Subject: common/bl: document and slightly optimize ptr::release().
X-Git-Tag: wip-pdonnell-testing-20200918.022351~759^2
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=d427acf4a5db758ce47260069ec6d0c08aa2ff79;p=ceph-ci.git

common/bl: document and slightly optimize ptr::release().

Before the patch GCC was generating:
```
  10f9910:   mov    (%rdi),%rax
  10f9913:   test   %rax,%rax
  10f9916:   je     10f9951 <ceph::buffer::v15_2_0::ptr::release()+0x41>
  10f9918:   mov    0x2c(%rax),%eax
  10f991b:   cmp    $0x1,%eax
  10f991e:   jne    10f9940 <ceph::buffer::v15_2_0::ptr::release()+0x30>
  10f9920:   mov    (%rdi),%rax
  10f9923:   movq   $0x0,(%rdi)
  10f992a:   test   %rax,%rax
  10f992d:   je     10f9958 <ceph::buffer::v15_2_0::ptr::release()+0x48>
  10f992f:   mov    (%rax),%rdx
  10f9932:   mov    %rax,%rdi
  10f9935:   mov    0x8(%rdx),%rdx
  10f9939:   jmpq   *%rdx
  10f993b:   nopl   0x0(%rax,%rax,1)
  10f9940:   mov    (%rdi),%rax
  10f9943:   lock subl $0x1,0x2c(%rax)
  10f9948:   je     10f9920 <ceph::buffer::v15_2_0::ptr::release()+0x10>
  10f994a:   movq   $0x0,(%rdi)
  10f9951:   retq
  10f9952:   nopw   0x0(%rax,%rax,1)
  10f9958:   retq
  10f9959:   nop
  10f995a:   nopw   0x0(%rax,%rax,1)
```

after:
```
  11209b0:   mov    (%rdi),%rax
  11209b3:   movq   $0x0,(%rdi)
  11209ba:   test   %rax,%rax
  11209bd:   je     11209e6 <ceph::buffer::v15_2_0::ptr::release()+0x36>
  11209bf:   mov    0x2c(%rax),%edx
  11209c2:   lea    0x2c(%rax),%rcx
  11209c6:   cmp    $0x1,%edx
  11209c9:   jne    11209e0 <ceph::buffer::v15_2_0::ptr::release()+0x30>
  11209cb:   mov    (%rax),%rdx
  11209ce:   mov    %rax,%rdi
  11209d1:   mov    0x8(%rdx),%rdx
  11209d5:   jmpq   *%rdx
  11209d7:   nopw   0x0(%rax,%rax,1)
  11209de:
  11209e0:   lock subl $0x1,(%rcx)
  11209e4:   je     11209cb <ceph::buffer::v15_2_0::ptr::release()+0x1b>
  11209e6:   retq
```

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
---

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index 331ad890e55..bf4b427cc24 100644
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -457,21 +457,29 @@ static ceph::spinlock debug_lock;
 
   void buffer::ptr::release()
   {
-    if (_raw) {
-      bdout << "ptr " << this << " release " << _raw << bendl;
-      const bool last_one = (1 == _raw->nref.load(std::memory_order_acquire));
-      if (likely(last_one) || --_raw->nref == 0) {
-        // BE CAREFUL: this is called also for hypercombined ptr_node. After
-        // freeing underlying raw, `*this` can become inaccessible as well!
-        const auto* delete_raw = _raw;
-        _raw = nullptr;
-	//cout << "hosing raw " << (void*)_raw << " len " << _raw->len << std::endl;
-        ANNOTATE_HAPPENS_AFTER(&delete_raw->nref);
-        ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&delete_raw->nref);
-	delete delete_raw;  // dealloc old (if any)
+    // BE CAREFUL: this is called also for hypercombined ptr_node. After
+    // freeing underlying raw, `*this` can become inaccessible as well!
+    //
+    // cache the pointer to avoid unncecessary reloads and repeated
+    // checks.
+    if (auto* const cached_raw = std::exchange(_raw, nullptr);
+	cached_raw) {
+      bdout << "ptr " << this << " release " << cached_raw << bendl;
+      // optimize the common case where a particular `buffer::raw` has
+      // only a single reference. Altogether with initializing `nref` of
+      // freshly fabricated one with `1` through the std::atomic's ctor
+      // (which doesn't impose a memory barrier on the strongly-ordered
+      // x86), this allows to avoid all atomical operations in such case.
+      const bool last_one = \
+        (1 == cached_raw->nref.load(std::memory_order_acquire));
+      if (likely(last_one) || --cached_raw->nref == 0) {
+	bdout << "deleting raw " << static_cast<void*>(cached_raw)
+	      << " len " << cached_raw->len << bendl;
+	ANNOTATE_HAPPENS_AFTER(&cached_raw->nref);
+	ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&cached_raw->nref);
+	delete cached_raw;  // dealloc old (if any)
       } else {
-        ANNOTATE_HAPPENS_BEFORE(&_raw->nref);
-        _raw = nullptr;
+	ANNOTATE_HAPPENS_BEFORE(&cached_raw->nref);
       }
     }
   }