From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Yury Norov <ynorov@caviumnetworks.com>, libc-alpha@sourceware.org
Subject: Re: [PATCH] Consolidate Linux readahead() implementations
Date: Thu, 22 Sep 2016 21:36:00 -0000 [thread overview]
Message-ID: <827f758c-b744-22f2-5dbb-4471208cd6b2@linaro.org> (raw)
In-Reply-To: <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
next prev parent reply other threads:[~2016-09-22 21:36 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-22 20:44 Yury Norov
2016-09-22 20:59 ` Dmitry V. Levin
2016-09-22 21:26 ` Adhemerval Zanella
2016-09-22 21:36 ` Adhemerval Zanella [this message]
2016-09-22 23:21 ` Yury Norov
2016-09-23 6:08 ` Florian Weimer
2016-09-23 12:45 ` Yury Norov
2016-09-23 13:32 ` Adhemerval Zanella
2016-09-23 14:12 ` Yury Norov
2016-09-23 14:24 ` Adhemerval Zanella
2016-09-23 15:45 ` Yury Norov
2016-09-23 19:50 ` Adhemerval Zanella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=827f758c-b744-22f2-5dbb-4471208cd6b2@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
--cc=ynorov@caviumnetworks.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).