[patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b]

From: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

This patch adds preadv and pwritev system calls.  These syscalls are a
pretty straightforward combination of pread and readv (same for write). 
They are quite useful for doing vectored I/O in threaded applications. 
Using lseek+readv instead opens race windows you'll have to plug with
locking.

Other systems have such system calls too, for example NetBSD, check
here: http://www.daemon-systems.org/man/preadv.2.html

The application-visible interface provided by glibc should look like
this to be compatible to the existing implementations in the *BSD family:

  ssize_t preadv(int d, const struct iovec *iov, int iovcnt, off_t offset);
  ssize_t pwritev(int d, const struct iovec *iov, int iovcnt, off_t offset);

This prototype has one problem though: On 32bit archs is the (64bit)
offset argument unaligned, which the syscall ABI of several archs doesn't
allow to do.  At least s390 needs a wrapper in glibc to handle this.  As
we'll need a wrappers in glibc anyway I've decided to push problem to
glibc entriely and use a syscall prototype which works without
arch-specific wrappers inside the kernel: The offset argument is
explicitly splitted into two 32bit values.

The patch sports the actual system call implementation and the windup in
the x86 system call tables.  Other archs follow as separate patches.

Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
Cc: <linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: <linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
Cc: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
---

 arch/x86/ia32/ia32entry.S          |    2 +
 arch/x86/include/asm/unistd_32.h   |    2 +
 arch/x86/include/asm/unistd_64.h   |    4 ++
 arch/x86/kernel/syscall_table_32.S |    2 +
 fs/compat.c                        |   36 +++++++++++++++++++
 fs/read_write.c                    |   50 +++++++++++++++++++++++++++
 include/linux/compat.h             |    6 +++
 include/linux/syscalls.h           |    4 ++
 8 files changed, 106 insertions(+)

diff -puN arch/x86/ia32/ia32entry.S~preadv-pwritev-add-preadv-and-pwritev-system-calls arch/x86/ia32/ia32entry.S
--- a/arch/x86/ia32/ia32entry.S~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/arch/x86/ia32/ia32entry.S
@@ -828,4 +828,6 @@ ia32_sys_call_table:
 	.quad sys_dup3			/* 330 */
 	.quad sys_pipe2
 	.quad sys_inotify_init1
+	.quad compat_sys_preadv
+	.quad compat_sys_pwritev
 ia32_syscall_end:
diff -puN arch/x86/include/asm/unistd_32.h~preadv-pwritev-add-preadv-and-pwritev-system-calls arch/x86/include/asm/unistd_32.h
--- a/arch/x86/include/asm/unistd_32.h~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/arch/x86/include/asm/unistd_32.h
@@ -338,6 +338,8 @@
 #define __NR_dup3		330
 #define __NR_pipe2		331
 #define __NR_inotify_init1	332
+#define __NR_preadv		333
+#define __NR_pwritev		334
 
 #ifdef __KERNEL__
 
diff -puN arch/x86/include/asm/unistd_64.h~preadv-pwritev-add-preadv-and-pwritev-system-calls arch/x86/include/asm/unistd_64.h
--- a/arch/x86/include/asm/unistd_64.h~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/arch/x86/include/asm/unistd_64.h
@@ -653,6 +653,10 @@ __SYSCALL(__NR_dup3, sys_dup3)
 __SYSCALL(__NR_pipe2, sys_pipe2)
 #define __NR_inotify_init1			294
 __SYSCALL(__NR_inotify_init1, sys_inotify_init1)
+#define __NR_preadv				295
+__SYSCALL(__NR_preadv, sys_preadv)
+#define __NR_pwritev				296
+__SYSCALL(__NR_pwritev, sys_pwritev)
 
 
 #ifndef __NO_STUBS
diff -puN arch/x86/kernel/syscall_table_32.S~preadv-pwritev-add-preadv-and-pwritev-system-calls arch/x86/kernel/syscall_table_32.S
--- a/arch/x86/kernel/syscall_table_32.S~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/arch/x86/kernel/syscall_table_32.S
@@ -332,3 +332,5 @@ ENTRY(sys_call_table)
 	.long sys_dup3			/* 330 */
 	.long sys_pipe2
 	.long sys_inotify_init1
+	.long sys_preadv
+	.long sys_pwritev
diff -puN fs/compat.c~preadv-pwritev-add-preadv-and-pwritev-system-calls fs/compat.c
--- a/fs/compat.c~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/fs/compat.c
@@ -1232,6 +1232,24 @@ compat_sys_readv(unsigned long fd, const
 	return ret;
 }
 
