[PATCH] Consolidate Linux readahead() implementations

[PATCH] Consolidate Linux readahead() implementations

[Yury Norov]

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
+
 #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
+
 /* 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
 # 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);
-- 
2.7.4

Re: [PATCH] Consolidate Linux readahead() implementations

[Dmitry V. Levin]

On Thu, Sep 22, 2016 at 11:44:28PM +0300, Yury Norov wrote:
> All Linux users pass 4 arguments to readahead() except arm and mips.

That's odd because 64-bit kernels take only 3 arguments, and powerpc takes 5.


-- 
ldv

Re: [PATCH] Consolidate Linux readahead() implementations

[Adhemerval Zanella]

On 22/09/2016 17:59, Dmitry V. Levin wrote:
> On Thu, Sep 22, 2016 at 11:44:28PM +0300, Yury Norov wrote:
>> All Linux users pass 4 arguments to readahead() except arm and mips.
> 
> That's odd because 64-bit kernels take only 3 arguments, and powerpc takes 5.
> 
> 

Yes, it is because powerpc and 64-bits kernels syscalls wrappers are
generated by syscalls.list instead of C implementations.  On powerpc
it is generated by:

(echo '#define SYSCALL_NAME readahead'; \
 echo '#define SYSCALL_NARGS 5'; \
 echo '#define SYSCALL_SYMBOL __readahead'; \
 echo '#define SYSCALL_CANCELLABLE 0'; \
 echo '#define SYSCALL_NOERRNO 0'; \
 echo '#define SYSCALL_ERRVAL 0'; \
 echo '#include <syscall-template.S>'; \
 echo 'weak_alias (__readahead, readahead)'; \
 echo 'hidden_weak (readahead)'; \
...)

While on x86_64 it is:

(echo '#define SYSCALL_NAME readahead'; \
 echo '#define SYSCALL_NARGS 3'; \
 echo '#define SYSCALL_SYMBOL __readahead'; \
 echo '#define SYSCALL_CANCELLABLE 0'; \
 echo '#define SYSCALL_NOERRNO 0'; \
 echo '#define SYSCALL_ERRVAL 0'; \
 echo '#include <syscall-template.S>'; \
 echo 'weak_alias (__readahead, readahead)'; \
 echo 'hidden_weak (readahead)'; \

The 4 argument is only for 32-bit ABI the passes 64-bit arguments
in 2 registers.

Re: [PATCH] Consolidate Linux readahead() implementations

[Adhemerval Zanella]

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

Re: [PATCH] Consolidate Linux readahead() implementations

[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?

Is my understanding correct that powerpc layout is identical to arm
and mips? If so we can remove readahead from powerpc syscalls.abilst
and switch to linux/readahead.c with this patch (or with one proposed
by you).

Yury.

Re: [PATCH] Consolidate Linux readahead() implementations

[Florian Weimer]

* 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?

Re: [PATCH] Consolidate Linux readahead() implementations

[Yury Norov]

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.

Re: [PATCH] Consolidate Linux readahead() implementations

[Adhemerval Zanella]

On 23/09/2016 09:44, Yury Norov wrote:
> 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.
> 

Indeed, unfortunately tile seems to get its own readahead definition.
However I think it should not prevent us to use my previous strategy,
we can follow the SH example for pread (where it adds a dummy argument
before offset), and do something as:

sysdeps/unix/sysv/linux/tile/readahead.c

#include <sysdep.h>

#ifndef _LP64
/* Although tile 32-bit ABI passed 64-bit arguments in even registers,
   readahead interface does not follow this convention.  */
# undef __ALIGNMENT_ARG
#endif

#include <sysdeps/unix/sysv/linux/readhead.c>

Re: [PATCH] Consolidate Linux readahead() implementations

[Yury Norov]

