varnishcache
cache: Poll dying connection pools asynchronously
This PR moves the blocking sleep loop to a CLI hook.
See https://github.com/varnishcache/varnish-cache/pull/4064
@cartoush IIRC your commit messages and pull requests so far all were extremely terse. Can you please elaborate such that reviewers can easily understand which problem you are trying to solve, why you chose a particular approach and, if necessary, how you implemented it?
I recently noticed a guide which I would be tempted to propose to adopt for our project: https://github.com/axboe/liburing/blob/master/CONTRIBUTING.md
As pointed out by others, locking is not correct yet. I think we should just have a single lock for
dead_pools, and insert each dead cp under that lock. VCP_RelPoll (if we keep it like this) can just swap the list head under that mtx, work all entries unlocked, remembering those still active (n_kill > 0) on a new list, and finally appending the new list back todead_poolsunder the mtx.
I guess we could work with a mutex dedicated to the dead pools and not even care about the per-vcp lock. Since we care about observing n_kill == 0 we don't need to formally synchronize with waiters, we can only see n_kill after the waiter woke up for all connections and since this happens in a CLI hook, it will eventually be seen.
Alternatively, why do we not just hand dead pool handling to a worker thread?
Isn't a CLI hook good-enough for this kind of cleanup sweep?
Alternatively, why do we not just hand dead pool handling to a worker thread?
Isn't a CLI hook good-enough for this kind of cleanup sweep?
It probably is, but I do not know, this was just an option to maybe consider.
I would really like to see swapping the tree head and working the cleanup outside any connection pool mtx.
calls to vcp_free had not been turned into calls to vcp_destroy, its fixed now
Except for the one or two nitpicks, I like the structure of the patch now. I do not understand which complication @dridi is seeing, IMHO, this is a simplification.
I do not understand which complication @dridi is seeing, IMHO, this is a simplification.
That was in the optic of not misusing the VRBT API on purpose. I'll comment in that thread.