| Message ID | 1511455130-19179-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | c45d78aac47db08bc8ea7641c5330cccaecd3ddb |
| Headers | show |
| Series | posix: Fix generic p{read,write}v buffer allocation (BZ#22457) | expand |
* Adhemerval Zanella: > + size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize)); > + void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > + if (__glibc_unlikely (buffer == MAP_FAILED) > + || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize)))) > return -1; I don't think POSIX requires that the length of the mapping is a multiple of the page size. You could probably drop the alignment check, too, because I don't really see any reason why the alignment requirement would be related to the page size.
On 23/11/2017 17:08, Florian Weimer wrote: > * Adhemerval Zanella: > >> + size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize)); >> + void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); >> + if (__glibc_unlikely (buffer == MAP_FAILED) >> + || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize)))) >> return -1; > > I don't think POSIX requires that the length of the mapping is a > multiple of the page size. You could probably drop the alignment > check, too, because I don't really see any reason why the alignment > requirement would be related to the page size. > Indeed we do not need to align to page size, however the alignment requirement was the motivation to actually change its implementation (check commit message for c79a72aa5cb8357c216a71015c7448a9259c8531).
* Adhemerval Zanella: > On 23/11/2017 17:08, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> + size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize)); >>> + void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE, >>> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); >>> + if (__glibc_unlikely (buffer == MAP_FAILED) >>> + || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize)))) >>> return -1; >> >> I don't think POSIX requires that the length of the mapping is a >> multiple of the page size. You could probably drop the alignment >> check, too, because I don't really see any reason why the alignment >> requirement would be related to the page size. > Indeed we do not need to align to page size, however the alignment > requirement was the motivation to actually change its implementation > (check commit message for c79a72aa5cb8357c216a71015c7448a9259c8531). I meant that there is no way to query the alignment required for O_DIRECT, and that page size alignment might be insufficient. Some file systems hard-code a 4K alignment requirement, and microblaze has at least 4K pages, so we should be good. But it feels strange to write a check against the wrong constant.
On 24/11/2017 01:58, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 23/11/2017 17:08, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> + size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize)); >>>> + void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE, >>>> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); >>>> + if (__glibc_unlikely (buffer == MAP_FAILED) >>>> + || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize)))) >>>> return -1; >>> >>> I don't think POSIX requires that the length of the mapping is a >>> multiple of the page size. You could probably drop the alignment >>> check, too, because I don't really see any reason why the alignment >>> requirement would be related to the page size. > >> Indeed we do not need to align to page size, however the alignment >> requirement was the motivation to actually change its implementation >> (check commit message for c79a72aa5cb8357c216a71015c7448a9259c8531). > > I meant that there is no way to query the alignment required for > O_DIRECT, and that page size alignment might be insufficient. Some > file systems hard-code a 4K alignment requirement, and microblaze has > at least 4K pages, so we should be good. But it feels strange to > write a check against the wrong constant. > Alright, looks like I am over-enginering it. I will remove the check as well.
diff --git a/include/libc-pointer-arith.h b/include/libc-pointer-arith.h index 715cbc1..c918023 100644 --- a/include/libc-pointer-arith.h +++ b/include/libc-pointer-arith.h @@ -57,4 +57,8 @@ #define PTR_ALIGN_UP(base, size) \ ((__typeof__ (base)) ALIGN_UP ((uintptr_t) (base), (size))) +/* Check if base is aligned to boundary. */ +#define PTR_IS_ALIGNED(base, boundary) \ + (((uintptr_t) (base) & ((uintptr_t) ((boundary)) - 1)) == 0) + #endif diff --git a/sysdeps/posix/preadv_common.c b/sysdeps/posix/preadv_common.c index 37efdc0..099c37b 100644 --- a/sysdeps/posix/preadv_common.c +++ b/sysdeps/posix/preadv_common.c @@ -24,6 +24,7 @@ #include <malloc.h> #include <ldsodefs.h> +#include <libc-pointer-arith.h> /* Read data from file descriptor FD at the given position OFFSET without change the file pointer, and put the result in the buffers @@ -54,8 +55,11 @@ PREADV (int fd, const struct iovec *vector, int count, OFF_T offset) 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) + size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize)); + void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (__glibc_unlikely (buffer == MAP_FAILED) + || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize)))) return -1; ssize_t bytes_read = PREAD (fd, buffer, bytes, offset); @@ -78,6 +82,6 @@ PREADV (int fd, const struct iovec *vector, int count, OFF_T offset) } end: - free (buffer); + __munmap (buffer, mmap_size); return bytes_read; } diff --git a/sysdeps/posix/pwritev_common.c b/sysdeps/posix/pwritev_common.c index 0383065..610c54e 100644 --- a/sysdeps/posix/pwritev_common.c +++ b/sysdeps/posix/pwritev_common.c @@ -24,6 +24,7 @@ #include <malloc.h> #include <ldsodefs.h> +#include <libc-pointer-arith.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 @@ -54,8 +55,11 @@ PWRITEV (int fd, const struct iovec *vector, int count, OFF_T offset) 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) + size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize)); + void *buffer = __mmap (NULL, mmap_size, PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (__glibc_unlikely (buffer == MAP_FAILED) + || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize)))) return -1; /* Copy the data from BUFFER into the memory specified by VECTOR. */ @@ -66,7 +70,7 @@ PWRITEV (int fd, const struct iovec *vector, int count, OFF_T offset) ssize_t ret = PWRITE (fd, buffer, bytes, offset); - free (buffer); + __munmap (buffer, mmap_size); return ret; }