On Fri, Sep 23, 2016 at 10:32:35AM -0300, Adhemerval Zanella wrote:
> 
> 
> On 23/09/2016 09:44, Yury Norov wrote:
> > 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.
> > 
> 
> Indeed, unfortunately tile seems to get its own readahead definition.
> However I think it should not prevent us to use my previous strategy,
> we can follow the SH example for pread (where it adds a dummy argument
> before offset), and do something as:
> 
> sysdeps/unix/sysv/linux/tile/readahead.c
> 
> #include <sysdep.h>
> 
> #ifndef _LP64
> /* Although tile 32-bit ABI passed 64-bit arguments in even registers,
>    readahead interface does not follow this convention.  */
> # undef __ALIGNMENT_ARG
> #endif
> 
> #include <sysdeps/unix/sysv/linux/readhead.c>

Currently it looks like this to me (see below). If you think that separated file
is better than new option - I'm OK with it, but I think it's strange because in
other patches of series you introduce options (if I'm not mistake).

We also have 2 another implementations - in linux/wordsize-64/syscalls.list
and linux/mips/mips64/n32/syscalls.list.

I think wordsize-64 is safe to generalize, but I'm worry about mips64. If we'll
choose adding new options and so having a single file, it seems, we'll have to
add another option for mips64/n32, like this:

#if __ASSUME_READAHEAD_NO_PAIRS
# define SYSCALL_LL64(val) (val)
#endif

If we choose 3 implementations, we can introduce no option, but have
generic, tile and mips separated versions.

Yury.

--
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 71ce57a..ba7d745 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	1
+#endif
+
 /* The statfs64 syscalls are available in 2.5.74 (but not for alpha).  */
 #define __ASSUME_STATFS64	1
 
diff --git a/sysdeps/unix/sysv/linux/readahead.c b/sysdeps/unix/sysv/linux/readahead.c
index 92e5428..eea3142 100644
--- a/sysdeps/unix/sysv/linux/readahead.c
+++ b/sysdeps/unix/sysv/linux/readahead.c
@@ -23,16 +23,20 @@
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#include <kernel-features.h>
 
 #ifdef __NR_readahead
 
+#if !__ASSUME_READAHEAD_ALIGN
+# undef __ALIGNMENT_ARG
+# define __ALIGNMENT_ARG
+#endif
+
 ssize_t
 __readahead (int fd, off64_t offset, size_t count)
 {
-  return INLINE_SYSCALL (readahead, 4, fd,
-			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
-					   (off_t) (offset & 0xffffffff)),
-			 count);
+  return INLINE_SYSCALL_CALL (readahead, fd, __ALIGNMENT_ARG
+			      SYSCALL_LL64 (offset));
 }
 #else
 ssize_t
diff --git a/sysdeps/unix/sysv/linux/tile/kernel-features.h b/sysdeps/unix/sysv/linux/tile/kernel-features.h
index ded0e43..15ad2d3 100644
--- a/sysdeps/unix/sysv/linux/tile/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/tile/kernel-features.h
@@ -24,4 +24,5 @@
 #ifndef _LP64
 # define __ASSUME_ALIGNED_REGISTER_PAIRS	1
 # define __ASSUME_FADVISE64_64_NO_ALIGN		1
+# define __ASSUME_READAHEAD_ALIGN		0
 #endif
-- 
2.7.4

Re: [PATCH] Consolidate Linux readahead() implementations

[Adhemerval Zanella]

