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

[Jarno Rajahalme]

Thanks for the review. I sent a v2 with an implementation that just does the right thing (I hope) in the revalidator thread itself.

  Jarno

> On Sep 19, 2016, at 10:32 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> 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.