This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] posix: Fix and simplify default p{read,write}v implementation


Currently all architectures but microblaze use wire-up syscall for
p{readv,write}v.  Microblaze still uses the syscall emulation using
sysdep/posix/p{readv,writev}.c and it was reported in some ocasions
[1] [2] that it might have some issues with some linux specific
usage (mainly with O_DIRECT and the alignment requirement).

Although it is not an issue for virtually all the system, this
patch refactors the sysdeps/posix p{read,write}v syscall to avoid
such issue (by using posix_memalign on the buffer used on
p{read,write} call) and by refactoring it common files to avoid
the need check on defines to correct set the alias and internal
symbols.

Checked on microblaze-linux-gnu check with run-built-tests=no and
by using the sysdeps/posix implementation on x86_64-linux-gnu (just
for sanity test where it shown no regression).

	* sysdeps/posix/preadv.c: Use sysdeps/posix/preadv_common.c.
	* sysdeps/posix/preadv64.c: Likewise.
	* sysdeps/unix/sysv/linux/preadv.c: Likewise.
	* sysdeps/unix/sysv/linux/preadv64.c: Likewise.
	* sysdeps/posix/pwritev.c: Use sysdeps/posix/pwritev_common.c.
	* sysdeps/posix/pwritev64.c: Likewise.
	* sysdeps/unix/sysv/linux/pwritev.c: Likewise.
	* sysdeps/unix/sysv/linux/pwritev64.c: Likewise.
	* sysdeps/posix/preadv_common.c: New file.
	* sysdeps/posix/pwritev_common.c: Likewise.

[1] http://www.mail-archive.com/qemu-devel@nongnu.org/msg25282.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=563103#c8
---
 ChangeLog                           | 13 ++++++
 sysdeps/posix/preadv.c              | 88 +++----------------------------------
 sysdeps/posix/preadv64.c            | 30 ++++++++++---
 sysdeps/posix/preadv_common.c       | 81 ++++++++++++++++++++++++++++++++++
 sysdeps/posix/pwritev.c             | 76 +++-----------------------------
 sysdeps/posix/pwritev64.c           | 30 ++++++++++---
 sysdeps/posix/pwritev_common.c      | 70 +++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/preadv.c    |  2 +-
 sysdeps/unix/sysv/linux/preadv64.c  |  2 +-
 sysdeps/unix/sysv/linux/pwritev.c   |  2 +-
 sysdeps/unix/sysv/linux/pwritev64.c |  2 +-
 11 files changed, 226 insertions(+), 170 deletions(-)
 create mode 100644 sysdeps/posix/preadv_common.c
 create mode 100644 sysdeps/posix/pwritev_common.c

diff --git a/sysdeps/posix/preadv.c b/sysdeps/posix/preadv.c
index 7539bf8..d311205 100644
--- a/sysdeps/posix/preadv.c
+++ b/sysdeps/posix/preadv.c
@@ -15,93 +15,15 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <string.h>
-#include <limits.h>
-#include <stdbool.h>
-#include <sys/param.h>
-#if __WORDSIZE == 64 && !defined PREADV
-/* Hide the preadv64 declaration.  */
-# define preadv64 __redirect_preadv64
-#endif
-#include <sys/uio.h>
-#include <bits/wordsize.h>
+#include <sys/types.h>
+
+#ifndef __OFF_T_MATCHES_OFF64_T
 
-#ifndef PREADV
 # define PREADV preadv
 # define PREAD __pread
 # define OFF_T off_t
