diff mbox series

[ovs-dev,v2] seq: Make read of the current value atomic

Message ID 168597229126.1652700.11280515282374459957.stgit@ebuild
State Accepted
Commit e3ba0be48ca457ab3a1c9f1e3522e82218eca0f9
Headers show
Series [ovs-dev,v2] seq: Make read of the current value atomic | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Eelco Chaudron June 5, 2023, 1:39 p.m. UTC 
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 <echaudro@redhat.com>
---
v2: Update acquire and release semantics for seq->value.

 lib/ovs-rcu.c |    2 +-
 lib/seq.c     |   32 +++++++++++---------------------
 lib/seq.h     |    1 -
 3 files changed, 12 insertions(+), 23 deletions(-)

Comments

Simon Horman June 8, 2023, 2:48 p.m. UTC | #1 
On Mon, Jun 05, 2023 at 03:39:02PM +0200, 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 <echaudro@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Ilya Maximets June 12, 2023, 9:40 p.m. UTC | #2 
On 6/8/23 16:48, Simon Horman wrote:
> On Mon, Jun 05, 2023 at 03:39:02PM +0200, 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 <echaudro@redhat.com>
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 

Applied.  Thanks!

Best regards, Ilya Maximets.
diff mbox series

Patch

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..7c2fa0c69 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_explicit(&seq->value, seq_value, memory_order_release);
     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,12 @@  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);
-
+    /* 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;
 }
 
@@ -226,7 +216,7 @@  seq_wait_at(const struct seq *seq_, uint64_t value, const char *where)
     struct seq *seq = CONST_CAST(struct seq *, seq_);
 
     ovs_mutex_lock(&seq_mutex);
-    if (value == seq->value) {
+    if (value == seq_read(seq_)) {
         seq_wait__(seq, value, where);
     } else {
         poll_immediate_wake_at(where);
diff --git a/lib/seq.h b/lib/seq.h
index c88b9d1c8..fcfa01037 100644
--- a/lib/seq.h
+++ b/lib/seq.h
@@ -128,7 +128,6 @@  void seq_unlock(void);
 
 /* For observers. */
 uint64_t seq_read(const struct seq *);
-uint64_t seq_read_protected(const struct seq *);
 
 void seq_wait_at(const struct seq *, uint64_t value, const char *where);
 #define seq_wait(seq, value) seq_wait_at(seq, value, OVS_SOURCE_LOCATOR)