This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Consolidate Linux readahead() implementations
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Yury Norov <ynorov at caviumnetworks dot com>, libc-alpha at sourceware dot org
- Date: Thu, 22 Sep 2016 18:36:22 -0300
- Subject: Re: [PATCH] Consolidate Linux readahead() implementations
- Authentication-results: sourceware.org; auth=none
- References: <1474577068-1781-1-git-send-email-ynorov@caviumnetworks.com>
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