From patchwork Thu May 11 19:30:07 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 20415 Received: (qmail 59717 invoked by alias); 11 May 2017 19:30:45 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 59321 invoked by uid 89); 11 May 2017 19:30:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=sk:www.mai, sk:wwwmai X-HELO: mail-qk0-f178.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=sEglSExlaKmo2/QI+HfRZBMm7cQWPmGWpfWKLBULti4=; b=AdFnp6wM7sVKuNuRuWvi7kc3PUvMrHV7s7OEpB3WYRi0TEDC3lFLMZQ+ZQWdJa2WhP ni9grW4eBFbtigvrqx9jxAt3Fa00mpBj67R6vi3gTSdFZVGvQQT094YqRpkyNy96tFFV DdL4qw3ieZP5KhJik+g8SXHib9ed1ZJn9ZLOJ2Y7sxVotjJ87WcPDVIZYCHC1LOSzATp cLO2hJtH0m11/cmt53h5k8g1LjKLIMGhg4BVti1fKvH+Hfc7MHyTAYNcjNluD1U+KKIu F961M1YfeFyVJUv6r/EvlPf3uZ9RWyuyyun9wmUm5Ix6wIujnVNJPHbf7vq/T32IrquX oxiQ== X-Gm-Message-State: AODbwcC9p3y7PX1SmTENSzFJdFprw0sH5adn6/0e6TzGKEpwh53HPT8Z hqG/34SYdoakvqya09bTAA== X-Received: by 10.55.178.132 with SMTP id b126mr133845qkf.315.1494531014813; Thu, 11 May 2017 12:30:14 -0700 (PDT) From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH] posix: Fix and simplify default p{read, write}v implementation Date: Thu, 11 May 2017 16:30:07 -0300 Message-Id: <1494531007-8302-1-git-send-email-adhemerval.zanella@linaro.org> 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 . */ -#include -#include -#include -#include -#include -#include -#include -#if __WORDSIZE == 64 && !defined PREADV -/* Hide the preadv64 declaration. */ -# define preadv64 __redirect_preadv64 -#endif -#include -#include +#include + +#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 ) 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 - 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 +/* 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 + . */ + +#define PREADV preadv64 +#define PREAD __pread64 +#define OFF_T off64_t +#include + +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 + . */ + +#include +#include +#include +#include +#include + +#include + +/* 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 ) 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 . */ -#include -#include -#include -#include -#include -#include -#include -#if __WORDSIZE == 64 && !defined PWRITEV -/* Hide the pwritev64 declaration. */ -# define pwritev64 __redirect_pwritev64 -#endif -#include -#include +#include + +#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 - ) 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 - /* 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 +/* 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 + . */ + +#define PWRITEV pwritev64 +#define PWRITE __pwrite64 +#define OFF_T off64_t +#include + +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 + . */ + +#include +#include +#include +#include +#include + +#include + +/* 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 + ) 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 +# include # 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 +# include #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 +# include # 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 +# include #endif #ifdef __OFF_T_MATCHES_OFF64_T