On 23/09/2016 11:11, Yury Norov wrote:
> On Fri, Sep 23, 2016 at 10:32:35AM -0300, Adhemerval Zanella wrote:
>>
>> Indeed, unfortunately tile seems to get its own readahead definition.
>> However I think it should not prevent us to use my previous strategy,
>> we can follow the SH example for pread (where it adds a dummy argument
>> before offset), and do something as:
>>
>> sysdeps/unix/sysv/linux/tile/readahead.c
>>
>> #include <sysdep.h>
>>
>> #ifndef _LP64
>> /* Although tile 32-bit ABI passed 64-bit arguments in even registers,
>>    readahead interface does not follow this convention.  */
>> # undef __ALIGNMENT_ARG
>> #endif
>>
>> #include <sysdeps/unix/sysv/linux/readhead.c>
> 
> Currently it looks like this to me (see below). If you think that separated file
> is better than new option - I'm OK with it, but I think it's strange because in
> other patches of series you introduce options (if I'm not mistake).

I prefer to not add any more __ASSUME macro until it is more general
and for this specific case (tile seems the only supported ABI that
implements readhead different than usual ABI).

> 
> We also have 2 another implementations - in linux/wordsize-64/syscalls.list
> and linux/mips/mips64/n32/syscalls.list.
> 
> I think wordsize-64 is safe to generalize, but I'm worry about mips64. If we'll
> choose adding new options and so having a single file, it seems, we'll have to
> add another option for mips64/n32, like this:

My understanding is mipc64n32 adds it on syscall list to avoid current
default Linux implementation where it splits the off64_t (since mips64n32
passes off64_t in only one register afaik).  I think it is safe here,
since SYSCALL_LL64 for mips64n32 will correctly pass only one argument
instead of splitting it.

> 
> #if __ASSUME_READAHEAD_NO_PAIRS
> # define SYSCALL_LL64(val) (val)
> #endif
> 
> If we choose 3 implementations, we can introduce no option, but have
> generic, tile and mips separated versions.
> 
> Yury.
> 
> --
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index 71ce57a..ba7d745 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	1
> +#endif
> +
>  /* The statfs64 syscalls are available in 2.5.74 (but not for alpha).  */
>  #define __ASSUME_STATFS64	1
>  
> diff --git a/sysdeps/unix/sysv/linux/readahead.c b/sysdeps/unix/sysv/linux/readahead.c
> index 92e5428..eea3142 100644
> --- a/sysdeps/unix/sysv/linux/readahead.c
> +++ b/sysdeps/unix/sysv/linux/readahead.c
> @@ -23,16 +23,20 @@
>  #include <sysdep.h>
>  #include <sys/syscall.h>
>  
> +#include <kernel-features.h>
>  
>  #ifdef __NR_readahead
>  
> +#if !__ASSUME_READAHEAD_ALIGN
> +# undef __ALIGNMENT_ARG
> +# define __ALIGNMENT_ARG
> +#endif
> +
>  ssize_t
>  __readahead (int fd, off64_t offset, size_t count)
>  {
> -  return INLINE_SYSCALL (readahead, 4, fd,
> -			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
> -					   (off_t) (offset & 0xffffffff)),
> -			 count);
> +  return INLINE_SYSCALL_CALL (readahead, fd, __ALIGNMENT_ARG
> +			      SYSCALL_LL64 (offset));
>  }
>  #else
>  ssize_t
> diff --git a/sysdeps/unix/sysv/linux/tile/kernel-features.h b/sysdeps/unix/sysv/linux/tile/kernel-features.h
> index ded0e43..15ad2d3 100644
> --- a/sysdeps/unix/sysv/linux/tile/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/tile/kernel-features.h
> @@ -24,4 +24,5 @@
>  #ifndef _LP64
>  # define __ASSUME_ALIGNED_REGISTER_PAIRS	1
>  # define __ASSUME_FADVISE64_64_NO_ALIGN		1
> +# define __ASSUME_READAHEAD_ALIGN		0
>  #endif
> 

Re: [PATCH] Consolidate Linux readahead() implementations

[Yury Norov]

