[v2,02/16] Consolidate Linux msgctl implementation
diff mbox

Message ID 20161104153321.GA1271@yury-N73SV
State New
Headers show

Commit Message

Yury Norov Nov. 4, 2016, 3:33 p.m. UTC
On Wed, Nov 02, 2016 at 05:26:39PM -0200, Adhemerval Zanella wrote:
> Changes from previous version:
> 
>   - Use __ASSUME_SYSVIPC_SYSCALL instead of __NR_syscall to issue the
>     wired syscall or the ipc one.
> 
> --
> 
> This patch consolidates the msgctl Linux implementation in only
> one default file, sysdeps/unix/sysv/linux/msgctl.c.  If tries to use
> the direct syscall if it is supported, otherwise will use the old ipc
> multiplex mechanism.
> 
> The patch also simplify header inclusion and reorganize internal
> compat symbol to be built only if old ipc is defined.
> 
> Checked on x86_64, i686, powerpc64le, aarch64, and armhf.
> 
> 	* sysdeps/unix/sysv/linux/alpha/msgctl.c: Remove file.
> 	* sysdeps/unix/sysv/linux/arm/msgctl.c: Likewise.
> 	* sysdeps/unix/sysv/linux/microblaze/msgctl.c: Likewise.
> 	* sysdeps/unix/sysv/linux/mips/mips64/msgctl.c: Use default
> 	implementation.
> 	* sysdeps/unix/sysv/linux/msgctl.c (__new_msgctl): Use msgctl syscall
> 	if defined.
> ---
>  ChangeLog                                    |  8 ++++++
>  sysdeps/unix/sysv/linux/alpha/msgctl.c       |  1 -
>  sysdeps/unix/sysv/linux/arm/msgctl.c         | 33 ----------------------
>  sysdeps/unix/sysv/linux/microblaze/msgctl.c  |  1 -
>  sysdeps/unix/sysv/linux/mips/mips64/msgctl.c | 17 ++---------
>  sysdeps/unix/sysv/linux/msgctl.c             | 42 +++++++++++++---------------
>  6 files changed, 30 insertions(+), 72 deletions(-)
>  delete mode 100644 sysdeps/unix/sysv/linux/alpha/msgctl.c
>  delete mode 100644 sysdeps/unix/sysv/linux/arm/msgctl.c
>  delete mode 100644 sysdeps/unix/sysv/linux/microblaze/msgctl.c

Hi Adhemerval, 

I'm testing your series with aarch64/ilp32 + LTP, and see regressions:
msgctl01                       FAIL       1
msgctl06                       FAIL       1
msgsnd01                       FAIL       1
semctl01                       FAIL       1
semctl07                       FAIL       1
semget01                       FAIL       1
shmat01                        FAIL       1
shmctl01                       FAIL       1
shmget01                       FAIL       1

Regarding msgctl. objdump for libc.so shows that syscall is generated
from stub in sysdeps/unix/sysv/linux/generic/syscalls.list.
000d0e80 <msgctl>:
#else

/* This is a "normal" system call stub: if there is an error,
   it returns -1 and sets errno.  */

T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS)
  d0e80:       d2801768        mov     x8, #0xbb
  // #187
  d0e84:       d4000001        svc     #0x0
  d0e88:       b13ffc1f        cmn     x0, #0xfff
  d0e8c:       54000042        b.cs    d0e94
<msgctl+0x14>
        ret
  d0e90:       d65f03c0        ret
T_PSEUDO_END (SYSCALL_SYMBOL)
  d0e94:       17fd194b        b
  173c0 <__GI___syscall_error>
  d0e98:       d503201f        nop
  d0e9c:       d503201f        nop

For aarch64/ilp32 to drop custom implementation, we need 
to force __IPC_64, and so remove stubs for msgctl, semctl and shmctl
to let glibc find proper implementation under sysv/linux.

The minimal fix for my regression is below. Though, I'm not sure that it's
complete, and that it will not affect other ports. BTW, grepping for
msgctl in syscalls.list files shows that they has outdated comment
which should be fixed in this series too - in arm, microblaze and mips64 ports.

# Semaphore and shm system calls.  msgctl, shmctl, and semctl have C
# wrappers (to set __IPC_64).

Yury.

Comments

