linux-kernel - Performance regression introduced by commit b667b8673443 ("pipe: Advance tail pointer inside of wait spinlock in pipe

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>] [day] [month] [year] [list]

Message-ID: <c6ed1ca0-3e39-714c-9590-54e13695b9b9@redhat.com>
Date:   Fri, 17 Jan 2020 11:53:48 -0500
From:   Waiman Long <longman@...hat.com>
To:     David Howells <dhowells@...hat.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Performance regression introduced by commit b667b8673443 ("pipe:
 Advance tail pointer inside of wait spinlock in pipe_read()")

David,

I had found that parallel kernel build became much slower when a
5.5-based kernel is used. On a 2-socket 96-thread x86-64 system, the
"make -j88" time increased from less than 3 minutes with the 5.4 kernel
to more than double with the 5.5 kernel.

So I used bisection to try to find the culprit:

b667b867344301e24f21d4a4c844675ff61d89e1 is the first bad commit
commit b667b867344301e24f21d4a4c844675ff61d89e1
Author: David Howells <dhowells@...hat.com>
Date:   Tue Sep 24 16:09:04 2019 +0100

    pipe: Advance tail pointer inside of wait spinlock in pipe_read()
   
    Advance the pipe ring tail pointer inside of wait spinlock in
pipe_read()
    so that the pipe can be written into with kernel notifications from
    contexts where pipe->mutex cannot be taken.
   
    Signed-off-by: David Howells <dhowells@...hat.com>

diff --git a/fs/pipe.c b/fs/pipe.c
index 69afeab8a73a..ea134f69a292 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -325,9 +325,14 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
 
                        if (!buf->len) {
                                pipe_buf_release(pipe, buf);
+                               spin_lock_irq(&pipe->wait.lock);
                                tail++;
                                pipe->tail = tail;
-                               do_wakeup = 1;
+                               do_wakeup = 0;
+                               wake_up_interruptible_sync_poll_locked(
+                                       &pipe->wait, EPOLLOUT |
EPOLLWRNORM);
+                               spin_unlock_irq(&pipe->wait.lock);
+                               kill_fasync(&pipe->fasync_writers,
SIGIO, POLL_O
                        }

I guess the make command may make heavy use of pipe. The adding of
spinlock code in your patch may probably over-serialize the pipe
operation. Could you achieve the same functionality without adding a lock?

Cheers,
Longman