Hi Flavio and Karl, thanks for the patch! I have a couple of comments:
Can you point out a configuration where this is the bottleneck? I'm interested in reproducing this. I think the implementation would look simpler if we could avoid explicitly taking the mutex in dpif-netdev and instead having a ovsrcu_try_quiesce(). What do you think? I think we can avoid the recursive mutex as well if we introduce some explicit APIs in seq (seq_try_lock, seq_change_protected and seq_unlock), but I'd like to understand the performance implication of this commit first. On 23/03/2016 20:54, "dev on behalf of Flavio Leitner" <dev-boun...@openvswitch.org on behalf of f...@redhat.com> wrote: >The PMD thread needs to keep processing RX queues in order >archive maximum throughput. However, it also needs to run >other tasks after some time processing the RX queues which >a mutex can block the PMD thread. That causes latency >spikes and affects the throughput. > >Convert to recursive mutex so that PMD thread can test first >and if it gets the lock, continue as before, otherwise try >again another time. There is an additional logic to make >sure the PMD thread will try harder as the attempt to get >the mutex continues to fail. > >Co-authored-by: Karl Rister <kris...@redhat.com> >Signed-off-by: Flavio Leitner <f...@redhat.com> Oh, we're going to need a signoff from Karl as well :-) Thanks, Daniele >--- > include/openvswitch/thread.h | 3 +++ > lib/dpif-netdev.c | 33 ++++++++++++++++++++++----------- > lib/seq.c | 15 ++++++++++++++- > lib/seq.h | 3 +++ > 4 files changed, 42 insertions(+), 12 deletions(-) > >diff --git a/include/openvswitch/thread.h b/include/openvswitch/thread.h >index af6f2bb..6d20720 100644 >--- a/include/openvswitch/thread.h >+++ b/include/openvswitch/thread.h >@@ -44,6 +44,9 @@ struct OVS_LOCKABLE ovs_mutex { > #define OVS_ADAPTIVE_MUTEX_INITIALIZER OVS_MUTEX_INITIALIZER > #endif > >+#define OVS_RECURSIVE_MUTEX_INITIALIZER \ >+ { PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP, "<unlocked>" } >+ > /* ovs_mutex functions analogous to pthread_mutex_*() functions. > * > * Most of these functions abort the process with an error message on any >diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >index 0f2385a..a10b2d1 100644 >--- a/lib/dpif-netdev.c >+++ b/lib/dpif-netdev.c >@@ -2668,12 +2668,15 @@ static void * > pmd_thread_main(void *f_) > { > struct dp_netdev_pmd_thread *pmd = f_; >- unsigned int lc = 0; >+ unsigned int lc_max = 1024; >+ unsigned int lc_start; >+ unsigned int lc; > struct rxq_poll *poll_list; > unsigned int port_seq = PMD_INITIAL_SEQ; > int poll_cnt; > int i; > >+ lc_start = 0; > poll_cnt = 0; > poll_list = NULL; > >@@ -2698,24 +2701,32 @@ reload: > * reloading the updated configuration. */ > dp_netdev_pmd_reload_done(pmd); > >+ lc = lc_start; > for (;;) { > for (i = 0; i < poll_cnt; i++) { > dp_netdev_process_rxq_port(pmd, poll_list[i].port, >poll_list[i].rx); > } > >- if (lc++ > 1024) { >- unsigned int seq; >+ if (lc++ > lc_max) { >+ if (!seq_pmd_trylock()) { >+ unsigned int seq; >+ lc_start = 0; >+ lc = 0; > >- lc = 0; >+ emc_cache_slow_sweep(&pmd->flow_cache); >+ coverage_try_clear(); >+ ovsrcu_quiesce(); > >- emc_cache_slow_sweep(&pmd->flow_cache); >- coverage_try_clear(); >- ovsrcu_quiesce(); >+ seq_pmd_unlock(); > >- atomic_read_relaxed(&pmd->change_seq, &seq); >- if (seq != port_seq) { >- port_seq = seq; >- break; >+ atomic_read_relaxed(&pmd->change_seq, &seq); >+ if (seq != port_seq) { >+ port_seq = seq; >+ break; >+ } >+ } else { >+ lc_start += (lc_start + lc_max)/2; >+ lc = lc_start; > } > } > } >diff --git a/lib/seq.c b/lib/seq.c >index 9c3257c..198b2ce 100644 >--- a/lib/seq.c >+++ b/lib/seq.c >@@ -55,7 +55,7 @@ struct seq_thread { > bool waiting OVS_GUARDED; /* True if latch_wait() already >called. */ > }; > >-static struct ovs_mutex seq_mutex = OVS_MUTEX_INITIALIZER; >+static struct ovs_mutex seq_mutex = OVS_RECURSIVE_MUTEX_INITIALIZER; > > static uint64_t seq_next OVS_GUARDED_BY(seq_mutex) = 1; > >@@ -68,6 +68,19 @@ static void seq_thread_woke(struct seq_thread *) >OVS_REQUIRES(seq_mutex); > static void seq_waiter_destroy(struct seq_waiter *) >OVS_REQUIRES(seq_mutex); > static void seq_wake_waiters(struct seq *) OVS_REQUIRES(seq_mutex); > >+ >+int seq_pmd_trylock(void) >+ OVS_TRY_LOCK(0, seq_mutex) >+{ >+ return ovs_mutex_trylock(&seq_mutex); >+} >+ >+void seq_pmd_unlock(void) >+ OVS_RELEASES(seq_mutex) >+{ >+ ovs_mutex_unlock(&seq_mutex); >+} >+ > /* Creates and returns a new 'seq' object. */ > struct seq * OVS_EXCLUDED(seq_mutex) > seq_create(void) >diff --git a/lib/seq.h b/lib/seq.h >index b0ec6bf..f6b6e1a 100644 >--- a/lib/seq.h >+++ b/lib/seq.h >@@ -117,6 +117,9 @@ > #include <stdint.h> > #include "util.h" > >+int seq_pmd_trylock(void); >+void seq_pmd_unlock(void); >+ > /* For implementation of an object with a sequence number attached. */ > struct seq *seq_create(void); > void seq_destroy(struct seq *); >-- >2.5.0 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev