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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241227155428.GA15300@redhat.com>
Date: Fri, 27 Dec 2024 16:54:28 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: WangYuli <wangyuli@...ontech.com>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Christian Brauner <brauner@...nel.org>
Subject: Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping
 processes during pipe read/write

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.