Re: [PATCH] Consolidate Linux readahead() implementations

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

Hi Yury, some comments below.

On 22/09/2016 17:44, Yury Norov wrote:
> All Linux users pass 4 arguments to readahead() except arm and mips.
> That two also pass an alignement. Usually to have single implementation,
> __ASSUME_ALIGNED_REGISTER_PAIRS option is used. But readahead() cannot
> do this because __ASSUME_ALIGNED_REGISTER_PAIRS is also enabled for tile
> and powerpc.
> 
> To consolidate all implementations, new option  __ASSUME_READAHEAD_ALIGN
> is introduced here, and enabled only for arm and mips. This is also the
> case for new arm64/ilp32 as it's the arm32 successor.
> 
> 2016-09-22: Yury Norov  <ynorov@caviumnetworks.com>
> 	* sysdeps/unix/sysv/linux/arm/kernel-features.h: new
> 	  __ASSUME_READAHEAD_ALIGN option.
> 	* sysdeps/unix/sysv/linux/mips/kernel-features.h: Likewise.
> 	* sysdeps/unix/sysv/linux/kernel-features.h: Likewise.
> 	* sysdeps/unix/sysv/linux/arm/readahead.c: Remove.
> 	* sysdeps/unix/sysv/linux/mips/mips32/readahead.c: Remove.
> 	* sysdeps/unix/sysv/linux/readahead.c: insert padding
> 	  register if __ASSUME_READAHEAD_ALIGN is enabled.
> 
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> CC: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
>  sysdeps/unix/sysv/linux/arm/kernel-features.h   |  2 ++
>  sysdeps/unix/sysv/linux/arm/readahead.c         | 37 -------------------------
>  sysdeps/unix/sysv/linux/kernel-features.h       |  5 ++++
>  sysdeps/unix/sysv/linux/mips/kernel-features.h  |  1 +
>  sysdeps/unix/sysv/linux/mips/mips32/readahead.c |  1 -
>  sysdeps/unix/sysv/linux/readahead.c             | 11 +++++++-
>  6 files changed, 18 insertions(+), 39 deletions(-)
>  delete mode 100644 sysdeps/unix/sysv/linux/arm/readahead.c
>  delete mode 100644 sysdeps/unix/sysv/linux/mips/mips32/readahead.c
> 
> diff --git a/sysdeps/unix/sysv/linux/arm/kernel-features.h b/sysdeps/unix/sysv/linux/arm/kernel-features.h
> index 628d27f..cb3b6aa 100644
> --- a/sysdeps/unix/sysv/linux/arm/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/arm/kernel-features.h
> @@ -17,6 +17,8 @@
>     License along with the GNU C Library.  If not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#define __ASSUME_READAHEAD_ALIGN	1
> +

I do not think there is need to add this define. AFAIK, readahead interface
for 32-bit ports follow other interfaces that pass loff_t in odd argument
positions and we already have __ALIGNMENT_ARG for such cases.

>  #include_next <kernel-features.h>
>  
>  /* The ARM kernel before 3.14.3 may or may not support
> diff --git a/sysdeps/unix/sysv/linux/arm/readahead.c b/sysdeps/unix/sysv/linux/arm/readahead.c
> deleted file mode 100644
> index 9824e6f..0000000
> --- a/sysdeps/unix/sysv/linux/arm/readahead.c
> +++ /dev/null
> @@ -1,37 +0,0 @@
> -/* Provide kernel hint to read ahead.
> -   Copyright (C) 2002-2016 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library.  If not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <sys/types.h>
> -#include <endian.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -
> -ssize_t
> -__readahead (int fd, off64_t offset, size_t count)
> -{
> -  return INLINE_SYSCALL (readahead, 5, fd, 0,
> -			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
> -					   (off_t) (offset & 0xffffffff)),
> -			 count);
> -}
> -
> -weak_alias (__readahead, readahead)
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index 71ce57a..24216e5 100644
> --- a/sysdeps/unix/sysv/linux/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/kernel-features.h
> @@ -50,6 +50,11 @@
>  #define __ASSUME_ST_INO_64_BIT		1
>  #endif
>  
> +#ifndef __ASSUME_READAHEAD_ALIGN
> +/* readahead() adds padding to registers if this control is enabled.  */
> +# define __ASSUME_READAHEAD_ALIGN	0
> +#endif

Same as before.

> +
>  /* The statfs64 syscalls are available in 2.5.74 (but not for alpha).  */
>  #define __ASSUME_STATFS64	1
>  
> diff --git a/sysdeps/unix/sysv/linux/mips/kernel-features.h b/sysdeps/unix/sysv/linux/mips/kernel-features.h
> index b486d90..88f4f5e 100644
> --- a/sysdeps/unix/sysv/linux/mips/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/mips/kernel-features.h
> @@ -31,6 +31,7 @@
>  /* Define this if your 32-bit syscall API requires 64-bit register
>     pairs to start with an even-number register.  */
>  #if _MIPS_SIM == _ABIO32
> +# define __ASSUME_READAHEAD_ALIGN		1

Ditto.

>  # define __ASSUME_ALIGNED_REGISTER_PAIRS	1
>  #endif
>  
> diff --git a/sysdeps/unix/sysv/linux/mips/mips32/readahead.c b/sysdeps/unix/sysv/linux/mips/mips32/readahead.c
> deleted file mode 100644
> index 80170c3..0000000
> --- a/sysdeps/unix/sysv/linux/mips/mips32/readahead.c
> +++ /dev/null
> @@ -1 +0,0 @@
> -#include <sysdeps/unix/sysv/linux/arm/readahead.c>
> diff --git a/sysdeps/unix/sysv/linux/readahead.c b/sysdeps/unix/sysv/linux/readahead.c
> index 92e5428..d31bd6e 100644
> --- a/sysdeps/unix/sysv/linux/readahead.c
> +++ b/sysdeps/unix/sysv/linux/readahead.c
> @@ -23,13 +23,22 @@
>  #include <sysdep.h>
>  #include <sys/syscall.h>
>  
> +#include <kernel-features.h>
>  
>  #ifdef __NR_readahead
>  
> +#if __ASSUME_READAHEAD_ALIGN
> +# define __READAHEAD_ARGS		5
> +# define __READAHEAD_ALIGN		0,
> +#else
> +# define __READAHEAD_ARGS		4
> +# define __READAHEAD_ALIGN
> +#endif
> +
>  ssize_t
>  __readahead (int fd, off64_t offset, size_t count)
>  {
> -  return INLINE_SYSCALL (readahead, 4, fd,
> +  return INLINE_SYSCALL (readahead, __READAHEAD_ARGS, fd, __READAHEAD_ALIGN
>  			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
>  					   (off_t) (offset & 0xffffffff)),
>  			 count);
> 

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