+asmlinkage ssize_t
+compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
+		  unsigned long vlen, u32 pos_high, u32 pos_low)
+{
+	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	struct file *file;
+	ssize_t ret;
+
+	if (pos < 0)
+		return -EINVAL;
+	file = fget(fd);
+	if (!file)
+		return -EBADF;
+	ret = compat_readv(file, vec, vlen, &pos);
+	fput(file);
+	return ret;
+}
+
 static size_t compat_writev(struct file *file,
 			    const struct compat_iovec __user *vec,
 			    unsigned long vlen, loff_t *pos)
@@ -1269,6 +1287,24 @@ compat_sys_writev(unsigned long fd, cons
 	return ret;
 }
 
+asmlinkage ssize_t
+compat_sys_pwritev(unsigned long fd, const struct compat_iovec __user *vec,
+		   unsigned long vlen, u32 pos_high, u32 pos_low)
+{
+	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	struct file *file;
+	ssize_t ret;
+
+	if (pos < 0)
+		return -EINVAL;
+	file = fget(fd);
+	if (!file)
+		return -EBADF;
+	ret = compat_writev(file, vec, vlen, &pos);
+	fput(file);
+	return ret;
+}
+
 asmlinkage long
 compat_sys_vmsplice(int fd, const struct compat_iovec __user *iov32,
 		    unsigned int nr_segs, unsigned int flags)
diff -puN fs/read_write.c~preadv-pwritev-add-preadv-and-pwritev-system-calls fs/read_write.c
--- a/fs/read_write.c~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/fs/read_write.c
@@ -731,6 +731,56 @@ SYSCALL_DEFINE3(writev, unsigned long, f
 	return ret;
 }
 
+SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
+		unsigned long, vlen, u32, pos_high, u32, pos_low)
+{
+	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	struct file *file;
+	ssize_t ret = -EBADF;
+	int fput_needed;
+
+	if (pos < 0)
+		return -EINVAL;
+
+	file = fget_light(fd, &fput_needed);
+	if (file) {
+		ret = -ESPIPE;
+		if (file->f_mode & FMODE_PREAD)
+			ret = vfs_readv(file, vec, vlen, &pos);
+		fput_light(file, fput_needed);
+	}
+
+	if (ret > 0)
+		add_rchar(current, ret);
+	inc_syscr(current);
+	return ret;
+}
+
+SYSCALL_DEFINE5(pwritev, unsigned long, fd, const struct iovec __user *, vec,
+		unsigned long, vlen, u32, pos_high, u32, pos_low)
+{
+	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	struct file *file;
+	ssize_t ret = -EBADF;
+	int fput_needed;
+
+	if (pos < 0)
+		return -EINVAL;
+
+	file = fget_light(fd, &fput_needed);
+	if (file) {
+		ret = -ESPIPE;
+		if (file->f_mode & FMODE_PWRITE)
+			ret = vfs_writev(file, vec, vlen, &pos);
+		fput_light(file, fput_needed);
+	}
+
+	if (ret > 0)
+		add_wchar(current, ret);
+	inc_syscw(current);
+	return ret;
+}
+
 static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 			   size_t count, loff_t max)
 {
diff -puN include/linux/compat.h~preadv-pwritev-add-preadv-and-pwritev-system-calls include/linux/compat.h
--- a/include/linux/compat.h~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/include/linux/compat.h
@@ -191,6 +191,12 @@ asmlinkage ssize_t compat_sys_readv(unsi
 		const struct compat_iovec __user *vec, unsigned long vlen);
 asmlinkage ssize_t compat_sys_writev(unsigned long fd,
 		const struct compat_iovec __user *vec, unsigned long vlen);
+asmlinkage ssize_t compat_sys_preadv(unsigned long fd,
+		const struct compat_iovec __user *vec,
+		unsigned long vlen, u32 pos_high, u32 pos_low);
+asmlinkage ssize_t compat_sys_pwritev(unsigned long fd,
+		const struct compat_iovec __user *vec,
+		unsigned long vlen, u32 pos_high, u32 pos_low);
 
 int compat_do_execve(char * filename, compat_uptr_t __user *argv,
 	        compat_uptr_t __user *envp, struct pt_regs * regs);
diff -puN include/linux/syscalls.h~preadv-pwritev-add-preadv-and-pwritev-system-calls include/linux/syscalls.h
--- a/include/linux/syscalls.h~preadv-pwritev-add-preadv-and-pwritev-system-calls
+++ a/include/linux/syscalls.h
@@ -461,6 +461,10 @@ asmlinkage long sys_pread64(unsigned int
 			    size_t count, loff_t pos);
 asmlinkage long sys_pwrite64(unsigned int fd, const char __user *buf,
 			     size_t count, loff_t pos);
+asmlinkage long sys_preadv(unsigned long fd, const struct iovec __user *vec,
+			   unsigned long vlen, u32 pos_high, u32 pos_low);
+asmlinkage long sys_pwritev(unsigned long fd, const struct iovec __user *vec,
+			    unsigned long vlen, u32 pos_high, u32 pos_low);
 asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
 asmlinkage long sys_mkdir(const char __user *pathname, int mode);
 asmlinkage long sys_chdir(const char __user *filename);
_
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Linus Torvalds]

