Quite possibly I missed something, but I have some concerns after
a quick glance...
On 12/25, WangYuli wrote:
>
> +config PIPE_SKIP_SLEEPER
> + bool "Skip sleeping processes during pipe read/write"
> + default n
> + help
> + This option introduces a check whether the sleep queue will
> + be awakened during pipe read/write.
> +
> + It often leads to a performance improvement. However, in
> + low-load or single-task scenarios, it may introduce minor
> + performance overhead.
> +
> + If unsure, say N.
> +
Well, IMO the new config option should be avoided for this optimization.
> +static inline bool
> +pipe_check_wq_has_sleeper(struct wait_queue_head *wq_head)
> +{
> + if (IS_ENABLED(CONFIG_PIPE_SKIP_SLEEPER))
> + return wq_has_sleeper(wq_head);
> + else
> + return true;
> +}
I think that another helper makes more sense:
pipe_wake_xxx(wait_queue_head_t wait, flags)
{
if (wq_has_sleeper(wait))
wake_up_interruptible_sync_poll(wait, flags);
}
-------------------------------------------------------------------------------
But either way I am not sure this optimization is 100% correct, see below.
> @@ -377,7 +386,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
> * _very_ unlikely case that the pipe was full, but we got
> * no data.
> */
> - if (unlikely(was_full))
> + if (unlikely(was_full) && pipe_check_wq_has_sleeper(&pipe->wr_wait))
> wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM);
OK, at first glance this can't race with wait_event_xxx(wr_wait, pipe_writable),
wq_has_sleeper() and prepare_to_wait_event() have the necessary barriers.
But what about pipe_poll() ?
To oversimplify, pipe_poll(FMODE_WRITE) does
// poll_wait()
__pollwait() -> add_wait_queue(pipe->wr_wait) -> list_add()
check pipe_full()
and I don't see the (in theory) necessary barrier between list_add()
and LOAD(pipe->head/tail).
Oleg.
