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 <[email protected]>
---
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;
base-commit: 69b41ac87e4a664de78a395ff97166f0b2943210
--
2.31.1
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 <[email protected]>
> ---
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 <[email protected]> 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 <[email protected]>
> > ---
>
> 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 <[email protected]>
...
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 <[email protected]>
>> ---
>
> 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 <[email protected]> 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 <[email protected]>
>>> ---
>>
>> 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 <[email protected]>
> ...
>
> 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