On Thu, 2 Apr 2009, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org wrote:
>
> glibc entriely and use a syscall prototype which works without
> arch-specific wrappers inside the kernel: The offset argument is
> explicitly split into two 32bit values.

Grr. That may make sense on 32-bit architectures, but makes no sense 
what-so-ever on 64-bit ones.

Why not just say that it's two "unsigned long" arguments, and split it by 
"sizeof unsigned long". And then, on 64-bit architectures, the high bits 
can be ignored. You can even get gcc to optimize it all away by doing 
something like

	#define HALF_LONG (BITS_IN_LONG / 2)

	offset = (((loff_t)high << HALF_LONG) << HALF_LONG) | low;

and then gcc should automaticaly notice that shifting up that way is just 
a fancy way of saying "0", and ignore the high bits.

How married are people to this crazy 2x "32 bits" model?

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Gerd Hoffmann]

Linus Torvalds wrote:
> 
> On Thu, 2 Apr 2009, akpm@linux-foundation.org wrote:
>> glibc entriely and use a syscall prototype which works without
>> arch-specific wrappers inside the kernel: The offset argument is
>> explicitly split into two 32bit values.
> 
> Grr. That may make sense on 32-bit architectures, but makes no sense 
> what-so-ever on 64-bit ones.
> 
> Why not just say that it's two "unsigned long" arguments, and split it by 
> "sizeof unsigned long". And then, on 64-bit architectures, the high bits 
> can be ignored. You can even get gcc to optimize it all away by doing 
> something like
> 
> 	#define HALF_LONG (BITS_IN_LONG / 2)
> 
> 	offset = (((loff_t)high << HALF_LONG) << HALF_LONG) | low;
> 
> and then gcc should automaticaly notice that shifting up that way is just 
> a fancy way of saying "0", and ignore the high bits.
> 
> How married are people to this crazy 2x "32 bits" model?

The tricky corner case are 32bit compat syscalls on 64bit kernel, where
a noticable number of archs (IIRC at least ppc, mips and s390) can't
pass unaligned 64bit values through the syscall abi.  The 32/32 split
was done because of that.

I think using two unsigned longs instead of two u32 should work.  And
allows us to handle 128bit file offsets on 64bit.  That I'll call
future-proof interface!  Nevertheless I'd like to have comments from the
arch maintainers on the idea.

cheers,
  Gerd

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

Gerd Hoffmann wrote:
>> How married are people to this crazy 2x "32 bits" model?
> 
> The tricky corner case are 32bit compat syscalls on 64bit kernel, where
> a noticable number of archs (IIRC at least ppc, mips and s390) can't
> pass unaligned 64bit values through the syscall abi.  The 32/32 split
> was done because of that.
> 
> I think using two unsigned longs instead of two u32 should work.  And
> allows us to handle 128bit file offsets on 64bit.  That I'll call
> future-proof interface!  Nevertheless I'd like to have comments from the
> arch maintainers on the idea.

Patch attached for comments (builds, but otherwise not tested yet).

cheers,
  Gerd


