On 16 May 2023, at 21:48, Ilya Maximets wrote:
> On 5/16/23 10:20, Eelco Chaudron wrote: >> >> >> On 15 May 2023, at 17:47, Ilya Maximets wrote: >> >>> On 5/15/23 16:24, Eelco Chaudron wrote: >>>> >>>> >>>> On 4 May 2023, at 0:55, Ilya Maximets wrote: >>>> >>>>> On 3/27/23 13:25, Eelco Chaudron wrote: >>>>>> Make the read of the current seq->value atomic, i.e., not needing to >>>>>> acquire the global mutex when reading it. On 64-bit systems, this >>>>>> incurs no overhead, and it will avoid the mutex and potentially >>>>>> a system call. >>>>>> >>>>>> For incrementing the value followed by waking up the threads, we are >>>>>> still taking the mutex, so the current behavior is not changing. The >>>>>> seq_read() behavior is already defined as, "Returns seq's current >>>>>> sequence number (which could change immediately)". So the change >>>>>> should not impact the current behavior. >>>>>> >>>>>> Signed-off-by: Eelco Chaudron <echau...@redhat.com> >>>>>> --- >>>>>> lib/ovs-rcu.c | 2 +- >>>>>> lib/seq.c | 37 ++++++++++++++++--------------------- >>>>>> lib/seq.h | 1 - >>>>>> 3 files changed, 17 insertions(+), 23 deletions(-) >>>>>> >>>>>> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c >>>>>> index 946aa04d1..9e07d9bab 100644 >>>>>> --- a/lib/ovs-rcu.c >>>>>> +++ b/lib/ovs-rcu.c >>>>>> @@ -170,7 +170,7 @@ ovsrcu_try_quiesce(void) >>>>>> ovs_assert(!single_threaded()); >>>>>> perthread = ovsrcu_perthread_get(); >>>>>> if (!seq_try_lock()) { >>>>>> - perthread->seqno = seq_read_protected(global_seqno); >>>>>> + perthread->seqno = seq_read(global_seqno); >>>>>> if (perthread->cbset) { >>>>>> ovsrcu_flush_cbset__(perthread, true); >>>>>> } >>>>>> diff --git a/lib/seq.c b/lib/seq.c >>>>>> index 99e5bf8bd..22646f3f8 100644 >>>>>> --- a/lib/seq.c >>>>>> +++ b/lib/seq.c >>>>>> @@ -32,7 +32,7 @@ COVERAGE_DEFINE(seq_change); >>>>>> >>>>>> /* A sequence number object. */ >>>>>> struct seq { >>>>>> - uint64_t value OVS_GUARDED; >>>>>> + atomic_uint64_t value; >>>>>> struct hmap waiters OVS_GUARDED; /* Contains 'struct seq_waiter's. >>>>>> */ >>>>>> }; >>>>>> >>>>>> @@ -72,6 +72,7 @@ static void seq_wake_waiters(struct seq *) >>>>>> OVS_REQUIRES(seq_mutex); >>>>>> struct seq * OVS_EXCLUDED(seq_mutex) >>>>>> seq_create(void) >>>>>> { >>>>>> + uint64_t seq_value; >>>>>> struct seq *seq; >>>>>> >>>>>> seq_init(); >>>>>> @@ -81,7 +82,8 @@ seq_create(void) >>>>>> COVERAGE_INC(seq_change); >>>>>> >>>>>> ovs_mutex_lock(&seq_mutex); >>>>>> - seq->value = seq_next++; >>>>>> + seq_value = seq_next++; >>>>>> + atomic_store_relaxed(&seq->value, seq_value); >>>>>> hmap_init(&seq->waiters); >>>>>> ovs_mutex_unlock(&seq_mutex); >>>>>> >>>>>> @@ -126,9 +128,11 @@ void >>>>>> seq_change_protected(struct seq *seq) >>>>>> OVS_REQUIRES(seq_mutex) >>>>>> { >>>>>> + uint64_t seq_value = seq_next++; >>>>>> + >>>>>> COVERAGE_INC(seq_change); >>>>>> >>>>>> - seq->value = seq_next++; >>>>>> + atomic_store_relaxed(&seq->value, seq_value); >>>>>> seq_wake_waiters(seq); >>>>>> } >>>>>> >>>>>> @@ -143,18 +147,6 @@ seq_change(struct seq *seq) >>>>>> ovs_mutex_unlock(&seq_mutex); >>>>>> } >>>>>> >>>>>> -/* Returns 'seq''s current sequence number (which could change >>>>>> immediately). >>>>>> - * >>>>>> - * seq_read() and seq_wait() can be used together to yield a race-free >>>>>> wakeup >>>>>> - * when an object changes, even without an ability to lock the object. >>>>>> See >>>>>> - * Usage in seq.h for details. */ >>>>>> -uint64_t >>>>>> -seq_read_protected(const struct seq *seq) >>>>>> - OVS_REQUIRES(seq_mutex) >>>>>> -{ >>>>>> - return seq->value; >>>>>> -} >>>>>> - >>>>>> /* Returns 'seq''s current sequence number (which could change >>>>>> immediately). >>>>>> * >>>>>> * seq_read() and seq_wait() can be used together to yield a race-free >>>>>> wakeup >>>>>> @@ -162,14 +154,15 @@ seq_read_protected(const struct seq *seq) >>>>>> * Usage in seq.h for details. */ >>>>>> uint64_t >>>>>> seq_read(const struct seq *seq) >>>>>> - OVS_EXCLUDED(seq_mutex) >>>>>> { >>>>>> uint64_t value; >>>>>> >>>>>> - ovs_mutex_lock(&seq_mutex); >>>>>> - value = seq_read_protected(seq); >>>>>> - ovs_mutex_unlock(&seq_mutex); >>>>>> - >>>>>> + /* We use atomic_read(), memory_order_seq_cst, to simulate the full >>>>>> + * memory barrier you would get with locking and unlocking the >>>>>> global >>>>>> + * mutex. >>>>> >>>>> Hi, Eelco. I'm not sure this is actually correct. >>>>> >>>>> We're performing memory_order_seq_cst operation on the value. >>>>> >>>>> Sequential consistency means just acquire-release semantics in our case, >>>>> since >>>>> we're pairing with non-seq-cst operations. And acquire-release semantics >>>>> works >>>>> between acquire loads and release stores on the same atomic. But all the >>>>> loads >>>>> on the value we have with this change are relaxed. And the reader >>>>> doesn't take >>>>> the mutex. Meaning there is no acquire-release pair that can synchronize >>>>> the >>>>> memory between threads. memory_order_seq_cst read on its own can't really >>>>> guarantee any synchronization. >>>>> >>>>> Also, the documentation for the sequence number library explicitly says >>>>> that >>>>> """ >>>>> seq_change() synchronizes with seq_read() and >>>>> seq_wait() on the same variable in release-acquire fashion. That >>>>> is, all effects of the memory accesses performed by a thread prior >>>>> to seq_change() are visible to the threads returning from >>>>> seq_read() or seq_wait() observing that change. >>>>> """ >>>>> >>>>> And I don't think that's the case with this change anymore. >>>> >>>> It was more than a year ago I did this patch, so can’t really remember my >>>> reasoning for doing it this way, and thinking it’s fine :) >>>> >>>> I think it might have been the following: >>>> >>>> 1) Before increasing the value in seq_change() there is a full memory >>>> barrier due to taking the lock >>>> 2a) Now if we read the atomic seq in seq_read() and this new value is >>>> incremented in the seq_read above, the memory should be in sync >>>> 2b) If we read it before we increase it, it’s the old value and it should >>>> not matter if the read has happened or not. >>>> >>>> Maybe I miss something obvious, if so let me know. >>> >>> The problem here is not the value of the sequence number, but the >>> fact that pair seq_change() + seq_read() is documented as an >>> acquire-release synchronization pair. Meaning that these functions >>> has to provide memory synchronization on unrelated memory locations. >> >> Yes, that’s what I tried to explain, but looks like I failed ;) I think it’s >> a bit less restricted, as it is only for the sequence number returned by >> seq_read compare to the new sequence number generated by seq_change. >> >>> And we don't have an acquire-release pair of operations that would >>> ensure that, IIUC. It might work with current implementations for >>> well known architectures today, but I don't think it's required for >>> any of these operations to always perform full memory barriers >>> regardless of memory location. >> >> Yes, you are right, I think the memory barrier is not a requirement for the >> mutex_lock(), I guess only for the mutex_unlock(). >> >>> Might be fixed by using release semantics for stores though, I guess. >> >> So something like this could solve it: >> >> void >> seq_change_protected(struct seq *seq) >> OVS_REQUIRES(seq_mutex) >> { >> uint64_t seq_value = seq_next++; >> >> COVERAGE_INC(seq_change); >> >> atomic_thread_fence(memory_order_release); >> atomic_store_relaxed(&seq->value, seq_value); >> seq_wake_waiters(seq); >> } >> >> uint64_t >> seq_read(const struct seq *seq) >> { >> uint64_t value; >> >> /* Note that the odd CONST_CAST() is here to keep sparse happy. */ >> atomic_read_relaxed(&CONST_CAST(struct seq *, seq)->value, &value); >> atomic_thread_fence(memory_order_acquire); >> return value; >> } >> >> What do you think? > > Why not just explicit read/store with acquire-release? I'm not sure why > we need to use a fence. Could you elaborate? I misread the spec, and assumed this order was only for the atomic var itself. Found a nice blog explaining this, https://preshing.com/20120913/acquire-and-release-semantics/, it also had my solution as an option, so at least I was on the right track :) So taking their example the following should be enough (as you suggested :) void seq_change_protected(struct seq *seq) OVS_REQUIRES(seq_mutex) { uint64_t seq_value = seq_next++; COVERAGE_INC(seq_change); atomic_store_explicit(&seq->value, seq_value, memory_order_release); seq_wake_waiters(seq); } uint64_t seq_read(const struct seq *seq) { uint64_t value; /* Note that the odd CONST_CAST() is here to keep sparse happy. */ atomic_read_explicit(&CONST_CAST(struct seq *, seq)->value, &value, memory_order_acquire); return value; } _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev