[ovs-dev] [PATCH] ofproto-dpif-upcall: reduce number of wakeup

[Zhou, Han]

Hi,

On Wed, Sep 25, 2013 at 01:38:58PM +0900, YAMAMOTO Takashi wrote:
> if a queue length is long (ie. non-0), the consumer thread should
> already be busy working on the queue.  there's no need to wake it
> up repeatedly.
...
> @@ -530,9 +532,13 @@ recv_upcalls(struct udpif *udpif)
>          ovs_mutex_lock(&handler->mutex);
>          if (handler->n_upcalls < MAX_QUEUE_LENGTH) {
>              list_push_back(&handler->upcalls, &upcall->list_node);
> -            handler->n_new_upcalls = ++handler->n_upcalls;
> -
> -            if (handler->n_new_upcalls >= FLOW_MISS_MAX_BATCH) {
> +            if (handler->n_upcalls == 0) {
> +                handler->need_signal = true;

Wakeup is not needed when n_upcalls == 0, but only when n_upcalls > 0. 
This is not an issue for the "batch" loop, but has problem for the final 
xpthread_cond_signal() at the end of this function:

> @@ -555,8 +561,8 @@ recv_upcalls(struct udpif *udpif)
>      for (n = 0; n < udpif->n_handlers; ++n) {
>          struct handler *handler = &udpif->handlers[n];
> 
> -        if (handler->n_new_upcalls) {
> -            handler->n_new_upcalls = 0;
> +        if (handler->need_signal) {
Here is the problem. need_signal is true even when n_upcalls is 0, and 
Still do the wakeup! I had a test that in massive flow setup scenario the 
miss_handler threads are still woken up by this final wakeup when there 
is only 1 or 2 upcalls, instead of the "batch" wakeup, because dpif_recv() 
returns EAGAIN (i.e., the n_upcalls rarely reach the batch size 50). This 
condition change from "if (handler->n_new_upcalls)" to 
"if (handler->need_signal) just make things worse: there were already 
too frequent wakeups and now more of them when there is no upcalls ...
> +            handler->need_signal = false;
>              ovs_mutex_lock(&handler->mutex);
>              xpthread_cond_signal(&handler->wake_cond);
>              ovs_mutex_unlock(&handler->mutex);

In addition, in miss_handler code it is "if" condition instead of "while" loop 
when checking against the condition n_upcalls for ovs_mutex_cond_wait():
         if (!handler->n_upcalls) {
             ovs_mutex_cond_wait(&handler->wake_cond, &handler->mutex);
         }

So I would like to propose below patch, to avoid unnecessary wakeups and also avoid
wasted handling in miss_handler when no upcalls, and remove the unused var. 
---
ofproto/ofproto-dpif-upcall.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index dde6430..744250c 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -58,7 +58,6 @@ struct handler {
     struct list upcalls OVS_GUARDED;
     size_t n_upcalls OVS_GUARDED;
 
-    size_t n_new_upcalls;              /* Only changed by the dispatcher. */
     bool need_signal;                  /* Only changed by the dispatcher. */
 
     pthread_cond_t wake_cond;          /* Wakes 'thread' while holding
@@ -406,7 +405,7 @@ udpif_upcall_handler(void *arg)
             return NULL;
         }
 
-        if (!handler->n_upcalls) {
+        while (!handler->n_upcalls) {
             ovs_mutex_cond_wait(&handler->wake_cond, &handler->mutex);
         }
 
@@ -533,10 +532,10 @@ recv_upcalls(struct udpif *udpif)
         ovs_mutex_lock(&handler->mutex);
         if (handler->n_upcalls < MAX_QUEUE_LENGTH) {
             list_push_back(&handler->upcalls, &upcall->list_node);
-            if (handler->n_upcalls == 0) {
+            handler->n_upcalls++;
+            if (!handler->need_signal && (handler->n_upcalls > 0)) {
                 handler->need_signal = true;
             }
-            handler->n_upcalls++;
             if (handler->need_signal &&
                 handler->n_upcalls >= FLOW_MISS_MAX_BATCH) {
                 handler->need_signal = false;

---

Best regards,
Han