-#endif
-
-
-static void
-ifree (char **ptrp)
-{
-  free (*ptrp);
-}
-
-
-/* Read data from file descriptor FD at the given position OFFSET
-   without change the file pointer, and put the result in the buffers
-   described by VECTOR, which is a vector of COUNT 'struct iovec's.
-   The buffers are filled in the order specified.  Operates just like
-   'pread' (see <unistd.h>) except that data are put in VECTOR instead
-   of a contiguous buffer.  */
-ssize_t
-PREADV (int fd, const struct iovec *vector, int count, OFF_T offset)
-{
-  /* Find the total number of bytes to be read.  */
-  size_t bytes = 0;
-  for (int i = 0; i < count; ++i)
-    {
-      /* Check for ssize_t overflow.  */
-      if (SSIZE_MAX - bytes < vector[i].iov_len)
-	{
-	  __set_errno (EINVAL);
-	  return -1;
-	}
-      bytes += vector[i].iov_len;
-    }
-
-  /* Allocate a temporary buffer to hold the data.  We should normally
-     use alloca since it's faster and does not require synchronization
-     with other threads.  But we cannot if the amount of memory
-     required is too large.  */
-  char *buffer;
-  char *malloced_buffer __attribute__ ((__cleanup__ (ifree))) = NULL;
-  if (__libc_use_alloca (bytes))
-    buffer = (char *) __alloca (bytes);
-  else
-    {
-      malloced_buffer = buffer = (char *) malloc (bytes);
-      if (buffer == NULL)
-	return -1;
-    }
-
-  /* Read the data.  */
-  ssize_t bytes_read = PREAD (fd, buffer, bytes, offset);
-  if (bytes_read < 0)
-    return -1;
-
-  /* Copy the data from BUFFER into the memory specified by VECTOR.  */
-  bytes = bytes_read;
-  for (int i = 0; i < count; ++i)
-    {
-      size_t copy = MIN (vector[i].iov_len, bytes);
-
-      (void) memcpy ((void *) vector[i].iov_base, (void *) buffer, copy);
+# include <sysdeps/posix/preadv_common.c>
 
-      buffer += copy;
-      bytes -= copy;
-      if (bytes == 0)
-	break;
-    }
+libc_hidden_def (preadv)
 
-  return bytes_read;
-}
-#if __WORDSIZE == 64 && defined preadv64
-# undef preadv64
-strong_alias (preadv, preadv64)
 #endif
diff --git a/sysdeps/posix/preadv64.c b/sysdeps/posix/preadv64.c
index 1986223..631c7f4 100644
--- a/sysdeps/posix/preadv64.c
+++ b/sysdeps/posix/preadv64.c
@@ -1,9 +1,27 @@
-#include <bits/wordsize.h>
+/* Copyright (C) 2009-2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
 
-#if __WORDSIZE == 32
-# define PREADV preadv64
-# define PREAD __pread64
-# define OFF_T off64_t
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
 
-# include "preadv.c"
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#define PREADV preadv64
+#define PREAD __pread64
+#define OFF_T off64_t
+#include <sysdeps/posix/preadv_common.c>
+
+libc_hidden_def (preadv64)
+#ifdef __OFF_T_MATCHES_OFF64_T
+strong_alias (preadv64, preadv)
+libc_hidden_def (preadv)
 #endif
diff --git a/sysdeps/posix/preadv_common.c b/sysdeps/posix/preadv_common.c
new file mode 100644
index 0000000..e17fe20
--- /dev/null
+++ b/sysdeps/posix/preadv_common.c
@@ -0,0 +1,81 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+#include <sys/uio.h>
+#include <sys/param.h>
+#include <errno.h>
+#include <malloc.h>
+
+#include <ldsodefs.h>
+
+/* Read data from file descriptor FD at the given position OFFSET
+   without change the file pointer, and put the result in the buffers
+   described by VECTOR, which is a vector of COUNT 'struct iovec's.
+   The buffers are filled in the order specified.  Operates just like
+   'pread' (see <unistd.h>) except that data are put in VECTOR instead
+   of a contiguous buffer.  */
+ssize_t
+PREADV (int fd, const struct iovec *vector, int count, OFF_T offset)
+{
+  /* Find the total number of bytes to be read.  */
+  size_t bytes = 0;
+  for (int i = 0; i < count; ++i)
+    {
+      /* Check for ssize_t overflow.  */
+      if (SSIZE_MAX - bytes < vector[i].iov_len)
+	{
+	  __set_errno (EINVAL);
+	  return -1;
+	}
+      bytes += vector[i].iov_len;
+    }
+
+  /* Allocate a temporary buffer to hold the data.  It could be done with a
+     stack allocation, but due limitations on some system (Linux with
+     O_DIRECT) it aligns the buffer to pagesize.  An possible optimization
+     would be querying if the syscall would impose any alignment constraint,
+     but 1. it is system specific (not meant in generic implementation), and
+     2. it would make the implementation more complex, and 3. it will require
+     another syscall (fcntl).  */
+  void *buffer = NULL;
+  if (__posix_memalign (&buffer, GLRO(dl_pagesize), bytes) != 0)
+    return -1;
+
+  ssize_t bytes_read = PREAD (fd, buffer, bytes, offset);
+  if (bytes_read < 0)
+    goto end;
+
+  /* Copy the data from BUFFER into the memory specified by VECTOR.  */
+  bytes = bytes_read;
+  void *buf = buffer;
+  for (int i = 0; i < count; ++i)
+    {
+      size_t copy = MIN (vector[i].iov_len, bytes);
+
+      memcpy (vector[i].iov_base, buf, copy);
+
+      buf += copy;
+      bytes -= copy;
+      if (bytes == 0)
+	break;
+    }
+
+end:
+  free (buffer);
+  return bytes_read;
+}
diff --git a/sysdeps/posix/pwritev.c b/sysdeps/posix/pwritev.c
index 57e641b..06bfd3d 100644
--- a/sysdeps/posix/pwritev.c
+++ b/sysdeps/posix/pwritev.c
@@ -15,81 +15,15 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <string.h>
-#include <limits.h>
-#include <stdbool.h>
-#include <sys/param.h>
-#if __WORDSIZE == 64 && !defined PWRITEV
-/* Hide the pwritev64 declaration.  */
-# define pwritev64 __redirect_pwritev64
-#endif
-#include <sys/uio.h>
-#include <bits/wordsize.h>
+#include <sys/types.h>
+
+#ifndef __OFF_T_MATCHES_OFF64_T
 
