[ovs-dev] [PATCH 3/3] seq: Add support for initial delay before waking up.

[Ben Pfaff]

On Fri, Sep 16, 2016 at 04:10:51PM -0700, Jarno Rajahalme wrote:
> The execution time of 'ovs-ofctl add-flows' with a large number of
> flows can be more than halved if revalidators are not running after
> each flow mod separately.  This was first suspected when it was found
> that 'ovs-ofctl --bundle add-flows' is about 10 times faster than the
> same command without the '--bundle' option in a scenario where there
> is a large set of flows being added and no datapath flows at all.  One
> of the differences caused by the '--bundle' option is that the
> revalidators are woken up only once, at the end of the whole set of
> flow table changes, rather than after each flow table change
> individually.
> 
> This patch limits the revalidation to run at most 200 times a second
> by enforcing a minimum of 5ms delay for flow table change wakeup after
> each complete revalidation round.  This is implemented by adding a new
> seq_wait_delay() function, that takes a delay parameter, which, when
> non-zero, causes the wakeup to be postponed by the given number of
> milliseconds from the original seq_wait_delay() call time.  If nothing
> happens in, say 6 milliseconds, and then a new flow table change is
> signaled, the revalidator threads wake up immediately without any
> further delay.  Values smaller than 5 were found to increase the
> 'ovs-ofctl add-flows' execution time noticeably.
> 
> Since the revalidators are not running after each flow mod, the
> overall OVS CPU utilization during the 'ovs-ofctl add-flows' run time
> is reduced roughly by one core on a four core machine.
> 
> In testing the 'ovs-ofctl add-flows' execution time is not
> significantly improved from this even if the revalidators are not
> notified about the flow table changes at all.
> 
> Signed-off-by: Jarno Rajahalme <jarno at ovn.org>

I need some help understanding how this works.  Before this commit, a
thread that wanted to wake up if a seq changed would call seq_wait().
If the seq changed, it got awakened immediately because seq_change()
called seq_wake_waiters() which set a latch that woke the thread.  After
this commit, that still works fine with delay==0.  However suppose that
delay==5 instead.  As I see it, the same chain of events will occur
except that seq_wake_waiters() might do nothing because the time hasn't
come yet.  Suppose that nothing ever calls seq_change() again for that
seq.  What wakes up the thread after 5 ms?

Once I understand that, here's a possible nit to look into.  A couple of
places talk about how time_msec() has to be called without holding
seq_mutex because time_msec() calls time_init(), which creates a seq,
which takes seq_mutex.  Maybe this is good enough.  But it also appears
to be easy to eliminate the dependency of seq_create() on seq_mutex.
Here is why it takes seq_mutex:

    ovs_mutex_lock(&seq_mutex);
    seq->value = seq_next++;
    hmap_init(&seq->waiters);
    ovs_mutex_unlock(&seq_mutex);

The first statement is in seq_mutex because seq_next requires it.  This
could be fixed by make seq_next atomic and then using atomic_add()
on it.  The second statement is in seq_mutex only to silence Clang; an
OVS_NO_THREAD_SAFETY_ANALYSIS annotation would also silence it.

Thanks,

Ben.