| 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
| ||
|
Message-ID: <f1b30cc1-018a-b8a4-7363-69af2709775f@loongson.cn>
Date: Fri, 3 Feb 2023 09:42:24 +0800
From: Hongchen Zhang <zhanghongchen@...ngson.cn>
To: Alexander Viro <viro@...iv.linux.org.uk>,
Andrew Morton <akpm@...ux-foundation.org>,
"Christian Brauner (Microsoft)" <brauner@...nel.org>,
Luis Chamberlain <mcgrof@...nel.org>,
David Howells <dhowells@...hat.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"Fabio M. De Francesco" <fmdefrancesco@...il.com>,
Christophe JAILLET <christophe.jaillet@...adoo.fr>,
Linus Torvalds <torvalds@...ux-foundation.org>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
maobibo <maobibo@...ngson.cn>,
Matthew Wilcox <willy@...radead.org>,
Sedat Dilek <sedat.dilek@...il.com>
Subject: Re: [PATCH v4] pipe: use __pipe_{lock,unlock} instead of spinlock
Hi Linus,
Sorry to bother you. Can you help review this patch?
I tested this patch with the test code in you commit 0ddad21d3e99,
and the result looks better after applied this patch.
Best Regards
Hongchen Zhang
On 2023/1/29 am 2:04, Hongchen Zhang wrote:
> Use spinlock in pipe_{read,write} cost too much time,IMO
> pipe->{head,tail} can be protected by __pipe_{lock,unlock}.
> On the other hand, we can use __pipe_{lock,unlock} to protect
> the pipe->{head,tail} in pipe_resize_ring and
> post_one_notification.
>
> Reminded by Matthew, I tested this patch using UnixBench's pipe
> test case on a x86_64 machine,and get the following data:
> 1) before this patch
> System Benchmarks Partial Index BASELINE RESULT INDEX
> Pipe Throughput 12440.0 493023.3 396.3
> ========
> System Benchmarks Index Score (Partial Only) 396.3
>
> 2) after this patch
> System Benchmarks Partial Index BASELINE RESULT INDEX
> Pipe Throughput 12440.0 507551.4 408.0
> ========
> System Benchmarks Index Score (Partial Only) 408.0
>
> so we get ~3% speedup.
>
> Reminded by Andrew, I tested this patch with the test code in
> Linus's 0ddad21d3e99 and get following result:
> 1) before this patch
> 13,136.54 msec task-clock # 3.870 CPUs utilized
> 1,186,779 context-switches # 90.342 K/sec
> 668,867 cpu-migrations # 50.917 K/sec
> 895 page-faults # 68.131 /sec
> 29,875,711,543 cycles # 2.274 GHz
> 12,372,397,462 instructions # 0.41 insn per cycle
> 2,480,235,723 branches # 188.804 M/sec
> 47,191,943 branch-misses # 1.90% of all branches
>
> 3.394806886 seconds time elapsed
>
> 0.037869000 seconds user
> 0.189346000 seconds sys
>
> 2) after this patch
>
> 12,395.63 msec task-clock # 4.138 CPUs utilized
> 1,193,381 context-switches # 96.274 K/sec
> 585,543 cpu-migrations # 47.238 K/sec
> 1,063 page-faults # 85.756 /sec
> 27,691,587,226 cycles # 2.234 GHz
> 11,738,307,999 instructions # 0.42 insn per cycle
> 2,351,299,522 branches # 189.688 M/sec
> 45,404,526 branch-misses # 1.93% of all branches
>
> 2.995280878 seconds time elapsed
>
> 0.010615000 seconds user
> 0.206999000 seconds sys
> After adding this patch, the time used on this test program becomes less.
>
> Signed-off-by: Hongchen Zhang <zhanghongchen@...ngson.cn>
>
> v4:
> - fixes a typo in changelog when reviewed by Sedat.
> v3:
> - fixes the error reported by kernel test robot <oliver.sang@...el.com>
> Link: https://lore.kernel.org/oe-lkp/202301061340.c954d61f-oliver.sang@intel.com
> - add perf stat data for the test code in Linus's 0ddad21d3e99 in
> commit message.
> v2:
> - add UnixBench test data in commit message
> - fixes the test error reported by kernel test robot <lkp@...el.com>
> by adding the missing fs.h header file.
> ---
> fs/pipe.c | 22 +---------------------
> include/linux/pipe_fs_i.h | 12 ++++++++++++
> kernel/watch_queue.c | 8 ++++----
> 3 files changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 42c7ff41c2db..4355ee5f754e 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -98,16 +98,6 @@ void pipe_unlock(struct pipe_inode_info *pipe)
> }
> EXPORT_SYMBOL(pipe_unlock);
>
> -static inline void __pipe_lock(struct pipe_inode_info *pipe)
> -{
> - mutex_lock_nested(&pipe->mutex, I_MUTEX_PARENT);
> -}
> -
> -static inline void __pipe_unlock(struct pipe_inode_info *pipe)
> -{
> - mutex_unlock(&pipe->mutex);
> -}
> -
> void pipe_double_lock(struct pipe_inode_info *pipe1,
> struct pipe_inode_info *pipe2)
> {
> @@ -253,8 +243,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
> */
> was_full = pipe_full(pipe->head, pipe->tail, pipe->max_usage);
> for (;;) {
> - /* Read ->head with a barrier vs post_one_notification() */
> - unsigned int head = smp_load_acquire(&pipe->head);
> + unsigned int head = pipe->head;
> unsigned int tail = pipe->tail;
> unsigned int mask = pipe->ring_size - 1;
>
> @@ -322,14 +311,12 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
>
> if (!buf->len) {
> pipe_buf_release(pipe, buf);
> - spin_lock_irq(&pipe->rd_wait.lock);
> #ifdef CONFIG_WATCH_QUEUE
> if (buf->flags & PIPE_BUF_FLAG_LOSS)
> pipe->note_loss = true;
> #endif
> tail++;
> pipe->tail = tail;
> - spin_unlock_irq(&pipe->rd_wait.lock);
> }
> total_len -= chars;
> if (!total_len)
> @@ -506,16 +493,13 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
> * it, either the reader will consume it or it'll still
> * be there for the next write.
> */
> - spin_lock_irq(&pipe->rd_wait.lock);
>
> head = pipe->head;
> if (pipe_full(head, pipe->tail, pipe->max_usage)) {
> - spin_unlock_irq(&pipe->rd_wait.lock);
> continue;
> }
>
> pipe->head = head + 1;
> - spin_unlock_irq(&pipe->rd_wait.lock);
>
> /* Insert it into the buffer array */
> buf = &pipe->bufs[head & mask];
> @@ -1260,14 +1244,12 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots)
> if (unlikely(!bufs))
> return -ENOMEM;
>
> - spin_lock_irq(&pipe->rd_wait.lock);
> mask = pipe->ring_size - 1;
> head = pipe->head;
> tail = pipe->tail;
>
> n = pipe_occupancy(head, tail);
> if (nr_slots < n) {
> - spin_unlock_irq(&pipe->rd_wait.lock);
> kfree(bufs);
> return -EBUSY;
> }
> @@ -1303,8 +1285,6 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots)
> pipe->tail = tail;
> pipe->head = head;
>
> - spin_unlock_irq(&pipe->rd_wait.lock);
> -
> /* This might have made more room for writers */
> wake_up_interruptible(&pipe->wr_wait);
> return 0;
> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index 6cb65df3e3ba..f5084daf6eaf 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -2,6 +2,8 @@
> #ifndef _LINUX_PIPE_FS_I_H
> #define _LINUX_PIPE_FS_I_H
>
> +#include <linux/fs.h>
> +
> #define PIPE_DEF_BUFFERS 16
>
> #define PIPE_BUF_FLAG_LRU 0x01 /* page is on the LRU */
> @@ -223,6 +225,16 @@ static inline void pipe_discard_from(struct pipe_inode_info *pipe,
> #define PIPE_SIZE PAGE_SIZE
>
> /* Pipe lock and unlock operations */
> +static inline void __pipe_lock(struct pipe_inode_info *pipe)
> +{
> + mutex_lock_nested(&pipe->mutex, I_MUTEX_PARENT);
> +}
> +
> +static inline void __pipe_unlock(struct pipe_inode_info *pipe)
> +{
> + mutex_unlock(&pipe->mutex);
> +}
> +
> void pipe_lock(struct pipe_inode_info *);
> void pipe_unlock(struct pipe_inode_info *);
> void pipe_double_lock(struct pipe_inode_info *, struct pipe_inode_info *);
> diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
> index a6f9bdd956c3..92e46cfe9419 100644
> --- a/kernel/watch_queue.c
> +++ b/kernel/watch_queue.c
> @@ -108,7 +108,7 @@ static bool post_one_notification(struct watch_queue *wqueue,
> if (!pipe)
> return false;
>
> - spin_lock_irq(&pipe->rd_wait.lock);
> + __pipe_lock(pipe);
>
> mask = pipe->ring_size - 1;
> head = pipe->head;
> @@ -135,17 +135,17 @@ static bool post_one_notification(struct watch_queue *wqueue,
> buf->offset = offset;
> buf->len = len;
> buf->flags = PIPE_BUF_FLAG_WHOLE;
> - smp_store_release(&pipe->head, head + 1); /* vs pipe_read() */
> + pipe->head = head + 1;
>
> if (!test_and_clear_bit(note, wqueue->notes_bitmap)) {
> - spin_unlock_irq(&pipe->rd_wait.lock);
> + __pipe_unlock(pipe);
> BUG();
> }
> wake_up_interruptible_sync_poll_locked(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM);
> done = true;
>
> out:
> - spin_unlock_irq(&pipe->rd_wait.lock);
> + __pipe_unlock(pipe);
> if (done)
> kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> return done;
>
Powered by blists - more mailing lists