From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17712 invoked by alias); 15 Jun 2016 13:45:48 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 17687 invoked by uid 89); 15 Jun 2016 13:45:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=sk:__ASSUM, sk:__assum, 995, personal X-HELO: mail-yw0-f181.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=kKbDLy5ewrmULdCsxnCY0bb20RCVO8ITJPDkTwOGEus=; b=Au6aWfStt0v6C9UvLiDM2CSc89o2ElojriUKA7S9ZghCNiP7WQ3tvDuGQrm304KBA2 uyIuSUuB8LtcsJOa4EjYa0UNkV591SOUB7Sg/Qf/vqRKaFtt4tJ2OtDC+hC1eOkSFpR+ s73ZV3MxAWZ0YIoI0U1zznO4TJ/+icAP+7UG3lL6gZCVEjTc2yZJvRcdAcp17uwrQt++ j+SUMeEQHKrZ1MmSnkwiDNbZpMpbb4GIqBQo9+0mNlBaIo6f09uOuM6/9xwU3IhsRrPv RoU/VaHbTXjEBYyVxjLx5FjB86gbRgOT6dz1Wnm88dQEGwRJMIu8r3D5UDUMfGDe2pxs afpg== X-Gm-Message-State: ALyK8tIqXuKCxuXcOwGdbVI2W1OjLMzzvrXNMQ+DeQMBYFjrQhYH4fX2WPfJeTFeWQXR0IfR X-Received: by 10.37.209.75 with SMTP id i72mr4980172ybg.142.1465998334967; Wed, 15 Jun 2016 06:45:34 -0700 (PDT) Subject: Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation To: libc-alpha@sourceware.org References: <1465941275-3459-1-git-send-email-adhemerval.zanella@linaro.org> <20160615053714.GM4053@vapier.lan> From: Adhemerval Zanella Message-ID: <57615BFB.2050101@linaro.org> Date: Wed, 15 Jun 2016 13:45:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160615053714.GM4053@vapier.lan> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-06/txt/msg00589.txt.bz2 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 :(