reserve a fraction of idle threads for each priority class

[nigoroll]

Previously, we used a minimum number of idle threads (the reserve) to ensure that we do not assign all threads with client requests and no threads left over for backend requests.

This was actually only a special case of the more general issue exposed by h2: Lower priority tasks depend on higher priority tasks (for h2, sessions need streams, which need requests, which may need backend requests).

To solve this situation, we divide the reserve by the number of priority classes and schedule lower priority tasks only if there are enough idle threads to run higher priority tasks eventually.

This change does not guarantee any upper limit on the amount of time it can take for a task to be scheduled (e.g. backend requests could be blocking on arbitrarily long timeouts), so the thread pool watchdog is still warranted. But this change should guarantee that we do make progress eventually.

[nigoroll]

Maybe I could have chosen a cleaner wording: The reserve is proportional to the priority, so TASK_QUEUE_BO can use all of the reserve, TASK_QUEUE_RUSH 4/5th, TASK_QUEUE_REQ 3/5th etc.

[nigoroll]

bugwash conclusion: phk to ponder a bit longer

[nigoroll]

rebased, force pushed

[bsdphk]

Bugwash generally in favour.

@dridi to give it a whirl and feedback

should be fixed

[dridi]

I managed to salvage r2418.vtc from the grave, following my intuition that the new reserve logic would not remove the deadlock.

Show test case

varnishtest "h2 queuing deadlock"

barrier b1 cond 2

# dridi> I didn't work out exactly how I tickled the watchdog, because unlike
# the current reserve system on the master branch I run into a deadlock before
# before even involving a 3rd client. Analysis for later, I was sidetracked by
# a bug (049ce0e73) in varnishtest.

varnish v1 -cliok "param.set thread_pools 1"
varnish v1 -cliok "param.set thread_pool_min 5"
varnish v1 -cliok "param.set thread_pool_max 5"
varnish v1 -cliok "param.set thread_pool_reserve 0"
varnish v1 -cliok "param.set thread_pool_watchdog 2"
varnish v1 -cliok "param.set feature +http2"
varnish v1 -cliok "param.set feature +no_coredump"

varnish v1 -vcl {
	backend be none;
	sub vcl_recv {
		return (synth(200));
	}
} -start

logexpect l1 -v v1 -g raw -q "vxid == 0" {
	expect * 0	Error	"Pool Herder: Queue does not move"
} -start

client c1 {
	txpri
	stream 0 rxsettings -run

	barrier b1 sync

	stream 1 {
		txreq
		# dridi> not sure how relevant this still is
	} -run
} -start

client c2 {
	barrier b1 sync
	txpri
	delay 3
} -start

client c1 -wait
client c2 -wait

logexpect l1 -wait

varnish v1 -cliok panic.show
varnish v1 -cliok panic.clear

varnish v1 -expectexit 0x20

[daghf]

I'm no longer able to deadlock Varnish with this patch set in place. Good job :-)

@dridi the test case does not deadlock, the reason we trigger the watchdog is due to the delay 3 being longer than the watchdog timeout.

I wonder if we might still have a theoretical deadlock possibility for h/2 via Upgrade: h2c, where the upgraded request gets dispatched as a stream with prio TASK_QUEUE_REQ in h2_ou_session(). We should probably turn this into TASK_QUEUE_STR.

[dridi]

Thank you @daghf for your analysis, when I produced the test case I wasn't sure what was happening yet and when I tried to analyze what was going on I couldn't focus properly.

Besides your suggestion, do you have an idea on how to fix the spurious watchdog trigger? This is caused by a client, so this is something we should either fix as part of this pull request or soon after if we decide to merge it in its current form.

But this is not something we should allow to fall on the back burner.

[nigoroll]

@daghf a big thank you also from my side. As @dridi has passed the maintainer hat on to you for this one, would you mind leaving an approval review?

[dridi]

I think I managed to reproduce the h2 panic in #3142 so there is nothing holding my approval back, except maybe that you might as well squash squashme commits when you rebase against master.

[daghf]

LGTM

[nigoroll]

review session with @bsdphk and @dridi :

• rename TASK_QUEUE_END -> TASK_QUEUE__END
• add an assert thread_pool_min.min is >= TASK_QUEUE__END

OK otherwise

[nigoroll]

FTR:

add an assert thread_pool_min.min is >= TASK_QUEUE__END

from the commit message:

the respective places are marked and an assert is in place to ensure we do not start a worker with too small a number of workers. I decided against checks in the manager to avoid include pollution from the worker (cache.h) into the manager.