On Fri, Sep 23, 2016 at 11:24:32AM -0300, Adhemerval Zanella wrote:
> 
> 
> On 23/09/2016 11:11, Yury Norov wrote:
> > On Fri, Sep 23, 2016 at 10:32:35AM -0300, Adhemerval Zanella wrote:
> >>
> >> Indeed, unfortunately tile seems to get its own readahead definition.
> >> However I think it should not prevent us to use my previous strategy,
> >> we can follow the SH example for pread (where it adds a dummy argument
> >> before offset), and do something as:
> >>
> >> sysdeps/unix/sysv/linux/tile/readahead.c
> >>
> >> #include <sysdep.h>
> >>
> >> #ifndef _LP64
> >> /* Although tile 32-bit ABI passed 64-bit arguments in even registers,
> >>    readahead interface does not follow this convention.  */
> >> # undef __ALIGNMENT_ARG
> >> #endif
> >>
> >> #include <sysdeps/unix/sysv/linux/readhead.c>
> > 
> > Currently it looks like this to me (see below). If you think that separated file
> > is better than new option - I'm OK with it, but I think it's strange because in
> > other patches of series you introduce options (if I'm not mistake).
> 
> I prefer to not add any more __ASSUME macro until it is more general
> and for this specific case (tile seems the only supported ABI that
> implements readhead different than usual ABI).
> 
> > 
> > We also have 2 another implementations - in linux/wordsize-64/syscalls.list
> > and linux/mips/mips64/n32/syscalls.list.
> > 
> > I think wordsize-64 is safe to generalize, but I'm worry about mips64. If we'll
> > choose adding new options and so having a single file, it seems, we'll have to
> > add another option for mips64/n32, like this:
> 
> My understanding is mipc64n32 adds it on syscall list to avoid current
> default Linux implementation where it splits the off64_t (since mips64n32
> passes off64_t in only one register afaik).  I think it is safe here,
> since SYSCALL_LL64 for mips64n32 will correctly pass only one argument
> instead of splitting it.

OK. So for v2:
 - remove arm, mips, wordsize-64, powerpc implementations
 - generalize linux/readahead.c like below
 - add tile exception
 - remove all new __ASSUME 

If it's correct, I'll send new patch this evening or tomorrow.

Yury.

> >  ssize_t
> >  __readahead (int fd, off64_t offset, size_t count)
> >  {
> > -  return INLINE_SYSCALL (readahead, 4, fd,
> > -			 __LONG_LONG_PAIR ((off_t) (offset >> 32),
> > -					   (off_t) (offset & 0xffffffff)),
> > -			 count);
> > +  return INLINE_SYSCALL_CALL (readahead, fd, __ALIGNMENT_ARG
> > +			      SYSCALL_LL64 (offset));

Re: [PATCH] Consolidate Linux readahead() implementations

[Adhemerval Zanella]

On 23/09/2016 12:44, Yury Norov wrote:
> On Fri, Sep 23, 2016 at 11:24:32AM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 23/09/2016 11:11, Yury Norov wrote:
>>> On Fri, Sep 23, 2016 at 10:32:35AM -0300, Adhemerval Zanella wrote:
>>>>
>>>> Indeed, unfortunately tile seems to get its own readahead definition.
>>>> However I think it should not prevent us to use my previous strategy,
>>>> we can follow the SH example for pread (where it adds a dummy argument
>>>> before offset), and do something as:
>>>>
>>>> sysdeps/unix/sysv/linux/tile/readahead.c
>>>>
>>>> #include <sysdep.h>
>>>>
>>>> #ifndef _LP64
>>>> /* Although tile 32-bit ABI passed 64-bit arguments in even registers,
>>>>    readahead interface does not follow this convention.  */
>>>> # undef __ALIGNMENT_ARG
>>>> #endif
>>>>
>>>> #include <sysdeps/unix/sysv/linux/readhead.c>
>>>
>>> Currently it looks like this to me (see below). If you think that separated file
>>> is better than new option - I'm OK with it, but I think it's strange because in
>>> other patches of series you introduce options (if I'm not mistake).
>>
>> I prefer to not add any more __ASSUME macro until it is more general
>> and for this specific case (tile seems the only supported ABI that
>> implements readhead different than usual ABI).
>>
>>>
>>> We also have 2 another implementations - in linux/wordsize-64/syscalls.list
>>> and linux/mips/mips64/n32/syscalls.list.
>>>
>>> I think wordsize-64 is safe to generalize, but I'm worry about mips64. If we'll
>>> choose adding new options and so having a single file, it seems, we'll have to
>>> add another option for mips64/n32, like this:
>>
>> My understanding is mipc64n32 adds it on syscall list to avoid current
>> default Linux implementation where it splits the off64_t (since mips64n32
>> passes off64_t in only one register afaik).  I think it is safe here,
>> since SYSCALL_LL64 for mips64n32 will correctly pass only one argument
>> instead of splitting it.
> 
> OK. So for v2:
>  - remove arm, mips, wordsize-64, powerpc implementations
>  - generalize linux/readahead.c like below
>  - add tile exception
>  - remove all new __ASSUME 
> 
> If it's correct, I'll send new patch this evening or tomorrow.

That would be my recommendation.