diff mbox series

pipe: use __pipe_{lock,unlock} instead of spinlock

Message ID 20230103030329.20252-1-zhanghongchen@loongson.cn (mailing list archive)
State Superseded
Headers show
Series pipe: use __pipe_{lock,unlock} instead of spinlock | expand

Commit Message

Hongchen Zhang Jan. 3, 2023, 3:03 a.m. UTC 
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.

Signed-off-by: Hongchen Zhang <zhanghongchen@loongson.cn>
---
 fs/pipe.c                 | 24 ++++--------------------
 include/linux/pipe_fs_i.h | 10 ++++++++++
 kernel/watch_queue.c      |  8 ++++----
 3 files changed, 18 insertions(+), 24 deletions(-)


base-commit: c8451c141e07a8d05693f6c8d0e418fbb4b68bb7

Comments

kernel test robot Jan. 3, 2023, 4:30 a.m. UTC | #1 
Hi Hongchen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on c8451c141e07a8d05693f6c8d0e418fbb4b68bb7]

url:    https://github.com/intel-lab-lkp/linux/commits/Hongchen-Zhang/pipe-use-__pipe_-lock-unlock-instead-of-spinlock/20230103-110504
base:   c8451c141e07a8d05693f6c8d0e418fbb4b68bb7
patch link:    https://lore.kernel.org/r/20230103030329.20252-1-zhanghongchen%40loongson.cn
patch subject: [PATCH] pipe: use __pipe_{lock,unlock} instead of spinlock
config: x86_64-rhel-8.3-kselftests
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/74b922b9da7ec76cce2065114d07afc5065b2df9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hongchen-Zhang/pipe-use-__pipe_-lock-unlock-instead-of-spinlock/20230103-110504
        git checkout 74b922b9da7ec76cce2065114d07afc5065b2df9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/tls/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/splice.h:12,
                    from net/tls/tls_sw.c:41:
   include/linux/pipe_fs_i.h: In function '__pipe_lock':
>> include/linux/pipe_fs_i.h:228:41: error: 'I_MUTEX_PARENT' undeclared (first use in this function)
     228 |         mutex_lock_nested(&pipe->mutex, I_MUTEX_PARENT);
         |                                         ^~~~~~~~~~~~~~
   include/linux/pipe_fs_i.h:228:41: note: each undeclared identifier is reported only once for each function it appears in


vim +/I_MUTEX_PARENT +228 include/linux/pipe_fs_i.h

   224	
   225	/* Pipe lock and unlock operations */
   226	static inline void __pipe_lock(struct pipe_inode_info *pipe)
   227	{
 > 228		mutex_lock_nested(&pipe->mutex, I_MUTEX_PARENT);
   229	}
   230
kernel test robot Jan. 3, 2023, 4:40 a.m. UTC | #2 
Hi Hongchen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on c8451c141e07a8d05693f6c8d0e418fbb4b68bb7]

url:    https://github.com/intel-lab-lkp/linux/commits/Hongchen-Zhang/pipe-use-__pipe_-lock-unlock-instead-of-spinlock/20230103-110504
base:   c8451c141e07a8d05693f6c8d0e418fbb4b68bb7
patch link:    https://lore.kernel.org/r/20230103030329.20252-1-zhanghongchen%40loongson.cn
patch subject: [PATCH] pipe: use __pipe_{lock,unlock} instead of spinlock
config: sh-allmodconfig
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/74b922b9da7ec76cce2065114d07afc5065b2df9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hongchen-Zhang/pipe-use-__pipe_-lock-unlock-instead-of-spinlock/20230103-110504
        git checkout 74b922b9da7ec76cce2065114d07afc5065b2df9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash net/tls/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/splice.h:12,
                    from net/tls/tls_sw.c:41:
   include/linux/pipe_fs_i.h: In function '__pipe_lock':
>> include/linux/pipe_fs_i.h:228:41: error: 'I_MUTEX_PARENT' undeclared (first use in this function)
     228 |         mutex_lock_nested(&pipe->mutex, I_MUTEX_PARENT);
         |                                         ^~~~~~~~~~~~~~
   include/linux/pipe_fs_i.h:228:41: note: each undeclared identifier is reported only once for each function it appears in


vim +/I_MUTEX_PARENT +228 include/linux/pipe_fs_i.h

   224	
   225	/* Pipe lock and unlock operations */
   226	static inline void __pipe_lock(struct pipe_inode_info *pipe)
   227	{
 > 228		mutex_lock_nested(&pipe->mutex, I_MUTEX_PARENT);
   229	}
   230
diff mbox series

Patch

diff --git a/fs/pipe.c b/fs/pipe.c
index 42c7ff41c2db..cf449779bf71 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,14 @@  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);
+	__pipe_lock(pipe);
 	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);
+		__pipe_unlock(pipe);
 		kfree(bufs);
 		return -EBUSY;
 	}
@@ -1303,7 +1287,7 @@  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);
+	__pipe_unlock(pipe);
 
 	/* This might have made more room for writers */
 	wake_up_interruptible(&pipe->wr_wait);
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 6cb65df3e3ba..baae3d062422 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -223,6 +223,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;