[-- Attachment #2: 0001-preadv-pwritev-tweak-interface.patch --]
[-- Type: text/plain, Size: 4632 bytes --]

>From 83ba4d31b29e931c08da1570bb47a2defef353fe Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 3 Apr 2009 11:08:00 +0200
Subject: [PATCH] preadv/pwritev: tweak interface.

Go from "u32" to "unsigned long" for the pos_{high,low} syscall
arguments.  That way we avoid the 32/32 split for 64bit archs,
and we still side-step the ABI issue with unaligned 64bit args
on a number of 32bit archs.  pos_high on 64bit is unused for
the time being. Some day we might be happy to have it though ;)

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 fs/compat.c              |    6 +++---
 fs/read_write.c          |   12 ++++++++----
 include/linux/compat.h   |    2 +-
 include/linux/syscalls.h |    4 ++--
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 1c859da..053f843 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1236,9 +1236,9 @@ compat_sys_readv(unsigned long fd, const struct compat_iovec __user *vec,
 
 asmlinkage ssize_t
 compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
-		  unsigned long vlen, u32 pos_high, u32 pos_low)
+		  unsigned long vlen, unsigned long pos_high, unsigned long pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = ((loff_t)pos_high << BITS_PER_COMPAT_LONG) | pos_low;
 	struct file *file;
 	int fput_needed;
 	ssize_t ret;
@@ -1295,7 +1295,7 @@ asmlinkage ssize_t
 compat_sys_pwritev(unsigned long fd, const struct compat_iovec __user *vec,
 		   unsigned long vlen, u32 pos_high, u32 pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = ((loff_t)pos_high << BITS_PER_COMPAT_LONG) | pos_low;
 	struct file *file;
 	int fput_needed;
 	ssize_t ret;
diff --git a/fs/read_write.c b/fs/read_write.c
index 6d5d8ff..8201e60 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -731,10 +731,14 @@ SYSCALL_DEFINE3(writev, unsigned long, fd, const struct iovec __user *, vec,
 	return ret;
 }
 
+#define HALF_LONG (BITS_PER_LONG / 2)
+
 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen, u32, pos_high, u32, pos_low)
+		unsigned long, vlen, unsigned long, pos_high, unsigned long, pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	/* gcc optimizes away the shifts for
+	 * sizeof(pos_high) == sizeof(pos), i.e. on 64bit archs */
+	loff_t pos = (((loff_t)pos_high << HALF_LONG) << HALF_LONG) | pos_low;
 	struct file *file;
 	ssize_t ret = -EBADF;
 	int fput_needed;
@@ -757,9 +761,9 @@ SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
 }
 
 SYSCALL_DEFINE5(pwritev, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen, u32, pos_high, u32, pos_low)
+		unsigned long, vlen, unsigned long, pos_high, unsigned long, pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = (((loff_t)pos_high << HALF_LONG) << HALF_LONG) | pos_low;
 	struct file *file;
 	ssize_t ret = -EBADF;
 	int fput_needed;
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 9723edd..065944e 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -193,7 +193,7 @@ asmlinkage ssize_t compat_sys_writev(unsigned long fd,
 		const struct compat_iovec __user *vec, unsigned long vlen);
 asmlinkage ssize_t compat_sys_preadv(unsigned long fd,
 		const struct compat_iovec __user *vec,
-		unsigned long vlen, u32 pos_high, u32 pos_low);
+		unsigned long vlen, unsigned long pos_high, unsigned long pos_low);
 asmlinkage ssize_t compat_sys_pwritev(unsigned long fd,
 		const struct compat_iovec __user *vec,
 		unsigned long vlen, u32 pos_high, u32 pos_low);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b299a82..25e60b0 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -462,9 +462,9 @@ asmlinkage long sys_pread64(unsigned int fd, char __user *buf,
 asmlinkage long sys_pwrite64(unsigned int fd, const char __user *buf,
 			     size_t count, loff_t pos);
 asmlinkage long sys_preadv(unsigned long fd, const struct iovec __user *vec,
-			   unsigned long vlen, u32 pos_high, u32 pos_low);
+			   unsigned long vlen, unsigned long pos_high, unsigned long pos_low);
 asmlinkage long sys_pwritev(unsigned long fd, const struct iovec __user *vec,
-			    unsigned long vlen, u32 pos_high, u32 pos_low);
+			    unsigned long vlen, unsigned long pos_high, unsigned long pos_low);
 asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
 asmlinkage long sys_mkdir(const char __user *pathname, int mode);
 asmlinkage long sys_chdir(const char __user *filename);
-- 
1.6.1.3

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]

Gerd Hoffmann wrote:

> Patch attached for comments (builds, but otherwise not tested yet).

New patch, updates compat_sys_writev() which was incomplete in the
previous version.  Now tested on x86_64, both 32 and 64-bit binaries.

cheers,
  Gerd


