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 <b...@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 <ja...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
