chenBright commented on code in PR #3541: URL: https://github.com/apache/brpc/pull/3541#discussion_r3999920130
########## src/butil/synchronization/seqlock.h: ########## @@ -0,0 +1,302 @@ +// 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. + +#ifndef BUTIL_SYNCHRONIZATION_SEQLOCK_H +#define BUTIL_SYNCHRONIZATION_SEQLOCK_H + +#include <stdint.h> +#include <functional> +#include <mutex> +#include <type_traits> +#include <utility> + +#include "butil/atomicops.h" +#include "butil/compiler_specific.h" +#include "butil/macros.h" +#include "butil/processor.h" + +namespace butil { +namespace internal { + +// Detects std::reference_wrapper<T>. +template <typename T> +struct IsReferenceWrapper : std::false_type {}; +template <typename T> +struct IsReferenceWrapper<std::reference_wrapper<T>> : std::true_type {}; + +// A sequence counter for consistent reads without acquiring a reader mutex +// around caller-owned payloads. This is an implementation detail of butil::Seqlock; +// use butil::Seqlock instead. +// +// SeqCounter does not protect payload accesses from C++ data races. Callers must +// access shared payloads atomically (typically with relaxed ordering) or use +// another mechanism that makes concurrent accesses valid. +// +// +// Cache-line aligned so the sequence counter does not falsely share a line +// with adjacent data (e.g. the writer mutex in Seqlock<Mutex> or a neighbouring +// payload), which would bounce the line between readers and writers. +class BAIDU_CACHELINE_ALIGNMENT SeqCounter { +public: + SeqCounter() : _seq(0) {} + DISALLOW_COPY_AND_ASSIGN(SeqCounter); + + // Repeatedly invoke `load_payload` until it observes one consistent version. + // `load_payload` may run several times when it races with a writer, so it + // must be cheap and side-effect free: only read the shared payload and + // return a copy, never mutate observable state. It must also access the + // shared payload without causing a C++ data race (relaxed atomics). + // + // The callback MUST return an owning value (a copy of the data), never a + // pointer, reference, or view (string_view, span, reference_wrapper, ...) + // into the payload. The consistency guarantee covers only the bytes copied + // out before the validating load: once load() returns, a later writer may + // mutate the payload, so any handle that still points into it no longer + // refers to a consistent snapshot. + template <typename Load> + typename std::decay<decltype(std::declval<Load&>()())>::type + load(Load&& load_payload) const { + typedef typename std::decay<decltype(std::declval<Load&>()())>::type Result; + static_assert(!std::is_void<Result>::value, + "SeqCounter load callback must return a value"); + static_assert(!std::is_pointer<Result>::value, + "SeqCounter load callback must return an owning value, not " + "a pointer into the payload: the pointee can be mutated by " + "a later writer, so it is not a consistent snapshot"); + static_assert(!IsReferenceWrapper<Result>::value, + "SeqCounter load callback must return an owning value, not " + "a std::reference_wrapper into the payload: the referent " + "can be mutated by a later writer, so it is not a " + "consistent snapshot"); + + while (true) { + // Wait for any active writer, then read on an even sequence. + uint64_t seq; + while ((seq = _seq.load(memory_order_acquire)) & 1) { + cpu_relax(); + } + + Result result = load_payload(); + + // Keep the payload reads above before the sequence validation + // below; retry if a writer intervened. + atomic_thread_fence(memory_order_acquire); + if (_seq.load(memory_order_relaxed) == seq) { + return result; + } + } + } + + // Invoke a payload writer inside one write section. Writers must be serialized + // externally and must not nest write sections. The section is closed via RAII + // even if `store_payload` throws, so an exception does not leave the sequence + // permanently odd. A throwing writer may leave the payload partially updated, + // so callers must ensure any partial state is still safe (non-crashing) to read. + template <typename Store> + void store(Store&& store_payload) { + WriteGuard guard(*this); + store_payload(); + } + +private: + // RAII scope for a write section. + class WriteGuard { + public: + explicit WriteGuard(SeqCounter& sc) + : _seq_counter(&sc) + , _next_seq(sc._seq.load(memory_order_relaxed) + 2) { + // Writers are serialized, so no atomic read-modify-write is needed. + // Keep the next even sequence for publication when the guard exits. + _seq_counter->_seq.store(_next_seq - 1, memory_order_relaxed); + // Order the odd-sequence store before the payload stores that + // follow. A release fence is a StoreStore (+LoadStore) barrier: it + // keeps any store after the fence (the payload writes) from being + // reordered ahead of stores before it (the odd sequence), so no + // reader can observe a payload write without also observing the odd + // sequence. Equivalently, this release fence pairs -- through the + // payload atomics -- with the acquire fence in load(): if a reader + // reads a payload value published after this fence, that fence + // synchronizes-with the reader's acquire fence, so the reader is + // guaranteed to also see the odd sequence on its validating load + // and retry. The acquire half of an acq_rel fence would add + // nothing here (no prior load needs ordering), so release alone is + // sufficient and states the intent precisely. + atomic_thread_fence(memory_order_release); + } + + DISALLOW_COPY_AND_ASSIGN(WriteGuard); + + ~WriteGuard() { + // Publish the payload stores and leave the write section. + _seq_counter->_seq.store(_next_seq, memory_order_release); + } + + private: + SeqCounter* _seq_counter; + uint64_t _next_seq; + }; + + atomic<uint64_t> _seq; Review Comment: I agree that C++ does not guarantee lock-free 64-bit atomics on every target. However, an internally locked atomic implementation would affect performance and blocking behavior, not the correctness of the sequence-validation algorithm. I would prefer to retain the [64-bit counter](http://localhost:59097/api/v1/resource/extension/i-1789110347696-10081929842798143/3/Library/Application%20Support/JetBrains/CLion2025.3/plugins/codewiz-idea-plugin/server_lib/.rcs-buildin/rednote-codewiz/0.10.2/src/butil/synchronization/seqlock.h:153). Each write advances it by two, so a 32-bit counter repeats after 2³¹ writes—about 36 minutes at one million writes per second. A reader suspended across a full cycle could incorrectly accept a snapshot. Keeping 64 bits makes that window vastly larger; the algorithm still assumes that no individual read attempt spans a full counter cycle. This is not a claim of universally lock-free atomic operations, nor of lock-free progress for the overall read operation. The absence of internal locking depends on the target’s atomic implementation, including the payload atomics. The current implementation does not enforce that property, so the “no mutex” wording should be understood as referring to the seqlock’s explicit reader-side synchronization, not as a portable guarantee about the underlying atomic library. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