[-- Attachment #2: 0001-preadv-pwritev-tweak-interface.patch --]
[-- Type: text/plain, Size: 5049 bytes --]

>From fc35c3cee3c3f821c39359fa6b33198d1a0219e6 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date: Fri, 3 Apr 2009 11:08:00 +0200
Subject: [PATCH] preadv/pwritev: tweak interface.

Go from "u32" to "unsigned long" for the pos_{high,low} syscall
arguments.  That way we avoid the 32/32 split for 64bit archs,
and we still side-step the ABI issue with unaligned 64bit args
on a number of 32bit archs.  pos_high on 64bit is unused for
the time being. Some day we might be happy to have it though ;)

Signed-off-by: Gerd Hoffmann <kraxel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/compat.c              |    8 ++++----
 fs/read_write.c          |   12 ++++++++----
 include/linux/compat.h   |    4 ++--
 include/linux/syscalls.h |    4 ++--
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 1c859da..17e1dd1 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1236,9 +1236,9 @@ compat_sys_readv(unsigned long fd, const struct compat_iovec __user *vec,
 
 asmlinkage ssize_t
 compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
-		  unsigned long vlen, u32 pos_high, u32 pos_low)
+		  unsigned long vlen, unsigned long pos_high, unsigned long pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = ((loff_t)pos_high << BITS_PER_COMPAT_LONG) | pos_low;
 	struct file *file;
 	int fput_needed;
 	ssize_t ret;
@@ -1293,9 +1293,9 @@ compat_sys_writev(unsigned long fd, const struct compat_iovec __user *vec,
 
 asmlinkage ssize_t
 compat_sys_pwritev(unsigned long fd, const struct compat_iovec __user *vec,
-		   unsigned long vlen, u32 pos_high, u32 pos_low)
+		   unsigned long vlen, unsigned long pos_high, unsigned long pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = ((loff_t)pos_high << BITS_PER_COMPAT_LONG) | pos_low;
 	struct file *file;
 	int fput_needed;
 	ssize_t ret;
diff --git a/fs/read_write.c b/fs/read_write.c
index 6d5d8ff..8201e60 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -731,10 +731,14 @@ SYSCALL_DEFINE3(writev, unsigned long, fd, const struct iovec __user *, vec,
 	return ret;
 }
 
+#define HALF_LONG (BITS_PER_LONG / 2)
+
 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen, u32, pos_high, u32, pos_low)
+		unsigned long, vlen, unsigned long, pos_high, unsigned long, pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	/* gcc optimizes away the shifts for
+	 * sizeof(pos_high) == sizeof(pos), i.e. on 64bit archs */
+	loff_t pos = (((loff_t)pos_high << HALF_LONG) << HALF_LONG) | pos_low;
 	struct file *file;
 	ssize_t ret = -EBADF;
 	int fput_needed;
@@ -757,9 +761,9 @@ SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
 }
 
 SYSCALL_DEFINE5(pwritev, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen, u32, pos_high, u32, pos_low)
+		unsigned long, vlen, unsigned long, pos_high, unsigned long, pos_low)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = (((loff_t)pos_high << HALF_LONG) << HALF_LONG) | pos_low;
 	struct file *file;
 	ssize_t ret = -EBADF;
 	int fput_needed;
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 9723edd..f4b607e 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -193,10 +193,10 @@ asmlinkage ssize_t compat_sys_writev(unsigned long fd,
 		const struct compat_iovec __user *vec, unsigned long vlen);
 asmlinkage ssize_t compat_sys_preadv(unsigned long fd,
 		const struct compat_iovec __user *vec,
-		unsigned long vlen, u32 pos_high, u32 pos_low);
+		unsigned long vlen, unsigned long pos_high, unsigned long pos_low);
 asmlinkage ssize_t compat_sys_pwritev(unsigned long fd,
 		const struct compat_iovec __user *vec,
-		unsigned long vlen, u32 pos_high, u32 pos_low);
+		unsigned long vlen, unsigned long pos_high, unsigned long pos_low);
 
 int compat_do_execve(char * filename, compat_uptr_t __user *argv,
 	        compat_uptr_t __user *envp, struct pt_regs * regs);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b299a82..25e60b0 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -462,9 +462,9 @@ asmlinkage long sys_pread64(unsigned int fd, char __user *buf,
 asmlinkage long sys_pwrite64(unsigned int fd, const char __user *buf,
 			     size_t count, loff_t pos);
 asmlinkage long sys_preadv(unsigned long fd, const struct iovec __user *vec,
-			   unsigned long vlen, u32 pos_high, u32 pos_low);
+			   unsigned long vlen, unsigned long pos_high, unsigned long pos_low);
 asmlinkage long sys_pwritev(unsigned long fd, const struct iovec __user *vec,
-			    unsigned long vlen, u32 pos_high, u32 pos_low);
+			    unsigned long vlen, unsigned long pos_high, unsigned long pos_low);
 asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
 asmlinkage long sys_mkdir(const char __user *pathname, int mode);
 asmlinkage long sys_chdir(const char __user *filename);
