From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7A767C4338F for ; Wed, 25 Aug 2021 14:11:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 592FE61073 for ; Wed, 25 Aug 2021 14:11:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241142AbhHYOMj (ORCPT ); Wed, 25 Aug 2021 10:12:39 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:59760 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229760AbhHYOMg (ORCPT ); Wed, 25 Aug 2021 10:12:36 -0400 Received: from in01.mta.xmission.com ([166.70.13.51]:58078) by out01.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1mItd7-00DzB7-7F; Wed, 25 Aug 2021 08:11:49 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95]:53520 helo=email.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1mItd6-00D0uB-2n; Wed, 25 Aug 2021 08:11:48 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Linus Torvalds Cc: kernel test robot , Colin Ian King , Sandeep Patil , Mel Gorman , LKML , lkp@lists.01.org, kernel test robot , "Huang\, Ying" , Feng Tang , Zhengjun Xing References: <20210824151337.GC27667@xsang-OptiPlex-9020> Date: Wed, 25 Aug 2021 09:11:19 -0500 In-Reply-To: (Linus Torvalds's message of "Tue, 24 Aug 2021 10:39:25 -0700") Message-ID: <875yvtpqbc.fsf@disp2133> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1mItd6-00D0uB-2n;;;mid=<875yvtpqbc.fsf@disp2133>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1+f1x4Zi+tQtGk/ZhqOYIuQyM7dMciL9to= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [pipe] 3b844826b6: stress-ng.sigio.ops_per_sec -99.3% regression X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Linus Torvalds writes: > On Tue, Aug 24, 2021 at 10:32 AM Linus Torvalds > wrote: >> >> We could do the same ugly thing for FASYNC that we do for EPOLLET - >> make it always fasync on new data, exactly because the previous SIGIO >> might not have emptied the buffer completely. > > The patch would be something like the attached (UNTESTED!) We have two things going on, a pipe wake up and signal wake up. Does their order matter? It feels weird that it is possible that the data can be read from the pipe and the reader woken up to write more when the signal that notifies the reader of that state has not even been queued for delivery. Eric > Linus > > fs/pipe.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/fs/pipe.c b/fs/pipe.c > index 678dee2a8228..6d4342bad9f1 100644 > --- a/fs/pipe.c > +++ b/fs/pipe.c > @@ -363,10 +363,9 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) > * _very_ unlikely case that the pipe was full, but we got > * no data. > */ > - if (unlikely(was_full)) { > + if (unlikely(was_full)) > wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); > - kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > - } > + kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > > /* > * But because we didn't read anything, at this point we can > @@ -385,12 +384,11 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) > wake_next_reader = false; > __pipe_unlock(pipe); > > - if (was_full) { > + if (was_full) > wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); > - kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > - } > if (wake_next_reader) > wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); > + kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > if (ret > 0) > file_accessed(filp); > return ret; > @@ -565,10 +563,9 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) > * become empty while we dropped the lock. > */ > __pipe_unlock(pipe); > - if (was_empty) { > + if (was_empty) > wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); > - kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > - } > + kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe)); > __pipe_lock(pipe); > was_empty = pipe_empty(pipe->head, pipe->tail); > @@ -591,10 +588,9 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) > * Epoll nonsensically wants a wakeup whether the pipe > * was already empty or not. > */ > - if (was_empty || pipe->poll_usage) { > + if (was_empty || pipe->poll_usage) > wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); > - kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > - } > + kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > if (wake_next_writer) > wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); > if (ret > 0 && sb_start_write_trylock(file_inode(filp)->i_sb)) { From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============2670045224984317636==" MIME-Version: 1.0 From: Eric W. Biederman To: lkp@lists.01.org Subject: Re: [pipe] 3b844826b6: stress-ng.sigio.ops_per_sec -99.3% regression Date: Wed, 25 Aug 2021 09:11:19 -0500 Message-ID: <875yvtpqbc.fsf@disp2133> In-Reply-To: List-Id: --===============2670045224984317636== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Linus Torvalds writes: > On Tue, Aug 24, 2021 at 10:32 AM Linus Torvalds > wrote: >> >> We could do the same ugly thing for FASYNC that we do for EPOLLET - >> make it always fasync on new data, exactly because the previous SIGIO >> might not have emptied the buffer completely. > > The patch would be something like the attached (UNTESTED!) We have two things going on, a pipe wake up and signal wake up. Does their order matter? It feels weird that it is possible that the data can be read from the pipe and the reader woken up to write more when the signal that notifies the reader of that state has not even been queued for delivery. Eric > Linus > > fs/pipe.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/fs/pipe.c b/fs/pipe.c > index 678dee2a8228..6d4342bad9f1 100644 > --- a/fs/pipe.c > +++ b/fs/pipe.c > @@ -363,10 +363,9 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) > * _very_ unlikely case that the pipe was full, but we got > * no data. > */ > - if (unlikely(was_full)) { > + if (unlikely(was_full)) > wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNOR= M); > - kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > - } > + kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > = > /* > * But because we didn't read anything, at this point we can > @@ -385,12 +384,11 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) > wake_next_reader =3D false; > __pipe_unlock(pipe); > = > - if (was_full) { > + if (was_full) > wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM= ); > - kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > - } > if (wake_next_reader) > wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); > + kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); > if (ret > 0) > file_accessed(filp); > return ret; > @@ -565,10 +563,9 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) > * become empty while we dropped the lock. > */ > __pipe_unlock(pipe); > - if (was_empty) { > + if (was_empty) > wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM= ); > - kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > - } > + kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe)); > __pipe_lock(pipe); > was_empty =3D pipe_empty(pipe->head, pipe->tail); > @@ -591,10 +588,9 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) > * Epoll nonsensically wants a wakeup whether the pipe > * was already empty or not. > */ > - if (was_empty || pipe->poll_usage) { > + if (was_empty || pipe->poll_usage) > wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); > - kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > - } > + kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); > if (wake_next_writer) > wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM= ); > if (ret > 0 && sb_start_write_trylock(file_inode(filp)->i_sb)) { --===============2670045224984317636==--