ywkaras commented on a change in pull request #7382: URL: https://github.com/apache/trafficserver/pull/7382#discussion_r540449187
########## File path: include/tscore/ver_ptr.h ########## @@ -0,0 +1,288 @@ +/** @file + + Provides a "versioned pointer" data type. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#pragma once + +#include <cstdint> +#include <cstring> +#include <atomic> + +namespace ts +{ +namespace detail +{ + namespace versioned_ptr + { + // This is true if pointers are 64 bits but the two most significant bytes are not used, except for the + // most significant bit. + // + bool const Ptr64bits15unused = +#if defined(__x86_64__) || defined(__ia64__) || defined(__powerpc64__) || defined(__aarch64__) || defined(__mips64) + true +#else + false +#endif + ; + +#if TS_HAS_128BIT_CAS + + bool const CAE128 = true; + + using Bits128_type = __int128_t; + + static_assert(sizeof(Bits128_type) == 16); + + // This static assert is commented out because it fails. It seems that our Automake voodoo sets + // TS_HAS_128BIT_CAS to 1 even if there is no lock-free 128-bit CAS available. See + // https://godbolt.org/z/ncYsv9 . Looking at the generated assembly code, the 128-bit CAS operations + // cause calls into the run-time library, which may mean that are not actually lock-free. + // See also + // + // static_assert(std::atomic<Bits128_type>::is_always_lock_free); + +#else + + bool const CAE128 = false; + + using Bits128_type = char; // Not used. + +#endif + + struct VPS { + void *ptr; + unsigned version; + }; + + inline bool + operator==(VPS const &a, VPS const &b) + { + return (a.ptr == b.ptr) && (a.version == b.version); + } + + template <typename AAT, bool Version_in_ptr> class Type_ + { + public: + using Atomic_access_type = AAT; + + explicit Type_(void *p = nullptr, unsigned v = 0) + { + _vp.ptr = p; + _vp.version = v; + + const std::size_t Pad = sizeof(_aa) - sizeof(_vp); + if (Pad) { + // Make sure pad bytes are consistently zero. + // + std::memset(reinterpret_cast<char *>(&_vp) + sizeof(_vp), 0, Pad); + } + } + + explicit Type_(Atomic_access_type const &aa) { _aa = aa; } + + void * + ptr() const + { + return _vp.ptr; + } + + void + ptr(void *p) + { + _vp.ptr = p; + } + + unsigned + version() const + { + return _vp.version; + } + + void + version(unsigned v) + { + _vp.version = v; + } + + private: + union { + VPS _vp; + AAT _aa; + }; + + static_assert(sizeof(_vp) <= sizeof(_aa)); + }; + + namespace + { + unsigned const Version_bits = 15; + unsigned const Version_shift = 48; + constexpr unsigned + Mask_version(unsigned v) + { + return v & ((1U << (Version_bits + 1)) - 1); + } + std::uint64_t const Version_mask = static_cast<std::uint64_t>(Mask_version(~0U)) << Version_shift; + } // namespace + + template <> class Type_<std::uint64_t, true> + { + public: + using Atomic_access_type = std::uint64_t; + + Type_(){}; + + explicit Type_(void *p, unsigned v = 0) + { + _ptr = reinterpret_cast<std::uint64_t>(p) & ~Version_mask; + _ptr |= static_cast<uint64_t>(Mask_version(v)) << Version_shift; + } + + explicit Type_(Atomic_access_type const &ptr) { _ptr = ptr; } + + void * + ptr() const + { + return reinterpret_cast<void *>(_ptr & ~Version_mask); + } + + void + ptr(void *p) + { + _ptr &= Version_mask; + _ptr |= reinterpret_cast<std::uint64_t>(p) & ~Version_mask; + } + + unsigned + version() const + { + return Mask_version(static_cast<unsigned>(_ptr >> Version_shift)); + } + + void + version(unsigned v) + { + _ptr &= ~Version_mask; + _ptr |= static_cast<std::uint64_t>(Mask_version(v)) << Version_shift; + } + + private: + std::uint64_t _ptr; + }; + + enum Impl_type { + DEFAULT, // Atomic access may not be lock free. + VERSION_IN_PTR, // Use bits inside of 64-bit pointer to store version. + BITS64, // VPS structure is less than 64 bits, and 64-bit access is lock free. + BITS128 // VPS structure is less than 128 bits, and 128-bit compare-and-exchange is available. + }; + + constexpr Impl_type + Impl() + { + if (std::atomic<std::uint64_t>::is_always_lock_free) { + if (sizeof(VPS) <= 8) { + return BITS64; + + } else if (Ptr64bits15unused && (sizeof(void *) == 8)) { + return VERSION_IN_PTR; Review comment: For 64-bit architectures, I am favoring the approach where the version in stored in ignored bits in the pointer. Currently, ATS favors using 128-bit compare-and-swap/exchange if TS_HAS_128BIT_CAS is set to 1 in ink_config.h ( https://github.com/apache/trafficserver/blob/472075bd7b5755cc1d8bf9a4bbf750474c4da53c/include/tscore/ink_queue.h#L136 ). But I think this flag may be set to 1 even if the 128-bit CAS is not atomic. Looking at this example code: https://godbolt.org/z/ncYsv9 , the 128-bit CAS results in a call to a runtime library function (which I suspect contains a lock). Interestingly, the 128-bit CAS translates directly to a HW instruction for ARM v8: https://godbolt.org/z/aennnM . ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