-- 
1.6.1.3

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Linus Torvalds]

On Fri, 3 Apr 2009, Gerd Hoffmann wrote:

> Linus Torvalds wrote:
> > 
> > How married are people to this crazy 2x "32 bits" model?
> 
> The tricky corner case are 32bit compat syscalls on 64bit kernel, where
> a noticable number of archs (IIRC at least ppc, mips and s390) can't
> pass unaligned 64bit values through the syscall abi.  The 32/32 split
> was done because of that.

I really think that the 32-bit interfaces are fine. So I'm literally 
suggesting a patch something like the appended.

Some notes:

 - I changed the order to "low first". Why? Because when we have it like 
   this, the 64-bit system calls now don't use the "pos_high" argument at 
   all, and it makes more sense for the native system call to simply match 
   the user-mode prototype.

   But I don't much _care_ about this particular change. I just made it 
   because it seems more logical to me, and if we're changing the order 
   anyway..

 - I checked the resulting assembly code. At least on x86-64, gcc is 
   indeed smart enough to convert the "shift by half twice" into just a 
   zero, and I get the following code:

        testq   %rcx, %rcx      # pos_l
        js      .L122   #,
        movq    %rcx, -48(%rbp) # pos_l, pos

   which is from the C source

        loff_t pos = pos_from_hilo(pos_h, pos_l);
	...
        if (pos < 0)
                return -EINVAL;

   and the 'pos_h' register isn't even touched. So this does indeed 
   improve code generation. 

 - It _used_ to generate code like

        mov     %r8d, %r8d      # pos_low, pos_low
        salq    $32, %rcx       #, tmp71
        movq    %r8, %rax       # pos_low, pos.386
        orq     %rcx, %rax      # tmp71, pos.386
        js      .L122   #,
        movq    %rax, -48(%rbp) # pos.386, pos

   which isn't _that_ horrible, but I just think the new thing is more 
   sensible. Does it really matter? Probably not. 

(In both code sequences, the actual code had other instructions 
interspersed, so the above code-sequences are just me manually picking out 
the relevant instructions. Maybe I did something wrong).

So I'm not going to commit the suggested patch, but I just personally 
think it makes more sense that way.

Note: in all cases the user code wrapper can again be the same. You can 
just do

	#define HALF_BITS (sizeof(unsigned long)*4)
	__syscall(PWRITEV, fd, iov, count, offset, (offset >> HALF_BITS) >> HALF_BITS);

or something like that (and passing in a zero for the last argument is a 
good idea, even if nobody will ever care - this will be the _least_ of our 
problems if we ever do 128-bit loff_t). So the user call wrapper isn't 
really any more complicated - the HALF_BITS thing may look more 
complicated than just doing a hard-coded '16' there twice, but if you 
understand what it's doing it's actually more logical.

I think.

Comments?

			Linus

