This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Wed, 15 Jun 2016 10:45:31 -0300
- Subject: Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation
- Authentication-results: sourceware.org; auth=none
- References: <1465941275-3459-1-git-send-email-adhemerval dot zanella at linaro dot org> <20160615053714 dot GM4053 at vapier dot lan>
On 15/06/2016 02:37, Mike Frysinger wrote:
> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>> This patch fixes the p{readv,writev}{64} consolidation implementation
>> from commits 4e77815 and af5fdf5. Different from pread/pwrite
>> implementation, preadv/pwritev implementation does not require
>> __ALIGNMENT_ARG because kernel syscall prototypes define
>> the high and low part of the off_t, if it is the case, directly
>> (different from pread/pwrite where the architecture ABI for passing
>> 64-bit values must be in consideration for passsing the arguments).
>
> i had looked at that specifically but thought it ok because the old code
> was using the alignment arg. was the old code broken too ?
>
> this is what the preadv code looked like:
> -ssize_t
> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
> -{
> - assert (sizeof (offset) == 4);
> - return SYSCALL_CANCEL (preadv, fd,
> - vector, count, __ALIGNMENT_ARG
> - __LONG_LONG_PAIR (offset >> 31, offset));
> -}
>
> although i guess this isn't too surprising as this code was in the
> generic sysdeps dir which currently doesn't have as many users as
> we wish it did :).
I though it too, but before the consolidation patch only nios2 and tile 32-bits
used the generic preadv.c implementation. And only tile 32-bits defines
__ASSUME_ALIGNED_REGISTER_PAIRS (so __ALIGNMENT_ARG will pad the argument with 0).
However since the syscall is defined as in linux source:
fs/read_write.c:
991 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
992 unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
993 {
994 loff_t pos = pos_from_hilo(pos_h, pos_l);
995
996 return do_preadv(fd, vec, vlen, pos, 0);
997 }
The idea is not really to align the argument to zero pass, but rather to split
the possible 64-bits argument in high and low as required (as the default
implementation was doing [2]). On tile, it is working because the preadv.c
offset is 32-bits and thus the high word is indeed zero, but it is passing
one superfluous argument. Also my understanding is this generic implementation
does work correctly in every architecture because __LONG_LONG_PAIR relies
on endianess.
[1] sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c
[2] sysdeps/unix/sysv/linux/preadv.c
>
>> +#define FAIL() \
>> + do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
>> ...
>> + ret = pwritev (temp_fd, iov, 2, 0);
>> + if (ret == -1)
>> + FAIL ();
>
> as a personal stance, never been a big fan of messages that just have a
> line number. when you get reports/logs, you end having to chase down the
> exact same source tree as the reporter.
>
Right, I will change to a more descriptive error message.
> why can't we just assert() everywhere ? we rely on that in tests already
> and we wouldn't have to do any checking otherwise.
> ret = pwritev (temp_fd, iov, 2, 0);
> assert (ret == sizeof buf1 + sizeof buf2);
> -mike
>
As Florian has stated assert writes on stderr :(