Re: [PATCH] Consolidate Linux readahead() implementations

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 23/09/2016 09:44, Yury Norov wrote:
> On Fri, Sep 23, 2016 at 08:08:19AM +0200, Florian Weimer wrote:
>> * Yury Norov:
>>
>>> On Thu, Sep 22, 2016 at 06:36:22PM -0300, Adhemerval Zanella wrote:
>>>> Hi Yury, some comments below.
>>>>
>>>> On 22/09/2016 17:44, Yury Norov wrote:
>>>> I think we can use SYSCALL_LL64 plus __ALIGNMENT_ARG here.  The tricky is to pass
>>>> the correct argument size, since it used by the macro to select the correct 
>>>> arch-dependent one.  This is exactly why I proposed the patch to add
>>>> {INTERNAL,INLINE}_SYSCALL_CALL [1], readahead will be simplified to just:
>>>>
>>>> ssize_t
>>>> __readahead (int fd, off64_t offset, size_t count)
>>>> {
>>>>   return INLINE_SYSCALL_CALL (readahead, fd,
>>>> 			      __ALIGNMENT_ARG SYSCALL_LL64 (offset));
>>>> }
>>>>
>>>> I suggest you to wait the {INTERNAL,INLINE}_SYSCALL_CALL patch to land and
>>>> based this one on it.  I think I addressed most of Florian comments, I just
>>>> need to check if assembly generated using the new macros is the same as
>>>> before (which I expect to).
>>>>
>>>> [1] https://sourceware.org/ml/libc-alpha/2016-09/msg00452.html
>>>
>>> __ALIGNMENT_ARG is controlled by __ASSUME_ALIGNED_REGISTER_PAIRS,
>>> as well as __ALIGNMENT_COUNT(). Currently we have 4 ports that
>>> enable it: mips, arm, powerpc and tile. Mips and arm pass 5 arguments,
>>> so __ASSUME_ALIGNED_REGISTER_PAIRS is what they need. Powerpc uses
>>> syscalls.list (thanks for pointing it), and so is not affected by this
>>> change. But tile is still in case. Is my understanding correct that it
>>> uses linux/readahead.c as implementation? If so, this patch will break
>>> tile ABI. That's why I introduced new option.
>>>
>>> So, as for me we cannot use __ASSUME_ALIGNED_REGISTER_PAIRS because of
>>> tile. Is it correct?
>>
>> Does readahead even work on tile today?  Maybe it's already broken and
>> this change fixes it. :)
>>
>> Chris?
> 
> In kernel 32-bit tile expects 4 arguments:
> 
> arch/tile/kernel/sys.c:
> 61 #if !defined(__tilegx__) || defined(CONFIG_COMPAT)
> 62 
> 63 #ifdef __BIG_ENDIAN
> 64 #define SYSCALL_PAIR(name) u32 name ## _hi, u32 name ## _lo
> 65 #else
> 66 #define SYSCALL_PAIR(name) u32 name ## _lo, u32 name ## _hi
> 67 #endif
> 68 
> 69 ssize_t sys32_readahead(int fd, SYSCALL_PAIR(offset), u32 count)
> 70 {
> 71         return sys_readahead(fd, ((loff_t)offset_hi << 32) | offset_lo, count);
> 72 }
> 73 
> 74 int sys32_fadvise64_64(int fd, SYSCALL_PAIR(offset),
> 75                        SYSCALL_PAIR(len), int advice)
> 76 {
> 77         return sys_fadvise64_64(fd, ((loff_t)offset_hi << 32) | offset_lo,
> 78                                 ((loff_t)len_hi << 32) | len_lo, advice);
> 79 }
> 80 
> 81 #endif /* 32-bit syscall wrappers */
>  
> So it seems we have no chance to consolidate getdents() completely w/o
> new option. If no objections, I'll send v2 with removing getdents from
> powerpc syscalls.list, and rework new option in more suitable way.
> Objections?
> 
> Yury.
> 

Indeed, unfortunately tile seems to get its own readahead definition.
However I think it should not prevent us to use my previous strategy,
we can follow the SH example for pread (where it adds a dummy argument
before offset), and do something as:

sysdeps/unix/sysv/linux/tile/readahead.c

#include <sysdep.h>

#ifndef _LP64
/* Although tile 32-bit ABI passed 64-bit arguments in even registers,
   readahead interface does not follow this convention.  */
# undef __ALIGNMENT_ARG
#endif

#include <sysdeps/unix/sysv/linux/readhead.c>