Skip to contentSkip to conversation

dynamic pipe resizing

RFC PATCH In review Jens Axboe 3 participants 5 messages Aug 24, 2007 – Dec 15, 2007

Subsystems (4)
VFS Generic include/asm header files File locking (flock() and fcntl()/lockf()) Relay
lore
Share & tools
lore

Jens Axboe

author
#0 lore raw

RFC PATCH 1/1 dynamic pipe resizing

Hi,

Dabbling around with splice a bit, I added some code to change the size of a pipe. Currently it's hardcoded as 16 pages, with this patch you can shrink (if you wanted) or grow (the likely scenario) if you want to increase the size of your in-kernel buffer for splice operations.

Like with my original splice patches from 2005, I used fcntl() F_GETPIPE_SZ and F_SETPIPE_SZ to change the size of the pipe. I'm not particularly fond of that interface, so suggestions on how to improve it would be appreciated. Even if fcntl() should be the preferred approach, I think it would be better to pass in a byte based value instead of a number of pages.

fs/fcntl.c +5 -0
index 78b2ff0..c4571fc 100644
@@ -18,6 +18,7 @@
 #include <linux/ptrace.h>
 #include <linux/signal.h>
 #include <linux/rcupdate.h>
+#include <linux/pipe_fs_i.h>
 
 #include <asm/poll.h>
 #include <asm/siginfo.h>

@@ -376,6 +377,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 	case F_NOTIFY:
 		err = fcntl_dirnotify(fd, filp, arg);
 		break;
+	case F_SETPIPE_SZ:
+	case F_GETPIPE_SZ:
+		err = pipe_fcntl(filp, cmd, arg);
+		break;
 	default:
 		break;
 	}
fs/pipe.c +94 -12

168 lines not on this page · raw message

fs/splice.c +73 -30

236 lines not on this page · raw message

include/asm-generic/fcntl.h +7 -0
index b847741..1d50973 100644
@@ -73,6 +73,13 @@
 #define F_SETSIG	10	/* for sockets. */
 #define F_GETSIG	11	/* for sockets. */
 #endif
+#ifndef F_SETPIPE_SZ		/* for growing pipe sizes */
+#define F_SETPIPE_SZ	12
+#endif
+#ifndef F_GETPIPE_SZ
+#define F_GETPIPE_SZ	13
+#endif
+
 
 /* for F_[GET|SET]FL */
 #define FD_CLOEXEC	1	/* actually anything with low bit set goes */
include/linux/pipe_fs_i.h +7 -4
index 8e41202..4765076 100644
@@ -3,7 +3,7 @@
 
 #define PIPEFS_MAGIC 0x50495045
 
-#define PIPE_BUFFERS (16)
+#define PIPE_DEF_BUFFERS	16
 
 #define PIPE_BUF_FLAG_LRU	0x01	/* page is on the LRU */
 #define PIPE_BUF_FLAG_ATOMIC	0x02	/* was atomically mapped */