---
 fs/compat.c              |    4 ++--
 fs/read_write.c          |   14 ++++++++++----
 include/linux/compat.h   |    4 ++--
 include/linux/syscalls.h |    4 ++--
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 1c859da..3f84d5f 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1236,7 +1236,7 @@ compat_sys_readv(unsigned long fd, const struct compat_iovec __user *vec,
 
 asmlinkage ssize_t
 compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
-		  unsigned long vlen, u32 pos_high, u32 pos_low)
+		  unsigned long vlen, u32 pos_low, u32 pos_high)
 {
 	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
 	struct file *file;
@@ -1293,7 +1293,7 @@ compat_sys_writev(unsigned long fd, const struct compat_iovec __user *vec,
 
 asmlinkage ssize_t
 compat_sys_pwritev(unsigned long fd, const struct compat_iovec __user *vec,
-		   unsigned long vlen, u32 pos_high, u32 pos_low)
+		   unsigned long vlen, u32 pos_low, u32 pos_high)
 {
 	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
 	struct file *file;
diff --git a/fs/read_write.c b/fs/read_write.c
index 6d5d8ff..9ac8b6f 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -731,10 +731,16 @@ SYSCALL_DEFINE3(writev, unsigned long, fd, const struct iovec __user *, vec,
 	return ret;
 }
 
+static inline loff_t pos_from_hilo(unsigned long high, unsigned long low)
+{
+#define HALF_LONG_BITS (BITS_PER_LONG / 2)
+	return ((high << HALF_LONG_BITS) << HALF_LONG_BITS) | low;
+}
+
 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen, u32, pos_high, u32, pos_low)
+		unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = pos_from_hilo(pos_h, pos_l);
 	struct file *file;
 	ssize_t ret = -EBADF;
 	int fput_needed;
@@ -757,9 +763,9 @@ SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
 }
 
 SYSCALL_DEFINE5(pwritev, unsigned long, fd, const struct iovec __user *, vec,
-		unsigned long, vlen, u32, pos_high, u32, pos_low)
+		unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
 {
-	loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+	loff_t pos = pos_from_hilo(pos_h, pos_l);
 	struct file *file;
 	ssize_t ret = -EBADF;
 	int fput_needed;
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 9723edd..f2ded21 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -193,10 +193,10 @@ asmlinkage ssize_t compat_sys_writev(unsigned long fd,
 		const struct compat_iovec __user *vec, unsigned long vlen);
 asmlinkage ssize_t compat_sys_preadv(unsigned long fd,
 		const struct compat_iovec __user *vec,
-		unsigned long vlen, u32 pos_high, u32 pos_low);
+		unsigned long vlen, u32 pos_low, u32 pos_high);
 asmlinkage ssize_t compat_sys_pwritev(unsigned long fd,
 		const struct compat_iovec __user *vec,
-		unsigned long vlen, u32 pos_high, u32 pos_low);
+		unsigned long vlen, u32 pos_low, u32 pos_high);
 
 int compat_do_execve(char * filename, compat_uptr_t __user *argv,
 	        compat_uptr_t __user *envp, struct pt_regs * regs);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b299a82..18771ca 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -462,9 +462,9 @@ asmlinkage long sys_pread64(unsigned int fd, char __user *buf,
 asmlinkage long sys_pwrite64(unsigned int fd, const char __user *buf,
 			     size_t count, loff_t pos);
 asmlinkage long sys_preadv(unsigned long fd, const struct iovec __user *vec,
-			   unsigned long vlen, u32 pos_high, u32 pos_low);
+			   unsigned long vlen, unsigned long pos_l, unsigned long pos_h);
 asmlinkage long sys_pwritev(unsigned long fd, const struct iovec __user *vec,
-			    unsigned long vlen, u32 pos_high, u32 pos_low);
+			    unsigned long vlen, unsigned long pos_l, unsigned long pos_h);
 asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
 asmlinkage long sys_mkdir(const char __user *pathname, int mode);
 asmlinkage long sys_chdir(const char __user *filename);

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Gerd Hoffmann]

Linus Torvalds wrote:
>  - I changed the order to "low first". Why? Because when we have it like 
>    this, the 64-bit system calls now don't use the "pos_high" argument at 
>    all, and it makes more sense for the native system call to simply match 
>    the user-mode prototype.

Good idea.

> problems if we ever do 128-bit loff_t). So the user call wrapper isn't 
> really any more complicated - the HALF_BITS thing may look more 
> complicated than just doing a hard-coded '16' there twice, but if you 
> understand what it's doing it's actually more logical.

The whole point of the HALF_BITS thing is just to kill the gcc "hey your
shift is greater than bitsize of $type" warning, right?

> I think.
> 
> Comments?

I really like it.  Except for the low/high ordering it is basically
identical to the patch I've mailed.

While hashing out all the issues I've learned alot about arch specific
ABIs.  I can't see possible trouble spots and don't expect vetos from
arch maintainers.

cheers,
  Gerd

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Linus Torvalds]

On Fri, 3 Apr 2009, Gerd Hoffmann wrote:
> 
> The whole point of the HALF_BITS thing is just to kill the gcc "hey your
> shift is greater than bitsize of $type" warning, right?

No, it's literally a corectness issue.

If you do

	offset = ((loff_t) high) << BITS_IN_LONG;

then that is literally undefined behavior in C (if "loff_t" and "long" are 
the same size), and a lot of C compilers will end up just optimizing it 
away entirely.

