cache: Poll dying connection pools asynchronously

[cartoush]

This PR moves the blocking sleep loop to a CLI hook.

See #4064

[nigoroll]

@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

[dridi]

There is room for improvement but this is a good start!

[dridi]

LGTM, with nitpicks.

[dridi]

I just realized splitting this operation brings it in two different contexts, see comments. I have not thought much about how to proceed so my questions are open questions.

[nigoroll]

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 to dead_pools under the mtx.
Alternatively, why do we not just hand dead pool handling to a worker thread?

edit: this facility has a mascot

[dridi]

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 to dead_pools under 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?

[nigoroll]

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.

[cartoush]

Added a dead_pools_mtx instead of using conn_pools_mtx

[dridi]

I think I'm OK with this now, but I'm running out of brain cycles.

[cartoush]

calls to vcp_free had not been turned into calls to vcp_destroy, its fixed now

[dridi]

Reconciling the desire to loop without holding the lock with the reuse of the VRBT_ENTRY() field already present in struct conn_pool is complicating the change above my acceptable threshold.

If we put the cursor on the lock, then we should probably have a tail queue of dead connections as suggested before. We keep constant-time operations, including the ability to easily steal and reinject dead pools before and after the cleanup loop.

@nigoroll thoughts?

[dridi]

I don't think you can safely do that, it should look something like this instead:

dead = dead_pools;
VRBT_INIT(&dead_pools);

[nigoroll]

Yeah, the TAKE_OBJ is a smart idea, but using the tree macros as intended would be my preference also.

[dridi]

VRBT_INSERT() inserts a single entry, not another tree, or does it?

[cartoush]

From what I understood a single entry is a tree with only 1 element, so inserting the root of a tree should insert the whole tree but I could be wrong

[nigoroll]

this is actually pretty smart, IMHO, and I did not think of it: I originally thought we would iterate the leftovers and insert one by one, but as we do not care about ordering in the dead_pools tree, this should work.

[nigoroll]

I checked the tree macros and indeed, the comparison function is only used for insert and lookup (as expected). So yeah, I think this is still nicer than the alternative of using a union with list pointers in place of the tree pointers.

[dridi]

I wasn't suggesting a union, but if this is safe but "incorrect", we should add red tape to remind our future selves why this is good enough.

[nigoroll]

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.

[dridi]

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.

[cartoush]

Fixed comments and added comments to the VRBT_INSERT hack

[dridi]

Good job!

[nigoroll]

@bsdphk two people think this one is ripe