Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write

[Oleg Nesterov]

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.