pipe: fix and clarify pipe write wakeup logic

The pipe rework ends up having been extra painful, partly becaused of
actual bugs with ordering and caching of the pipe state, but also
because of subtle performance issues.

In particular, the pipe rework caused the kernel build to inexplicably
slow down.

The reason turns out to be that the GNU make jobserver (which limits the
parallelism of the build) uses a pipe to implement a "token" system: a
parallel submake will read a character from the pipe to get the job
token before starting a new job, and will write a character back to the
pipe when it is done. The overall job limit is thus easily controlled
by just writing the appropriate number of initial token characters into
the pipe.

But to work well, that really means that the old behavior of write
wakeups being synchronous (WF_SYNC) is very important - when the pipe
writer wakes up a reader, we want the reader to actually get scheduled
immediately. Otherwise you lose the parallelism of the build.

The pipe rework lost that synchronous wakeup on write, and we had
clearly all forgotten the reasons and rules for it.

This rewrites the pipe write wakeup logic to do the required Wsync
wakeups, but also clarifies the logic and avoids extraneous wakeups.

It also ends up addign a number of comments about what oit does and why,
so that we hopefully don't end up forgetting about this next time we
change this code.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Linus Torvalds 6 years ago 1b6b26ae ad910e36

+41 -18

1 changed file

expand all

pipe.c

+41 -18

fs/pipe.c

··· 391 391 struct pipe_inode_info *pipe = filp->private_data; 392 392 unsigned int head; 393 393 ssize_t ret = 0; 394 - int do_wakeup = 0; 395 394 size_t total_len = iov_iter_count(from); 396 395 ssize_t chars; 396 + bool was_empty = false; 397 397 398 398 /* Null write succeeds. */ 399 399 if (unlikely(total_len == 0)) ··· 407 407 goto out; 408 408 } 409 409 410 + /* 411 + * Only wake up if the pipe started out empty, since 412 + * otherwise there should be no readers waiting. 413 + * 414 + * If it wasn't empty we try to merge new data into 415 + * the last buffer. 416 + * 417 + * That naturally merges small writes, but it also 418 + * page-aligs the rest of the writes for large writes 419 + * spanning multiple pages. 420 + */ 410 421 head = pipe->head; 411 - 412 - /* We try to merge small writes */ 413 - chars = total_len & (PAGE_SIZE-1); /* size of the last buffer */ 414 - if (!pipe_empty(head, pipe->tail) && chars != 0) { 422 + was_empty = pipe_empty(head, pipe->tail); 423 + chars = total_len & (PAGE_SIZE-1); 424 + if (chars && !was_empty) { 415 425 unsigned int mask = pipe->ring_size - 1; 416 426 struct pipe_buffer *buf = &pipe->bufs[(head - 1) & mask]; 417 427 int offset = buf->offset + buf->len; ··· 436 426 ret = -EFAULT; 437 427 goto out; 438 428 } 439 - do_wakeup = 1; 429 + 440 430 buf->len += ret; 441 431 if (!iov_iter_count(from)) 442 432 goto out; ··· 481 471 } 482 472 483 473 pipe->head = head + 1; 484 - 485 - /* Always wake up, even if the copy fails. Otherwise 486 - * we lock up (O_NONBLOCK-)readers that sleep due to 487 - * syscall merging. 488 - * FIXME! Is this really true? 489 - */ 490 - wake_up_locked_poll( 491 - &pipe->wait, EPOLLIN | EPOLLRDNORM); 492 - 493 474 spin_unlock_irq(&pipe->wait.lock); 494 - kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); 495 475 496 476 /* Insert it into the buffer array */ 497 477 buf = &pipe->bufs[head & mask]; ··· 524 524 ret = -ERESTARTSYS; 525 525 break; 526 526 } 527 + 528 + /* 529 + * We're going to release the pipe lock and wait for more 530 + * space. We wake up any readers if necessary, and then 531 + * after waiting we need to re-check whether the pipe 532 + * become empty while we dropped the lock. 533 + */ 534 + if (was_empty) { 535 + wake_up_interruptible_sync_poll(&pipe->wait, EPOLLIN | EPOLLRDNORM); 536 + kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); 537 + } 527 538 pipe->waiting_writers++; 528 539 pipe_wait(pipe); 529 540 pipe->waiting_writers--; 541 + 542 + was_empty = pipe_empty(head, pipe->tail); 530 543 } 531 544 out: 532 545 __pipe_unlock(pipe); 533 - if (do_wakeup) { 534 - wake_up_interruptible_poll(&pipe->wait, EPOLLIN | EPOLLRDNORM); 546 + 547 + /* 548 + * If we do do a wakeup event, we do a 'sync' wakeup, because we 549 + * want the reader to start processing things asap, rather than 550 + * leave the data pending. 551 + * 552 + * This is particularly important for small writes, because of 553 + * how (for example) the GNU make jobserver uses small writes to 554 + * wake up pending jobs 555 + */ 556 + if (was_empty) { 557 + wake_up_interruptible_sync_poll(&pipe->wait, EPOLLIN | EPOLLRDNORM); 535 558 kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); 536 559 } 537 560 if (ret > 0 && sb_start_write_trylock(file_inode(filp)->i_sb)) {

Configure Feed

Configure Feed