Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

[Oleg Nesterov <oleg@redhat.com>]

Hi Sapkal,

On 02/24, Sapkal, Swapnil wrote:
>
> We saw hang in hackbench in our weekly regression testing on mainline
> kernel. The bisect pointed to this commit.

OMG. This patch caused a lot of "hackbench performance degradation" reports,
but hang??

Just in case, did you use

	https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git/tree/src/hackbench/hackbench.c

?

OK, I gave up ;) I'll send the revert patch tomorrow (can't do this today)
even if I still don't see how this patch can be wrong.

> Whenever I compare the case where was_full would have been set but
> wake_writer was not set, I see the following pattern:
>
> ret = 100 (Read was successful)
> pipe_full() = 1
> total_len = 0
> buf->len != 0
>
> total_len is computed using iov_iter_count() while the buf->len is the
> length of the buffer corresponding to tail(pipe->bufs[tail & mask].len).
> Looking at pipe_write(), there seems to be a case where the writer can make
> progress when (chars && !was_empty) which only looks at iov_iter_count().
> Could it be the case that there is still room in the buffer but we are not
> waking up the writer?

I don't think so, but perhaps I am totally confused.

If the writer sleeps on pipe->wr_wait, it has already tried to write into
the pipe->bufs[head - 1] buffer before the sleep.

Yes, the reader can read from that buffer, but this won't make it more "writable"
for this particular writer, "PAGE_SIZE - buf->offset + buf->len" won't be changed.
I even wrote the test-case, let me quote my old email below.

Thanks,

Oleg.
--------------------------------------------------------------------------------

Meanwhile I wrote a stupid test-case below.

Without the patch

	State:	S (sleeping)
	voluntary_ctxt_switches:	74
	nonvoluntary_ctxt_switches:	5
	State:	S (sleeping)
	voluntary_ctxt_switches:	4169
	nonvoluntary_ctxt_switches:	5
	finally release the buffer
	wrote next char!

With the patch

	State:	S (sleeping)
	voluntary_ctxt_switches:	74
	nonvoluntary_ctxt_switches:	3
	State:	S (sleeping)
	voluntary_ctxt_switches:	74
	nonvoluntary_ctxt_switches:	3
	finally release the buffer
	wrote next char!

As you can see, without this patch pipe_read() wakes the writer up
4095 times for no reason, the writer burns a bit of CPU and blocks
again after wakeup until the last read(fd[0], &c, 1).

Oleg.

-------------------------------------------------------------------------------
#include <stdlib.h>
#include <unistd.h>
#include <assert.h>
#include <sys/ioctl.h>
#include <stdio.h>
#include <errno.h>

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

	assert(pipe(fd) == 0);

	nb = 1; assert(ioctl(fd[1], FIONBIO, &nb) == 0);
	while (write(fd[1], &c, 1) == 1);
	assert(errno = -EAGAIN);
	nb = 0; assert(ioctl(fd[1], FIONBIO, &nb) == 0);

	// The pipe is full, the next write() will block.

	sprintf(cmd, "grep -e State -e ctxt_switches /proc/%d/status", getpid());

	if (!fork()) {
		// wait until the parent sleeps in pipe_write()
		usleep(10000);

		system(cmd);
		// trigger 4095 unnecessary wakeups
		for (cnt = 0; cnt < 4095; ++cnt) {
			assert(read(fd[0], &c, 1) == 1);
			usleep(1000);
		}
		system(cmd);

		// this should actually wake the writer
		printf("finally release the buffer\n");
		assert(read(fd[0], &c, 1) == 1);
		return 0;
	}

	assert(write(fd[1], &c, 1) == 1);
	printf("wrote next char!\n");

	return 0;
}