On 28 Mar 2023, at 19:32, Mike Pattrick wrote:
> On Mon, Mar 27, 2023 at 7:25 AM Eelco Chaudron <[email protected]> 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 <[email protected]> >> --- >> 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); > > I think it's also possible to get rid of the mutex here. Operations > like modifying a bridge or interface can result in calling seq_create > dozens of times, which could potentially lead to contention in cases > when a lot of interfaces are constantly added or removed. I'm mainly > thinking about kubernetes here. > > Other than that, I found this patch to reduce locking on seq_mutex > pretty significantly with a userspace workload: > > Before: 468727.2/sec > After: 229265.8/sec Yes I just counted syscalls, and I noticed a Hughe drop in futex calls especially if you have more PMD threads. > The rest of this patch looks good to me, but what do you think about: At first glance, it looks good to me however not sure if seq_create() is used often enough to see any gains. But removing a mutex in a function sounds good to me, so I would suggest submitting a patch. > diff --git a/lib/seq.c b/lib/seq.c > index 22646f3f8..09fdccce5 100644 > --- a/lib/seq.c > +++ b/lib/seq.c > @@ -57,7 +57,7 @@ struct seq_thread { > > static struct ovs_mutex seq_mutex = OVS_MUTEX_INITIALIZER; > > -static uint64_t seq_next OVS_GUARDED_BY(seq_mutex) = 1; > +static atomic_uint64_t seq_next = 1; I think we can keep this and use the OVS_EXCLUDED(seq_mutex) macro in the function below. > static pthread_key_t seq_thread_key; > > @@ -81,11 +81,9 @@ seq_create(void) > > COVERAGE_INC(seq_change); > > - ovs_mutex_lock(&seq_mutex); > - seq_value = seq_next++; > + seq_value = atomic_count_inc64(&seq_next); > atomic_store_relaxed(&seq->value, seq_value); > hmap_init(&seq->waiters); > - ovs_mutex_unlock(&seq_mutex); > > return seq; > } > @@ -128,7 +126,7 @@ void > seq_change_protected(struct seq *seq) > OVS_REQUIRES(seq_mutex) > { > - uint64_t seq_value = seq_next++; > + uint64_t seq_value = atomic_count_inc64(&seq_next); > > COVERAGE_INC(seq_change); _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
