On Thu, May 19, 2016 at 06:54:39PM -0700, Daniele Di Proietto wrote:
> Hi Flavio,
>
> thanks for the patch, it looks good to me
Don't apply this yet. Although I could not see any issues
during 24 hours testing, Karl seems to have found a spike
which I would like to investigate further.
> Some minor comments inline
OK
> 2016-05-04 11:49 GMT-07:00 Flavio Leitner <f...@redhat.com>:
>
> > The PMD thread needs to keep processing RX queues in order
> > to achieve maximum throughput. It also needs to sweep emc
> > cache and quiesce which use seq_mutex. That mutex can
> > eventually block the PMD thread causing latency spikes and
> > affecting the throughput.
> >
> > Since there is no requirement for running those tasks at a
> > specific time, this patch extend seq API to allow tentative
> > locking instead.
> >
> > Reported-by: Karl Rister <kris...@redhat.com>
> > Signed-off-by: Flavio Leitner <f...@redhat.com>
> > ---
> > lib/dpif-netdev.c | 5 +++--
> > lib/ovs-rcu.c | 36 ++++++++++++++++++++++++++++++++++--
> > lib/ovs-rcu.h | 1 +
> > lib/seq.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
> > lib/seq.h | 5 +++++
> > 5 files changed, 89 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 1e8a37c..cab3058 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2698,9 +2698,10 @@ reload:
> >
> > lc = 0;
> >
> > - emc_cache_slow_sweep(&pmd->flow_cache);
> > coverage_try_clear();
> > - ovsrcu_quiesce();
> > + if (!ovsrcu_try_quiesce()) {
> > + emc_cache_slow_sweep(&pmd->flow_cache);
> > + }
> >
>
> > atomic_read_relaxed(&pmd->change_seq, &seq);
> > if (seq != port_seq) {
> > diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
> > index 0ff508e..4ec3dc0 100644
> > --- a/lib/ovs-rcu.c
> > +++ b/lib/ovs-rcu.c
> > @@ -57,6 +57,7 @@ static struct guarded_list flushed_cbsets;
> > static struct seq *flushed_cbsets_seq;
> >
> > static void ovsrcu_init_module(void);
> > +static void ovsrcu_flush_cbset__(struct ovsrcu_perthread *, bool);
> > static void ovsrcu_flush_cbset(struct ovsrcu_perthread *);
> > static void ovsrcu_unregister__(struct ovsrcu_perthread *);
> > static bool ovsrcu_call_postponed(void);
> > @@ -151,6 +152,27 @@ ovsrcu_quiesce(void)
> > ovsrcu_quiesced();
> > }
> >
> > +int
> > +ovsrcu_try_quiesce(void)
> > +{
> > + struct ovsrcu_perthread *perthread;
> > + int ret = -1;
> > +
> > + ovs_assert(!single_threaded());
> >
>
> I'm curious: why is the above necessary?
Because ovsrcu_quiesced() will run RCU callbacks for single
thread and I cannot guarantee that these calls won't use
seq lock.
> > + perthread = ovsrcu_perthread_get();
> > + if (!seq_try_lock()) {
> > + perthread->seqno = seq_read_protected(global_seqno);
> > + if (perthread->cbset) {
> > + ovsrcu_flush_cbset__(perthread, true);
> > + }
> > + seq_change_protected(global_seqno);
> > + seq_unlock();
> > + ovsrcu_quiesced();
> > + ret = 0;
> > + }
> > + return ret;
> > +}
> > +
> > bool
> > ovsrcu_is_quiescent(void)
> > {
> > @@ -292,7 +314,7 @@ ovsrcu_postpone_thread(void *arg OVS_UNUSED)
> > }
> >
> > static void
> > -ovsrcu_flush_cbset(struct ovsrcu_perthread *perthread)
> > +ovsrcu_flush_cbset__(struct ovsrcu_perthread *perthread, bool protected)
> > {
> > struct ovsrcu_cbset *cbset = perthread->cbset;
> >
> > @@ -300,11 +322,21 @@ ovsrcu_flush_cbset(struct ovsrcu_perthread
> > *perthread)
> > guarded_list_push_back(&flushed_cbsets, &cbset->list_node,
> > SIZE_MAX);
> > perthread->cbset = NULL;
> >
> > - seq_change(flushed_cbsets_seq);
> > + if (protected) {
> > + seq_change_protected(flushed_cbsets_seq);
> > + } else {
> > + seq_change(flushed_cbsets_seq);
> > + }
> > }
> > }
> >
> > static void
> > +ovsrcu_flush_cbset(struct ovsrcu_perthread *perthread)
> > +{
> > + ovsrcu_flush_cbset__(perthread, false);
> > +}
> > +
> > +static void
> > ovsrcu_unregister__(struct ovsrcu_perthread *perthread)
> > {
> > if (perthread->cbset) {
> > diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h
> > index 600be0b..e4dcd7d 100644
> > --- a/lib/ovs-rcu.h
> > +++ b/lib/ovs-rcu.h
> > @@ -218,6 +218,7 @@ void ovsrcu_postpone__(void (*function)(void *aux),
> > void *aux);
> > void ovsrcu_quiesce_start(void);
> > void ovsrcu_quiesce_end(void);
> > void ovsrcu_quiesce(void);
> > +int ovsrcu_try_quiesce(void);
> > bool ovsrcu_is_quiescent(void);
> >
> > /* Synchronization. Waits for all non-quiescent threads to quiesce at
> > least
> > diff --git a/lib/seq.c b/lib/seq.c
> > index 57e0775..9abebdb 100644
> > --- a/lib/seq.c
> > +++ b/lib/seq.c
> > @@ -100,6 +100,38 @@ seq_destroy(struct seq *seq)
> > ovs_mutex_unlock(&seq_mutex);
> > }
> >
> > +int
> > +seq_try_lock(void)
> > +{
> > + return ovs_mutex_trylock(&seq_mutex);
> > +}
> > +
> > +void
> > +seq_lock(void)
> > + OVS_EXCLUDED(seq_mutex)
> >
>
> My clang (3.5) complains about this
Works with 3.7.0 :-)
> OVS_ACQUIRES appear to work
That's certainly better, I will fix it.
> > +{
> > + ovs_mutex_lock(&seq_mutex);
> > +}
> > +
> > +void
> > +seq_unlock(void)
> > + OVS_REQUIRES(seq_mutex)
> >
>
> My clang (3.5) complains about this
>
> OVS_RELEASES appear to work
Same here.
Thanks for your feedback!
fbl
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
