[6.1] 206e81918 explain a relevant detail of the worker thread signaling

[hermunn]

commit 206e81918a4fcdb80e3a38416fc7fb060823b9f3
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Thu Sep 27 10:27:44 2018 +0200

    explain a relevant detail of the worker thread signaling
    
    Over time, I have repeatedly stared at this code again and again
    wondering if (and why) our cv signaling is correct, just to end up
    with the same insight each time (but first overlooking #2719)
    
    Being fully aware that we do not want to plaster our code with
    outdated comments, I hope this explanation is warranted to save myself
    (and others, hopefully) from wasting precious life time on reiterating
    over the same question.

diff --git a/bin/varnishd/cache/cache_wrk.c b/bin/varnishd/cache/cache_wrk.c
index 7cd02533d..88a0f0935 100644
--- a/bin/varnishd/cache/cache_wrk.c
+++ b/bin/varnishd/cache/cache_wrk.c
@@ -27,6 +27,27 @@
  * SUCH DAMAGE.
  *
  * Worker thread stuff unrelated to the worker thread pools.
+ *
+ * --
+ * signaling_note:
+ *
+ * note on worker wakeup signaling through the wrk condition variable (cv)
+ *
+ * In the general case, a cv needs to be signaled while holding the
+ * corresponding mutex, otherwise the signal may be posted before the waiting
+ * thread could register itself on the cv and, consequently, the signal may be
+ * missed.
+ *
+ * In our case, any worker thread which we wake up comes from the idle queue,
+ * where it put itself under the mutex, releasing that mutex implicitly via
+ * Lck_CondWait() (which calls some variant of pthread_cond_wait). So we avoid
+ * additional mutex contention knowing that any worker thread on the idle queue
+ * is blocking on the cv.
+ *
+ * Except -- when it isn't, because it woke up for releasing its VCL
+ * Reference. To account for this case, we check if the task function has been
+ * set in the meantime, which in turn requires all of the task preparation to be
+ * done holding the pool mutex. (see also #2719)
  */
 
 #include "config.h"
@@ -220,6 +241,7 @@ Pool_Task_Arg(struct worker *wrk, enum task_prio prio, task_func_t *func,
 	wrk2->task.func = func;
 	wrk2->task.priv = wrk2->aws->f;
 	Lck_Unlock(&pp->mtx);
+	// see signaling_note at the top for explanation
 	if (retval)
 		AZ(pthread_cond_signal(&wrk2->cond));
 	return (retval);
@@ -252,6 +274,7 @@ Pool_Task(struct pool *pp, struct pool_task *task, enum task_prio prio)
 		wrk->task.func = task->func;
 		wrk->task.priv = task->priv;
 		Lck_Unlock(&pp->mtx);
+		// see signaling_note at the top for explanation
 		AZ(pthread_cond_signal(&wrk->cond));
 		return (0);
 	}
@@ -346,6 +369,7 @@ Pool_Work_Thread(struct pool *pp, struct worker *wrk)
 			VTAILQ_INSERT_HEAD(&pp->idle_queue, &wrk->task, list);
 			pp->nidle++;
 			do {
+				// see signaling_note at the top for explanation
 				i = Lck_CondWait(&wrk->cond, &pp->mtx,
 				    wrk->vcl == NULL ?  0 : wrk->lastused+60.);
 				if (i == ETIMEDOUT)