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