Adhemerval Zanella Nov. 4, 2016, 5:03 p.m. UTC | #1
On 04/11/2016 13:33, Yury Norov wrote:
> On Wed, Nov 02, 2016 at 05:26:39PM -0200, Adhemerval Zanella wrote:
>> Changes from previous version:
>>
>>   - Use __ASSUME_SYSVIPC_SYSCALL instead of __NR_syscall to issue the
>>     wired syscall or the ipc one.
>>
>> --
>>
>> This patch consolidates the msgctl Linux implementation in only
>> one default file, sysdeps/unix/sysv/linux/msgctl.c.  If tries to use
>> the direct syscall if it is supported, otherwise will use the old ipc
>> multiplex mechanism.
>>
>> The patch also simplify header inclusion and reorganize internal
>> compat symbol to be built only if old ipc is defined.
>>
>> Checked on x86_64, i686, powerpc64le, aarch64, and armhf.
>>
>> 	* sysdeps/unix/sysv/linux/alpha/msgctl.c: Remove file.
>> 	* sysdeps/unix/sysv/linux/arm/msgctl.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/microblaze/msgctl.c: Likewise.
>> 	* sysdeps/unix/sysv/linux/mips/mips64/msgctl.c: Use default
>> 	implementation.
>> 	* sysdeps/unix/sysv/linux/msgctl.c (__new_msgctl): Use msgctl syscall
>> 	if defined.
>> ---
>>  ChangeLog                                    |  8 ++++++
>>  sysdeps/unix/sysv/linux/alpha/msgctl.c       |  1 -
>>  sysdeps/unix/sysv/linux/arm/msgctl.c         | 33 ----------------------
>>  sysdeps/unix/sysv/linux/microblaze/msgctl.c  |  1 -
>>  sysdeps/unix/sysv/linux/mips/mips64/msgctl.c | 17 ++---------
>>  sysdeps/unix/sysv/linux/msgctl.c             | 42 +++++++++++++---------------
>>  6 files changed, 30 insertions(+), 72 deletions(-)
>>  delete mode 100644 sysdeps/unix/sysv/linux/alpha/msgctl.c
>>  delete mode 100644 sysdeps/unix/sysv/linux/arm/msgctl.c
>>  delete mode 100644 sysdeps/unix/sysv/linux/microblaze/msgctl.c
> 
> Hi Adhemerval, 
> 
> I'm testing your series with aarch64/ilp32 + LTP, and see regressions:
> msgctl01                       FAIL       1
> msgctl06                       FAIL       1
> msgsnd01                       FAIL       1
> semctl01                       FAIL       1
> semctl07                       FAIL       1
> semget01                       FAIL       1
> shmat01                        FAIL       1
> shmctl01                       FAIL       1
> shmget01                       FAIL       1
> 
> Regarding msgctl. objdump for libc.so shows that syscall is generated
> from stub in sysdeps/unix/sysv/linux/generic/syscalls.list.
> 000d0e80 <msgctl>:
> #else
> 
> /* This is a "normal" system call stub: if there is an error,
>    it returns -1 and sets errno.  */
> 
> T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS)
>   d0e80:       d2801768        mov     x8, #0xbb
>   // #187
>   d0e84:       d4000001        svc     #0x0
>   d0e88:       b13ffc1f        cmn     x0, #0xfff
>   d0e8c:       54000042        b.cs    d0e94
> <msgctl+0x14>
>         ret
>   d0e90:       d65f03c0        ret
> T_PSEUDO_END (SYSCALL_SYMBOL)
>   d0e94:       17fd194b        b
>   173c0 <__GI___syscall_error>
>   d0e98:       d503201f        nop
>   d0e9c:       d503201f        nop
> 
> For aarch64/ilp32 to drop custom implementation, we need 
> to force __IPC_64, and so remove stubs for msgctl, semctl and shmctl
> to let glibc find proper implementation under sysv/linux.
> 
> The minimal fix for my regression is below. Though, I'm not sure that it's
> complete, and that it will not affect other ports. BTW, grepping for
> msgctl in syscalls.list files shows that they has outdated comment
> which should be fixed in this series too - in arm, microblaze and mips64 ports.
> 
> # Semaphore and shm system calls.  msgctl, shmctl, and semctl have C
> # wrappers (to set __IPC_64).
> 
> Yury.
> 
> diff --git a/sysdeps/unix/sysv/linux/generic/syscalls.list b/sysdeps/unix/sysv/linux/generic/syscalls.list
> index 5993ab4..459892d 100644
> --- a/sysdeps/unix/sysv/linux/generic/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/generic/syscalls.list
> @@ -2,15 +2,12 @@
>  
>  # SysV APIs
>  msgget		-	msgget		i:ii	__msgget	msgget
> -msgctl		-	msgctl		i:iip	__msgctl	msgctl
>  msgrcv		-	msgrcv		Ci:ibnii __msgrcv	msgrcv
>  msgsnd		-	msgsnd		Ci:ibni	__msgsnd	msgsnd
>  semget		-	semget		i:iii	__semget	semget
> -semctl		-	semctl		i:iiii	__semctl	semctl
>  semtimedop	-	semtimedop	i:ipip	semtimedop
>  semop		-	semop		i:ipi	__semop		semop
>  shmget		-	shmget		i:iii	__shmget	shmget
> -shmctl		-	shmctl		i:iip	__shmctl	shmctl
>  shmat		-	shmat		i:ipi	__shmat		shmat
>  shmdt		-	shmdt		i:s	__shmdt		shmdt

Yes, auto-generation syscall won't add the __IPC_64 required for
some architectures.  And that's why I have added in default 
implementation, since kernel will just ignore for architecture
that do not require it.

And I think by removing them from generic syscalls.list seems
reasonable, since for some architecture. I will change my
local branch accordingly.

Patch
diff mbox

diff --git a/sysdeps/unix/sysv/linux/generic/syscalls.list b/sysdeps/unix/sysv/linux/generic/syscalls.list
index 5993ab4..459892d 100644
--- a/sysdeps/unix/sysv/linux/generic/syscalls.list
+++ b/sysdeps/unix/sysv/linux/generic/syscalls.list
@@ -2,15 +2,12 @@ 
 
 # SysV APIs
 msgget		-	msgget		i:ii	__msgget	msgget
-msgctl		-	msgctl		i:iip	__msgctl	msgctl
 msgrcv		-	msgrcv		Ci:ibnii __msgrcv	msgrcv
 msgsnd		-	msgsnd		Ci:ibni	__msgsnd	msgsnd
 semget		-	semget		i:iii	__semget	semget
-semctl		-	semctl		i:iiii	__semctl	semctl
 semtimedop	-	semtimedop	i:ipip	semtimedop
 semop		-	semop		i:ipi	__semop		semop
 shmget		-	shmget		i:iii	__shmget	shmget
-shmctl		-	shmctl		i:iip	__shmctl	shmctl
 shmat		-	shmat		i:ipi	__shmat		shmat
 shmdt		-	shmdt		i:s	__shmdt		shmdt