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