-#ifndef PWRITEV
 # define PWRITEV pwritev
 # define PWRITE __pwrite
 # define OFF_T off_t
-#endif
-
-
-static void
-ifree (char **ptrp)
-{
-  free (*ptrp);
-}
-
-
-/* Write data pointed by the buffers described by IOVEC, which is a
-   vector of COUNT 'struct iovec's, to file descriptor FD at the given
-   position OFFSET without change the file pointer.  The data is
-   written in the order specified.  Operates just like 'write' (see
-   <unistd.h>) except that the data are taken from IOVEC instead of a
-   contiguous buffer.  */
-ssize_t
-PWRITEV (int fd, const struct iovec *vector, int count, OFF_T offset)
-{
-  /* Find the total number of bytes to be read.  */
-  size_t bytes = 0;
-  for (int i = 0; i < count; ++i)
-    {
-      /* Check for ssize_t overflow.  */
-      if (SSIZE_MAX - bytes < vector[i].iov_len)
-	{
-	  __set_errno (EINVAL);
-	  return -1;
-	}
-      bytes += vector[i].iov_len;
-    }
-
-  /* Allocate a temporary buffer to hold the data.  We should normally
-     use alloca since it's faster and does not require synchronization
-     with other threads.  But we cannot if the amount of memory
-     required is too large.  */
-  char *buffer;
-  char *malloced_buffer __attribute__ ((__cleanup__ (ifree))) = NULL;
-  if (__libc_use_alloca (bytes))
-    buffer = (char *) __alloca (bytes);
-  else
-    {
-      malloced_buffer = buffer = (char *) malloc (bytes);
-      if (buffer == NULL)
-	return -1;
-    }
+# include <sysdeps/posix/pwritev_common.c>
 
-  /* Copy the data from BUFFER into the memory specified by VECTOR.  */
-  char *ptr = buffer;
-  for (int i = 0; i < count; ++i)
-    ptr = __mempcpy ((void *) ptr, (void *) vector[i].iov_base,
-		     vector[i].iov_len);
+libc_hidden_def (pwritev)
 
-  /* Write the data.  */
-  return PWRITE (fd, buffer, bytes, offset);
-}
-#if __WORDSIZE == 64 && defined pwritev64
-# undef pwritev64
-strong_alias (pwritev, pwritev64)
 #endif
