Re: [PATCH 4/4] Consolidate posix_fadvise implementations

[Yury Norov <ynorov at caviumnetworks dot com>]

On Fri, Aug 19, 2016 at 11:41:49AM -0300, Adhemerval Zanella wrote:
> diff --git a/sysdeps/unix/sysv/linux/posix_fadvise.c b/sysdeps/unix/sysv/linux/posix_fadvise.c
> index 093d707..869a642 100644
> --- a/sysdeps/unix/sysv/linux/posix_fadvise.c
> +++ b/sysdeps/unix/sysv/linux/posix_fadvise.c
> @@ -22,27 +22,46 @@
>  /* Advice the system about the expected behaviour of the application with
>     respect to the file associated with FD.  */
>  
> +#ifndef __OFF_T_MATCHES_OFF64_T
> +
> +/* Both arm and powerpc implements fadvise64_64 with last 'advise' argument
> +   just after 'fd' to avoid the requirement of implementing 7-arg syscalls.
> +   ARM also defines __NR_fadvise64_64 as __NR_arm_fadvise64_64.
> +
> +   tile requires __ASSUME_ALIGNED_REGISTER_PAIRS but implements the 32-bit
> +   fadvise64_64 without the padding 0 after fd.
> +
> +   s390 implements fadvice64_64 using a specific struct with arguments
> +   packed inside.  This is the only implementation handled in arch-specific
> +   code.  */
> +
>  int
>  posix_fadvise (int fd, off_t offset, off_t len, int advise)
>  {
> -#if defined(__NR_fadvise64) || defined(__NR_fadvise64_64)
>    INTERNAL_SYSCALL_DECL (err);
>  # ifdef __NR_fadvise64
> -  int ret = INTERNAL_SYSCALL (fadvise64, err, 5, fd,
> -			      __LONG_LONG_PAIR (offset >> 31, offset), len,
> -			      advise);
> +  int ret = INTERNAL_SYSCALL_CALL (fadvise64, err, fd,
> +				   __ALIGNMENT_ARG SYSCALL_LL (offset),
> +				   len, advise);
>  # else
> -  int ret = INTERNAL_SYSCALL (fadvise64_64, err, 6, fd,
> -			      __LONG_LONG_PAIR ((long) (offset >> 31),
> -						(long) offset),
> -			      __LONG_LONG_PAIR ((long) (len >> 31),
> -						(long) len),
> -			      advise);
> +#  ifdef __ASSUME_FADVISE64_64_6ARG
> +  int ret = INTERNAL_SYSCALL_CALL (fadvise64_64, err, fd, advise,
> +				   __ALIGNMENT_ARG SYSCALL_LL (offset),
> +				   SYSCALL_LL (len));
> +#  else
> +
> +#   ifdef __ASSUME_FADVISE64_64_NO_ALIGN
> +#    undef __ALIGNMENT_ARG
> +#    define __ALIGNMENT_ARG
> +#   endif
> +
> +  int ret = INTERNAL_SYSCALL_CALL (fadvise64_64, err, fd,
> +				   __ALIGNMENT_ARG SYSCALL_LL (offset),
> +				   SYSCALL_LL (len), advise);
> +#  endif
>  # endif
>    if (INTERNAL_SYSCALL_ERROR_P (ret, err))
>      return INTERNAL_SYSCALL_ERRNO (ret, err);
>    return 0;
> -#else
> -  return ENOSYS;
> -#endif
>  }
> +#endif /* __OFF_T_MATCHES_OFF64_T  */

[...]

> diff --git a/sysdeps/unix/sysv/linux/tile/kernel-features.h b/sysdeps/unix/sysv/linux/tile/kernel-features.h
> index 9425e1c..ded0e43 100644
> --- a/sysdeps/unix/sysv/linux/tile/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/tile/kernel-features.h
> @@ -23,4 +23,5 @@
>     pairs to start with an even-number register.  */
>  #ifndef _LP64
>  # define __ASSUME_ALIGNED_REGISTER_PAIRS	1
> +# define __ASSUME_FADVISE64_64_NO_ALIGN		1
>  #endif

Hi Adhemerval,

In discussion to the readahead() consolidation patch [1] you recommend
not to add new __ASSUME_ options, and have exceptional implementation
for tile. But in this patch for posix_fadvise() consolidation you add
new __ASSUME_FADVISE64_64_NO_ALIGN. This cases are very similar, so I
think, to be consistent, you'd also rework posix_fadvise() too. 

[1] https://sourceware.org/ml/libc-alpha/2016-09/msg00494.html

Yury.