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

[Oleg Nesterov <oleg@xxxxxxxxxx>]

On 12/26, Oleg Nesterov wrote:
>
> On 12/26, Linus Torvalds wrote:
> >
> > So the optimization may be valid
>
> I don't think so, see my initial reply.
>
> unlike wait_event(), __pollwait() + the head/tail checks in pipe_poll()
> doesn't have the necessary barriers (at least in theory) afaics. Between
> add_wait_queue()->list_add() and LOAD(head/tail).

Hmm...

Even if we add the wq_has_sleeper() check, the "wake up" logic would
be still suboptimal. Lets forget this patch for the moment.

Consider

	int main(void)
	{
		int fd[2], cnt;
		char c;

		pipe(fd);

		if (!fork()) {
			// wait until the parent blocks in pipe_write() ->
			// wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe));
			sleep(1);

			for (cnt = 0; cnt < 4096; ++cnt)
				read(fd[0], &c, 1);
			return 0;
		}

		// parent
		for (;;)
			write(fd[1], &c, 1);
	}

In this case the child will wakeup the parent 4095 times for no reason,
pipe_writable() == !pipe_pull() will still be true until the last
read(fd[0], &c, 1) does

	if (!buf->len)
		tail = pipe_update_tail(pipe, buf, tail);

and after that the parent can write the next char.

Or did I completely misread this code??

Oleg.