| Message ID | 20230106094844.26241-1-zhanghongchen@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] pipe: use __pipe_{lock,unlock} instead of spinlock | expand |
On Fri, Jan 06, 2023 at 05:48:44PM +0800, 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. > > 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. > > Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn> > --- After the above "---" line you should have the changlog descrption. For instance: v3: - fixes bleh blah blah v2: - fixes 0-day report by ... etc.. - fixes spelling or whatever I cannot decipher what you did here differently, not do I want to go looking and diff'ing. So you are making the life of reviewer harder. Luis
On Fri, Jan 6, 2023 at 8:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Fri, Jan 06, 2023 at 05:48:44PM +0800, 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. > > > > 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. > > > > Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn> > > --- > > After the above "---" line you should have the changlog descrption. > For instance: > > v3: > - fixes bleh blah blah > v2: > - fixes 0-day report by ... etc.. > - fixes spelling or whatever > > I cannot decipher what you did here differently, not do I want to go > looking and diff'ing. So you are making the life of reviewer harder. > Happy new 2023. Positive wording... You can make reviewers' life easy when... (encourage people). Life is easy, people live hard. +1 Adding ChangeLog of patch history Cannot say... Might be good to add the link to Linus test-case + your results in the commit message as well? ... Link: https://git.kernel.org/linus/0ddad21d3e99 (test-case of Linus suggested-by Andrew) ... Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn> ... Thanks. Best regards, -Sedat-
Hi Luis, On 2023/1/7 上午3:13, Luis Chamberlain wrote: > On Fri, Jan 06, 2023 at 05:48:44PM +0800, 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. >> >> 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. >> >> Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn> >> --- > > After the above "---" line you should have the changlog descrption. > For instance: > > v3: > - fixes bleh blah blah > v2: > - fixes 0-day report by ... etc.. > - fixes spelling or whatever > > I cannot decipher what you did here differently, not do I want to go > looking and diff'ing. So you are making the life of reviewer harder. > > Luis > Matthew also reminded me to add the change log, but I don't think it is necessary to write the change log to fix the errors in the patch. Anyway, I think it is a good habit and will add these contents in the new v3 version. Best Regards, Hongchen Zhang
Hi Sedat, On 2023/1/7 am 4:33, Sedat Dilek wrote: > On Fri, Jan 6, 2023 at 8:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote: >> >> On Fri, Jan 06, 2023 at 05:48:44PM +0800, 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. >>> >>> 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. >>> >>> Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn> >>> --- >> >> After the above "---" line you should have the changlog descrption. >> For instance: >> >> v3: >> - fixes bleh blah blah >> v2: >> - fixes 0-day report by ... etc.. >> - fixes spelling or whatever >> >> I cannot decipher what you did here differently, not do I want to go >> looking and diff'ing. So you are making the life of reviewer harder. >> > > Happy new 2023. > > Positive wording... You can make reviewers' life easy when... > (encourage people). > Life is easy, people live hard. > > +1 Adding ChangeLog of patch history > > Cannot say... > Might be good to add the link to Linus test-case + your results in the > commit message as well? > > ... > Link: https://git.kernel.org/linus/0ddad21d3e99 (test-case of Linus > suggested-by Andrew) > ... > Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn> > ... > > Thanks. > > Best regards, > -Sedat- > OK, I have send a new v3 patch with these messages in commit message, Please help to check and review again. Best Regards, Hongchen Zhang
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;
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. 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. Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn> --- fs/pipe.c | 22 +--------------------- include/linux/pipe_fs_i.h | 12 ++++++++++++ kernel/watch_queue.c | 8 ++++---- 3 files changed, 17 insertions(+), 25 deletions(-) base-commit: 69b41ac87e4a664de78a395ff97166f0b2943210