Re: [PATCH] Consolidate Linux readahead() implementations

[Yury Norov <ynorov at caviumnetworks dot com>]

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.