@@ -44,17 +44,17 @@ struct pipe_buffer {
  **/
 struct pipe_inode_info {
 	wait_queue_head_t wait;
-	unsigned int nrbufs, curbuf;
-	struct page *tmp_page;
+	unsigned int nrbufs, curbuf, buffers;
 	unsigned int readers;
 	unsigned int writers;
 	unsigned int waiting_writers;
 	unsigned int r_counter;
 	unsigned int w_counter;
+	struct page *tmp_page;
 	struct fasync_struct *fasync_readers;
 	struct fasync_struct *fasync_writers;
 	struct inode *inode;
-	struct pipe_buffer bufs[PIPE_BUFFERS];
+	struct pipe_buffer *bufs;
 };
 
 /*

@@ -148,4 +148,7 @@ void generic_pipe_buf_get(struct pipe_inode_info *, struct pipe_buffer *);
 int generic_pipe_buf_confirm(struct pipe_inode_info *, struct pipe_buffer *);
 int generic_pipe_buf_steal(struct pipe_inode_info *, struct pipe_buffer *);
 
+/* for F_SETPIPE_SZ and F_GETPIPE_SZ */
+long pipe_fcntl(struct file *, unsigned int, unsigned long arg);
+
 #endif
include/linux/splice.h +7 -0
index 33e447f..ebb5e60 100644
@@ -70,4 +70,11 @@ extern ssize_t splice_to_pipe(struct pipe_inode_info *,
 extern ssize_t splice_direct_to_actor(struct file *, struct splice_desc *,
 				      splice_direct_actor *);
 
+/*
+ * for dynamic pipe sizing
+ */
+extern int splice_grow_spd(struct pipe_inode_info *, struct splice_pipe_desc *);
+extern void splice_shrink_spd(struct pipe_inode_info *,
+				struct splice_pipe_desc *);
+
 #endif
kernel/relay.c +9 -4
index ad85501..407e82d 100644
@@ -1080,8 +1080,8 @@ static int subbuf_splice_actor(struct file *in,
 	size_t read_subbuf = read_start / subbuf_size;
 	size_t padding = rbuf->padding[read_subbuf];
 	size_t nonpad_end = read_subbuf * subbuf_size + subbuf_size - padding;
-	struct page *pages[PIPE_BUFFERS];
-	struct partial_page partial[PIPE_BUFFERS];
+	struct page *pages[PIPE_DEF_BUFFERS];
+	struct partial_page partial[PIPE_DEF_BUFFERS];
 	struct splice_pipe_desc spd = {
 		.pages = pages,
 		.nr_pages = 0,

@@ -1092,6 +1092,8 @@ static int subbuf_splice_actor(struct file *in,
 
 	if (rbuf->subbufs_produced == rbuf->subbufs_consumed)
 		return 0;
+	if (splice_grow_spd(pipe, &spd))
+		return -ENOMEM;
 
 	/*
 	 * Adjust read len, if longer than what is available

@@ -1135,16 +1137,19 @@ static int subbuf_splice_actor(struct file *in,
 		}
 	}
 
+	ret = 0;
 	if (!spd.nr_pages)
-		return 0;
+		goto out;
 
 	ret = *nonpad_ret = splice_to_pipe(pipe, &spd);
 	if (ret < 0 || ret < total_len)
-		return ret;
+		goto out;
 
         if (read_start + ret == nonpad_end)
                 ret += padding;
 
+out:
+	splice_shrink_spd(pipe, &spd);
         return ret;
 }
Signature · 1 line
Jens Axboe
#1 lore raw
Quoted 6 lines · Jens Axboe

Hi,

Dabbling around with splice a bit, I added some code to change the size of a pipe. Currently it's hardcoded as 16 pages, with this patch you can shrink (if you wanted) or grow (the likely scenario) if you want to increase the size of your in-kernel buffer for splice operations.

Wonderful, I just did the same {-.-}
(but then dropped the patch because it did not help my application at all, so never mind).

Quoted 6 lines

Like with my original splice patches from 2005, I used fcntl() F_GETPIPE_SZ and F_SETPIPE_SZ to change the size of the pipe. I'm not particularly fond of that interface, so suggestions on how to improve it would be appreciated. Even if fcntl() should be the preferred approach, I think it would be better to pass in a byte based value instead of a number of pages.

And using % pipe->buffers then?

Jan

author
#2 lore raw
Quoted 13 lines · Jan Engelhardt
Quoted 6 lines · Jens Axboe

Hi,

Dabbling around with splice a bit, I added some code to change the size of a pipe. Currently it's hardcoded as 16 pages, with this patch you can shrink (if you wanted) or grow (the likely scenario) if you want to increase the size of your in-kernel buffer for splice operations.

Wonderful, I just did the same {-.-}
(but then dropped the patch because it did not help my application at all, so never mind).

Quoted 8 lines
Quoted 6 lines

Like with my original splice patches from 2005, I used fcntl() F_GETPIPE_SZ and F_SETPIPE_SZ to change the size of the pipe. I'm not particularly fond of that interface, so suggestions on how to improve it would be appreciated. Even if fcntl() should be the preferred approach, I think it would be better to pass in a byte based value instead of a number of pages.

And using % pipe->buffers then?

I'd probably prefer sticking to power-of-2 array sizes, so just round it up appropriately. But it's likely not a big deal.

Signature · 1 line
Jens Axboe
#3 lore raw
In reply to Jens Axboe
Quoted 9 lines · Jens Axboe

Subject: [PATCH][RFC] dynamic pipe resizing

Hi,

Dabbling around with splice a bit, I added some code to change the size of a pipe. Currently it's hardcoded as 16 pages, with this patch you can shrink (if you wanted) or grow (the likely scenario) if you want to increase the size of your in-kernel buffer for splice operations.

Quoted 7 lines

Like with my original splice patches from 2005, I used fcntl() F_GETPIPE_SZ and F_SETPIPE_SZ to change the size of the pipe. I'm not particularly fond of that interface, so suggestions on how to improve it would be appreciated. Even if fcntl() should be the preferred approach, I think it would be better to pass in a byte based value instead of a number of pages.

Could this patch still make it in?
Yes, I think its set() and get() parts should use bytes and convert to/from pages.

Quoted 4 lines

/*
+ * Allocate a new array of pipe buffers and copy the info over. Returns the
+ * pipe size if successful, or return -ERROR on error.
+ */

fs/pipe.c int do_pipe(int *fd) · line 1099

Quoted 10 lines
fs/pipe.c +10 -0
+static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
+{
+	struct pipe_buffer *bufs;
+
+	/*
+	 * Must be a power-of-2 current
+	 */
+	if (arg & (arg - 1))
+		return -EINVAL;
+

Perhaps just round up, and mention the rounding in the manpage, so that noone gets a shock when the pipe is not exactly as small as requested.

fs/pipe.c int do_pipe(int *fd) · line 1107

Quoted 8 lines
fs/pipe.c +8 -0
+	/*
+	 * We can shrink the pipe, if arg >= pipe->nrbufs. Since we don't
+	 * expect a lot of shrink+grow operations, just free and allocate
+	 * again like we would do for growing. If the pipe currently
+	 * contains more buffers than arg, then return busy.
+	 */
+	if (arg < pipe->nrbufs)
+		return -EBUSY;

How big can a pipe become? k*alloc has a certain limit, so should perhaps vmalloc be used for when kalloc fails?

#4 lore raw
Quoted 9 lines · Jan Engelhardt
Quoted 8 lines · Jens Axboe

Subject: [PATCH][RFC] dynamic pipe resizing
Like with my original splice patches from 2005, I used fcntl() F_GETPIPE_SZ and F_SETPIPE_SZ to change the size of the pipe. I'm not particularly fond of that interface, so suggestions on how to improve it would be appreciated. Even if fcntl() should be the preferred approach, I think it would be better to pass in a byte based value instead of a number of pages.

Quoted 6 lines

Could this patch still make it in?
Yes, I think its set() and get() parts should use bytes and convert to/from pages.

Perhaps just round up, and mention the rounding in the manpage, so that noone gets a shock when the pipe is not exactly as small as requested.

Yes, but document only that it is rounding and make the unit arbitrary. That reduces the ABI requirements.

Best Regards

Ingo Oeser