| Message ID | 20250303230409.452687-2-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | some pipe + wait stuff | expand |
On 03/04, Mateusz Guzik wrote: > > @@ -529,10 +529,9 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from) > > if (!iov_iter_count(from)) > break; > - } > > - if (!pipe_full(head, pipe->tail, pipe->max_usage)) > continue; > + } Reviewed-by: Oleg Nesterov <oleg@redhat.com> It seems that we can also remove the unnecessary signal_pending() check, but I need to recheck and we need to cleanup the poll_usage logic first. This will also remove the unnecessary wakeups when the writer is interrupted by signal/ diff --git a/fs/pipe.c b/fs/pipe.c index b0641f75b1ba..ed55a86ca03b 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -541,12 +541,6 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from) ret = -EAGAIN; break; } - if (signal_pending(current)) { - if (!ret) - ret = -ERESTARTSYS; - break; - } - /* * We're going to release the pipe lock and wait for more * space. We wake up any readers if necessary, and then @@ -554,10 +548,11 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from) * become empty while we dropped the lock. */ mutex_unlock(&pipe->mutex); - if (was_empty) + if (was_empty || pipe->poll_usage) wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); - wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe)); + if (wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe)) < 0) + return ret ?: -ERESTARTSYS; mutex_lock(&pipe->mutex); was_empty = pipe_empty(pipe->head, pipe->tail); wake_next_writer = true;
On Tue, Mar 4, 2025 at 3:08 PM Oleg Nesterov <oleg@redhat.com> wrote: > > On 03/04, Mateusz Guzik wrote: > > > > @@ -529,10 +529,9 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from) > > > > if (!iov_iter_count(from)) > > break; > > - } > > > > - if (!pipe_full(head, pipe->tail, pipe->max_usage)) > > continue; > > + } > > Reviewed-by: Oleg Nesterov <oleg@redhat.com> > thanks > It seems that we can also remove the unnecessary signal_pending() > check, but I need to recheck and we need to cleanup the poll_usage > logic first. > > This will also remove the unnecessary wakeups when the writer is > interrupted by signal/ > [snip] There are many touch ups to do here, I don't have an opinion about this diff. I don't have compiled stats handy, but few months back I asked some people to query pipe writes with dtrace on FreeBSD. Everything is very heavily skewed towards < 128 bytes in total, including tons of 1 bytes (and no, it's not just make). However, there is also quite a bit of absolutely massive multipage ops as well -- north of 64K even. Doing that kind of op in one go is way faster than restarting rep mov every time interleaved with SMAP toggling, which is expensive in its own right. Thus I would argue someone(tm) should do it in Linux, but I don't have immediate plans. Perhaps you would be happy to do it? :) In the meantime, I would hope any refactoring in the area would make the above easier (my patch does, I'm not claiming your does not :P)
On 03/04, Mateusz Guzik wrote: > > On Tue, Mar 4, 2025 at 3:08 PM Oleg Nesterov <oleg@redhat.com> wrote: > > > > It seems that we can also remove the unnecessary signal_pending() > > check, but I need to recheck and we need to cleanup the poll_usage > > logic first. > > > > This will also remove the unnecessary wakeups when the writer is > > interrupted by signal/ > > > > There are many touch ups to do here, I don't have an opinion about this diff. ... > Thus I would argue someone(tm) should do it in Linux, but I don't have > immediate plans. Perhaps you would be happy to do it? :) Probably. In any case this needs a separate patch, sorry for confusion. Oleg.
diff --git a/fs/pipe.c b/fs/pipe.c index 19a7948ab234..d5238f6e0f08 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -529,10 +529,9 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from) if (!iov_iter_count(from)) break; - } - if (!pipe_full(head, pipe->tail, pipe->max_usage)) continue; + } /* Wait for buffer space to become available. */ if ((filp->f_flags & O_NONBLOCK) ||
The check operates on the stale value of 'head' and always loops back. Just do it unconditionally. No functional changes. Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- fs/pipe.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)