diff --git a/sysdeps/posix/pwritev64.c b/sysdeps/posix/pwritev64.c
index 4948d2e..7faae1d 100644
--- a/sysdeps/posix/pwritev64.c
+++ b/sysdeps/posix/pwritev64.c
@@ -1,9 +1,27 @@
-#include <bits/wordsize.h>
+/* Copyright (C) 2009-2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
 
-#if __WORDSIZE == 32
-# define PWRITEV pwritev64
-# define PWRITE __pwrite64
-# define OFF_T off64_t
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
 
-# include "pwritev.c"
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#define PWRITEV pwritev64
+#define PWRITE __pwrite64
+#define OFF_T off64_t
+#include <sysdeps/posix/pwritev_common.c>
+
+libc_hidden_def (pwritev64)
+#ifdef __OFF_T_MATCHES_OFF64_T
+strong_alias (pwritev64, pwritev)
+libc_hidden_def (pwritev)
 #endif
diff --git a/sysdeps/posix/pwritev_common.c b/sysdeps/posix/pwritev_common.c
new file mode 100644
index 0000000..59e19a0
--- /dev/null
+++ b/sysdeps/posix/pwritev_common.c
@@ -0,0 +1,70 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+#include <sys/uio.h>
+#include <sys/param.h>
+#include <errno.h>
+#include <malloc.h>
+
+#include <ldsodefs.h>
+
+/* Write data pointed by the buffers described by IOVEC, which is a
+   vector of COUNT 'struct iovec's, to file descriptor FD at the given
+   position OFFSET without change the file pointer.  The data is
+   written in the order specified.  Operates just like 'write' (see
+   <unistd.h>) except that the data are taken from IOVEC instead of a
+   contiguous buffer.  */
+ssize_t
+PWRITEV (int fd, const struct iovec *vector, int count, OFF_T offset)
+{
+  /* Find the total number of bytes to be read.  */
+  size_t bytes = 0;
+  for (int i = 0; i < count; ++i)
+    {
+      /* Check for ssize_t overflow.  */
+      if (SSIZE_MAX - bytes < vector[i].iov_len)
+	{
+	  __set_errno (EINVAL);
+	  return -1;
+	}
+      bytes += vector[i].iov_len;
+    }
+
+  /* Allocate a temporary buffer to hold the data.  It could be done with a
+     stack allocation, but due limitations on some system (Linux with
+     O_DIRECT) it aligns the buffer to pagesize.  An possible optimization
+     would be querying if the syscall would impose any alignment constraint,
+     but 1. it is system specific (not meant in generic implementation), and
+     2. it would make the implementation more complex, and 3. it will require
+     another syscall (fcntl).  */
+  void *buffer = NULL;
+  if (__posix_memalign (&buffer, GLRO(dl_pagesize), bytes) != 0)
+    return -1;
+
+  /* Copy the data from BUFFER into the memory specified by VECTOR.  */
+  char *ptr = buffer;
+  for (int i = 0; i < count; ++i)
+    ptr = __mempcpy ((void *) ptr, (void *) vector[i].iov_base,
+		     vector[i].iov_len);
+
+  ssize_t ret = PWRITE (fd, buffer, bytes, offset);
+
+  free (buffer);
+
+  return ret;
+}
diff --git a/sysdeps/unix/sysv/linux/preadv.c b/sysdeps/unix/sysv/linux/preadv.c
index ccfe763..7d971cc 100644
--- a/sysdeps/unix/sysv/linux/preadv.c
+++ b/sysdeps/unix/sysv/linux/preadv.c
@@ -48,6 +48,6 @@ preadv (int fd, const struct iovec *vector, int count, off_t offset)
 #  define PREADV static internal_function __atomic_preadv_replacement
 #  define PREAD __pread
 #  define OFF_T off_t
-#  include <sysdeps/posix/preadv.c>
+#  include <sysdeps/posix/preadv_common.c>
 # endif /* __ASSUME_PREADV  */
 #endif
diff --git a/sysdeps/unix/sysv/linux/preadv64.c b/sysdeps/unix/sysv/linux/preadv64.c
index 979db95..66daa74 100644
--- a/sysdeps/unix/sysv/linux/preadv64.c
+++ b/sysdeps/unix/sysv/linux/preadv64.c
@@ -46,7 +46,7 @@ preadv64 (int fd, const struct iovec *vector, int count, off64_t offset)
 # define PREADV static internal_function __atomic_preadv64_replacement
 # define PREAD __pread64
 # define OFF_T off64_t
-# include <sysdeps/posix/preadv.c>
+# include <sysdeps/posix/preadv_common.c>
 #endif
 
 #ifdef __OFF_T_MATCHES_OFF64_T
diff --git a/sysdeps/unix/sysv/linux/pwritev.c b/sysdeps/unix/sysv/linux/pwritev.c
index 2789943..ce02996 100644
--- a/sysdeps/unix/sysv/linux/pwritev.c
+++ b/sysdeps/unix/sysv/linux/pwritev.c
@@ -48,6 +48,6 @@ pwritev (int fd, const struct iovec *vector, int count, off_t offset)
 #  define PWRITEV static internal_function __atomic_pwritev_replacement
 #  define PWRITE __pwrite
 #  define OFF_T off_t
-#  include <sysdeps/posix/pwritev.c>
+#  include <sysdeps/posix/pwritev_common.c>
 # endif /* __ASSUME_PREADV  */
 #endif
diff --git a/sysdeps/unix/sysv/linux/pwritev64.c b/sysdeps/unix/sysv/linux/pwritev64.c
index 1e3a36c..45fb90b 100644
--- a/sysdeps/unix/sysv/linux/pwritev64.c
+++ b/sysdeps/unix/sysv/linux/pwritev64.c
@@ -46,7 +46,7 @@ pwritev64 (int fd, const struct iovec *vector, int count, off64_t offset)
 # define PWRITEV static internal_function __atomic_pwritev64_replacement
 # define PWRITE __pwrite64
 # define OFF_T off64_t
-# include <sysdeps/posix/pwritev.c>
+# include <sysdeps/posix/pwritev_common.c>
 #endif
 
 #ifdef __OFF_T_MATCHES_OFF64_T
-- 
2.7.4


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]