pipe: fix thundering herd problem in pipelock

[mjg]

Summary

All reads and writes are serialized with a hand-rolled lock, but unlocking it always wakes up all waiters. Existing flag fields get resized to make room for introduction of waiter counter without growing the struct.

I don't see an easy way to get rid of pipelock or convert into into an actual lock.

[markj]

In what situation is there a large number of threads blocked on the pipe lock?

sys/kern/sys_pipe.c

626 ↗

(On Diff #79743)

No newlines in kassert messages, ditto below.

[kib]

BTW why pipelock() is not sx ?

[mjg]

• newlines

[mjg]

In D27273#609320, @markj wrote:

In what situation is there a large number of threads blocked on the pipe lock?

For example make -j $BIGNUM. There are tons of processes all polling the same pipe for "tokens" and then reading.

BTW why pipelock() is not sx ?

As in why not convert pipelock to sx as it is? There are lock ordering issues against PIPE_LOCK which is a mutex held and dropped around calls to pipelock. Converting that to sx as well so that the code can wait for pipelock with PIPE_LOCK taken runs into other problems -- for example right now there is a larger window where it is possible to poll the pipe despite other ops running on it, and the lock taken with PIPE_LOCK is heavily contested there.

So a straight up conversion without reworking the locking scheme would regress things here. If going for serious effort someone should implement using the direct map to back pipe buffers and implement new locking with that in mind.

[kib]

I see that PIPE_LOCK is simply interlocked with PIPE_LOCKFL, then why cannot we just replace msleep() with sx_sleep() if pipelock() becomes sx ?

[mjg]

Not sure I follow.

In pipe_read the operation is PIPE_LOCK ; pipelock which relocks PIPE_LOCK and the read loop which performs:

 PIPE_UNLOCK(rpipe);
error = uiomove(
    &rpipe->pipe_buffer.buffer[rpipe->pipe_buffer.out],
    size, uio);
PIPE_LOCK(rpipe);

This allows pipe_poll to proceed while data is moved around, despite pipelock being taken. I don't think anything invasive locking-wiase is warranted without implementing direct map support.