From 19e964cc07441e8a5217964a0bcc080e45d90205 Mon Sep 17 00:00:00 2001 From: James Peach Date: Thu, 15 Sep 2016 11:56:57 -0700 Subject: [PATCH 1/2] TS-4872: Simplify atomic list header #ifdefs. --- lib/ts/ink_queue.h | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/ts/ink_queue.h b/lib/ts/ink_queue.h index 6794fb4d02e..0a5c7689082 100644 --- a/lib/ts/ink_queue.h +++ b/lib/ts/ink_queue.h @@ -85,20 +85,21 @@ void ink_queue_load_64(void *dst, void *src); // lock, use INK_QUEUE_LD to read safely. typedef union { #if (defined(__i386__) || defined(__arm__) || defined(__mips__)) && (SIZEOF_VOIDP == 4) - struct { - void *pointer; - int32_t version; - } s; - int64_t data; + typedef int32_t version_type; + typedef int64_t data_type; #elif TS_HAS_128BIT_CAS + typedef int64_t version_type; + typedef __int128_t data_type; +#else + typedef int64_t data_type; +#endif + struct { void *pointer; - int64_t version; + version_type version; } s; - __int128_t data; -#else - int64_t data; -#endif + + data_type data; } head_p; /* From cb566cc3cf19892a07d7d274af2b045e957d6ce1 Mon Sep 17 00:00:00 2001 From: James Peach Date: Thu, 15 Sep 2016 11:58:02 -0700 Subject: [PATCH 2/2] TS-4872: Hoist lock-free log buffer manipulation. Hoist the lock-free log buffer manipulations into helper functions. This make is a bit clearer what they are trying to do, and clarifies things so that the clang static analyzer doesn't think there is a memory leak here. --- proxy/logging/LogObject.cc | 54 +++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/proxy/logging/LogObject.cc b/proxy/logging/LogObject.cc index b93a0d5e82f..036de1866a0 100644 --- a/proxy/logging/LogObject.cc +++ b/proxy/logging/LogObject.cc @@ -354,6 +354,29 @@ LogObject::display(FILE *fd) fprintf(fd, "++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n"); } +static head_p +increment_pointer_version(volatile head_p *dst) +{ + head_p h; + head_p new_h; + + do { + INK_QUEUE_LD(h, *dst); + SET_FREELIST_POINTER_VERSION(new_h, FREELIST_POINTER(h), FREELIST_VERSION(h) + 1); + } while (ink_atomic_cas(&dst->data, h.data, new_h.data) == false); + + return h; +} + +static bool +write_pointer_version(volatile head_p *dst, head_p old_h, void *ptr, head_p::version_type vers) +{ + head_p tmp_h; + + SET_FREELIST_POINTER_VERSION(tmp_h, ptr, vers); + return ink_atomic_cas(&dst->data, old_h.data, tmp_h.data); +} + LogBuffer * LogObject::_checkout_write(size_t *write_offset, size_t bytes_needed) { @@ -365,14 +388,10 @@ LogObject::_checkout_write(size_t *write_offset, size_t bytes_needed) do { // To avoid a race condition, we keep a count of held references in // the pointer itself and add this to m_outstanding_references. - head_p h; - int result = 0; - do { - INK_QUEUE_LD(h, m_log_buffer); - head_p new_h; - SET_FREELIST_POINTER_VERSION(new_h, FREELIST_POINTER(h), FREELIST_VERSION(h) + 1); - result = ink_atomic_cas(&m_log_buffer.data, h.data, new_h.data); - } while (!result); + + // Increment the version of m_log_buffer, returning the previous version. + head_p h = increment_pointer_version(&m_log_buffer); + buffer = (LogBuffer *)FREELIST_POINTER(h); result_code = buffer->checkout_write(write_offset, bytes_needed); bool decremented = false; @@ -392,6 +411,7 @@ LogObject::_checkout_write(size_t *write_offset, size_t bytes_needed) // swap the new buffer for the old one INK_WRITE_MEMORY_BARRIER; head_p old_h; + do { INK_QUEUE_LD(old_h, m_log_buffer); if (FREELIST_POINTER(old_h) != FREELIST_POINTER(h)) { @@ -402,10 +422,8 @@ LogObject::_checkout_write(size_t *write_offset, size_t bytes_needed) delete new_buffer; break; } - head_p tmp_h; - SET_FREELIST_POINTER_VERSION(tmp_h, new_buffer, 0); - result = ink_atomic_cas(&m_log_buffer.data, old_h.data, tmp_h.data); - } while (!result); + } while (!write_pointer_version(&m_log_buffer, old_h, new_buffer, 0)); + if (FREELIST_POINTER(old_h) == FREELIST_POINTER(h)) { ink_atomic_increment(&buffer->m_references, FREELIST_VERSION(old_h) - 1); @@ -413,7 +431,9 @@ LogObject::_checkout_write(size_t *write_offset, size_t bytes_needed) Debug("log-logbuffer", "adding buffer %d to flush list after checkout", buffer->get_id()); m_buffer_manager[idx].add_to_flush_queue(buffer); Log::preproc_notify[idx].signal(); + buffer = NULL; } + decremented = true; break; @@ -434,21 +454,23 @@ LogObject::_checkout_write(size_t *write_offset, size_t bytes_needed) default: ink_assert(false); } + if (!decremented) { head_p old_h; + do { INK_QUEUE_LD(old_h, m_log_buffer); if (FREELIST_POINTER(old_h) != FREELIST_POINTER(h)) { break; } - head_p tmp_h; - SET_FREELIST_POINTER_VERSION(tmp_h, FREELIST_POINTER(h), FREELIST_VERSION(old_h) - 1); - result = ink_atomic_cas(&m_log_buffer.data, old_h.data, tmp_h.data); - } while (!result); + + } while (!write_pointer_version(&m_log_buffer, old_h, FREELIST_POINTER(h), FREELIST_VERSION(old_h) - 1)); + if (FREELIST_POINTER(old_h) != FREELIST_POINTER(h)) { ink_atomic_increment(&buffer->m_references, -1); } } + } while (retry && write_offset); // if write_offset is null, we do // not retry because we really do // not want to write to the buffer