Even if they don't optimize it away, and if they actually generate the 
instruction for it, architectures will not actually do what you think they 
do. At least on x86, shift counts are fundamentally limited to the low 
bits, so if you do 

	movl $64,%ecx
	shlq %ecx,reg

then an x86 will literally shift 'reg' by _zero_, because the count is 
masked by 6 bits (or 5, in the 32-bit case).

So literally, if gcc were to generate the obvious (stupid) code, it would 
give the wrong value.

With the double shift, we have well-defined behavior.

> I really like it.  Except for the low/high ordering it is basically
> identical to the patch I've mailed.
> 
> While hashing out all the issues I've learned alot about arch specific
> ABIs.  I can't see possible trouble spots and don't expect vetos from
> arch maintainers.

Ok, a few more ack's, and I can commit it. Anybody disagree?

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Andrew Morton]

On Fri, 3 Apr 2009 08:03:22 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:

> +static inline loff_t pos_from_hilo(unsigned long high, unsigned long low)
> +{
> +#define HALF_LONG_BITS (BITS_PER_LONG / 2)
> +	return ((high << HALF_LONG_BITS) << HALF_LONG_BITS) | low;
> +}

Does C promote the `unsigned long high' beforehand, or will the
intermediate expression overflow?

<does it the old-fashioned way>

y:/home/akpm> cat t.c

#define HALF_LONG_BITS 16

main()
{
        unsigned hi = 0x43;
        unsigned lo = 0x42;
        unsigned long long res;
	
        res = ((hi << 16) << 16) | lo;
        printf("%llx\n", res);
}
y:/home/akpm> gcc -O t.c
y:/home/akpm> ./a.out
42

I think it's wrong on 32-bit?

Also, HALF_LONG_BITS is 32 on 64-bit, so "high" gets shifted to zero. 
It's unclear whether this was deliberate, but either way, it's a sneaky
trick and deserves a code comment!

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[Linus Torvalds]

On Sat, 4 Apr 2009, Andrew Morton wrote:
> 
> Does C promote the `unsigned long high' beforehand, or will the
> intermediate expression overflow?

Thanks, good catch. Will fix. Needs the cast - I'm sure I had it at one 
point, but it got lost..

[ looks around ]

Heh. I had the cast in my original email that wasn't a patch:

        #define HALF_LONG (BITS_IN_LONG / 2)

	offset = (((loff_t)high << HALF_LONG) << HALF_LONG) | low;

but then when I actually turned it into a patch, I had lost it:

	+#define HALF_LONG_BITS (BITS_PER_LONG / 2)
	+       return ((high << HALF_LONG_BITS) << HALF_LONG_BITS) | low;

(but at least I had fixed the "BITS_IN_LONG" to "BITS_PER_LONG").

> I think it's wrong on 32-bit?
> 
> Also, HALF_LONG_BITS is 32 on 64-bit, so "high" gets shifted to zero. 
> It's unclear whether this was deliberate, but either way, it's a sneaky
> trick and deserves a code comment!

It was very much deliberate - the high bits disappear, since we don't have 
128-bit loff_t.

And it's not a sneaky trick, it's totally standard programming. We have 
the same thing in a few other places:

	drivers/net/starfire.c: writel( (np->queue_mem_dma >> 16) >> 16, ioaddr + RxDescQHiAddr);

Notice the ">> 16) >> 16" thing? That's a way to write ">> 32" without 
having the undefined overflow in 32 bits - if we have just a 32-bit dma 
address type, it just turns into "high bits are 0".

		Linus

Re: [patch 143/166] preadv/pwritev: Add preadv and pwritev system calls.

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Grr. That may make sense on 32-bit architectures, but makes no sense 
> what-so-ever on 64-bit ones.
> 
> Why not just say that it's two "unsigned long" arguments, and split it by 
> "sizeof unsigned long". And then, on 64-bit architectures, the high bits 
> can be ignored. You can even get gcc to optimize it all away by doing 
> something like
> 
> 	#define HALF_LONG (BITS_IN_LONG / 2)
> 
> 	offset = (((loff_t)high << HALF_LONG) << HALF_LONG) | low;
> 
> and then gcc should automaticaly notice that shifting up that way is just 
> a fancy way of saying "0", and ignore the high bits.
> 
> How married are people to this crazy 2x "32 bits" model?
> 

I certainly am *not* happy with it, and said so when it was first
posted.  My main complaint with it is that it introduces yet another
special case in the kernel<->userspace ABI, because although we've had
this problem before, it has been hacked around a different way every time.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html