[v3,18/21] nptl: s390: Fix Race conditions in pthread cancellation (BZ#12683)
diff mbox series

Message ID 20191014205656.29834-19-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • nptl: Fix Race conditions in pthread cancellation (BZ#12683)
Related show

Commit Message

Adhemerval Zanella Oct. 14, 2019, 8:56 p.m. UTC
This patch adds the s390 modifications required for the BZ#12683 fix
by adding the arch-specific cancellation syscall bridge.

Checked on s390-linux-gnu and s390x-linux-gnu.
---
 .../sysv/linux/s390/s390-32/syscall_cancel.S  | 83 +++++++++++++++++++
 .../sysv/linux/s390/s390-64/syscall_cancel.S  | 83 +++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
 create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S

Comments

Stefan Liebler Oct. 16, 2019, 3:46 p.m. UTC | #1
Hi Adhemerval,

I've added some notes below to the s390-64 file, but the same applies 
also for s390-32. I've also attached a diff.

I've also recognized that a call starting from e.g. write () involves 
various shuffling of the argument registers at each level:
write (ARGS in r2-r4)
-> __syscall_cancel (r2=nr, ARGS in r3-r6 and two stack-slots)
--> __syscall_cancel_arch (r2=*ch, r3=nr, ARGS in r4-r6 and three 
stack-slots)
---> "syscall-instruction" (ARGS in r2-r7)

Just as a quick idea (I don't know if there are other limitations), 
those shuffling instructions could perhaps be omitted if the nr / ch 
arguments of the __syscall_cancel / __syscall_cancel_arch functions 
would be the last arguments instead of the first ones.
I assume that also other archs could benefit from such an ordering.

Bye,
Stefan

On 10/14/19 10:56 PM, Adhemerval Zanella wrote:
> This patch adds the s390 modifications required for the BZ#12683 fix
> by adding the arch-specific cancellation syscall bridge.
> 
> Checked on s390-linux-gnu and s390x-linux-gnu.
> ---
>   .../sysv/linux/s390/s390-32/syscall_cancel.S  | 83 +++++++++++++++++++
>   .../sysv/linux/s390/s390-64/syscall_cancel.S  | 83 +++++++++++++++++++
>   2 files changed, 166 insertions(+)
>   create mode 100644 sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>   create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
> 
> diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
> new file mode 100644
> index 0000000000..3c934addbd
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
> @@ -0,0 +1,83 @@
> +/* Cancellable syscall wrapper.  Linux/s390 version.
> +   Copyright (C) 2019 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 <sysdep.h>
> +
> +/* long int __syscall_cancel_arch (int *cancelhandling,
> +				   __syscall_arg_t nr,
> +				   __syscall_arg_t arg1,
> +				   __syscall_arg_t arg2,
> +				   __syscall_arg_t arg3,
> +				   __syscall_arg_t arg4,
> +				   __syscall_arg_t arg5,
> +				   __syscall_arg_t arg6)  */
> +
> +ENTRY (__syscall_cancel_arch)
> +	stm	%r6,%r15,24(%r15)
> +	cfi_offset (6, -72)
> +	cfi_offset (7, -68)
> +	cfi_offset (8, -64)
> +	cfi_offset (9, -60)
> +	cfi_offset (10, -56)
> +	cfi_offset (11, -52)
> +	cfi_offset (12, -48)
> +	cfi_offset (13, -44)
> +	cfi_offset (14, -40)
> +	cfi_offset (15, -36)
> +	ahi	%r15, -96
> +	cfi_def_cfa_offset (192)
> +
> +	.globl __syscall_cancel_arch_start
> +	.type  __syscall_cancel_arch_start,@function
> +__syscall_cancel_arch_start:
> +	l	%r0, 0(%r2)
> +	tml	%r0, 4
> +	jne	1f
> +	lr	%r1, %r3
> +	lr	%r2, %r4
> +	lr	%r3, %r5
> +	lr	%r4, %r6
> +	l	%r5, 192(%r15)
> +	l	%r6, 196(%r15)
> +	l	%r7, 200(%r15)
> +	svc	0
> +
> +	.globl __syscall_cancel_arch_end
> +	.type  __syscall_cancel_arch_end,@function
> +__syscall_cancel_arch_end:
> +	l	%r4, 152(%r15)
> +	lm	%r6, %r15, 120(%r15)
> +	cfi_remember_state
> +	cfi_restore (15)
> +	cfi_restore (14)
> +	cfi_restore (13)
> +	cfi_restore (12)
> +	cfi_restore (11)
> +	cfi_restore (10)
> +	cfi_restore (9)
> +	cfi_restore (8)
> +	cfi_restore (7)
> +	cfi_restore (6)
> +	cfi_def_cfa_offset (96)
> +	br	%r4
> +
> +1:
> +	cfi_restore_state
> +	brasl	%r14, __syscall_do_cancel
> +END (__syscall_cancel_arch)
> +libc_hidden_def (__syscall_cancel_arch)
> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
> new file mode 100644
> index 0000000000..3480020fbb
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
> @@ -0,0 +1,83 @@
> +/* Cancellable syscall wrapper.  Linux/s390x version.
> +   Copyright (C) 2019 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 <sysdep.h>
> +
> +/* long int __syscall_cancel_arch (int *cancelhandling,
> +				   __syscall_arg_t nr,
> +				   __syscall_arg_t arg1,
> +				   __syscall_arg_t arg2,
> +				   __syscall_arg_t arg3,
> +				   __syscall_arg_t arg4,
> +				   __syscall_arg_t arg5,
> +				   __syscall_arg_t arg6)  */
> +
> +ENTRY (__syscall_cancel_arch)
> +	stmg	%r6, %r15, 48(%r15)
Please omit the spaces between the operands to have the same style as 
below (the same applies to multiple instructions in the s390-32 file.

> +	cfi_offset (6, -112)
Please use e.g. %r6 instead of just 6 for the cfi statements here and 
below for cfi_restore.
> +	cfi_offset (7, -104)
> +	cfi_offset (8, -96)
> +	cfi_offset (9, -88)
> +	cfi_offset (10, -80)
> +	cfi_offset (11, -72)
> +	cfi_offset (12, -64)
> +	cfi_offset (13, -56)
> +	cfi_offset (14, -48)
> +	cfi_offset (15, -40)
> +	aghi	%r15,-160
> +	cfi_def_cfa_offset (320)
I think the new stack frame is not needed at all.  The kernel does not 
clobber any registers and also does not need an own stack frame.
(I've recognized that syscall.S also contains an extra frame and I will 
work on a patch)

Then r8-r15 are not clobbered here and we just have to store and restore 
r6 and r7.
> +
> +	.globl __syscall_cancel_arch_start
> +	.type  __syscall_cancel_arch_start,@function
> +__syscall_cancel_arch_start:
Could you add the comments like e.g. done in the x86_64 patch?
	/* if (*cancelhandling & CANCELED_BITMASK)
	     __syscall_do_cancel()  */
> +	l	%r0,0(%r2)
> +	tmll	%r0,4
We can combine l and tmll here:
tm 3(%r2),4

> +	jne	1f
	/* Issue a 6 argument syscall, the nr [%r1] being the syscall
	   number.  */
> +	lgr	%r1,%r3
> +	lgr	%r2,%r4
> +	lgr	%r3,%r5
> +	lgr	%r4,%r6
> +	lg	%r5,320(%r15)
> +	lg	%r6,328(%r15)
> +	lg	%r7,336(%r15)
We can use lmg in order to load r5-r7.
> +	svc	0
> +
> +	.globl __syscall_cancel_arch_end
> +	.type  __syscall_cancel_arch_end,@function
> +__syscall_cancel_arch_end:
> +	lg	%r4,272(%r15)
> +	lmg	%r6,%r15,208(%r15)
> +	cfi_remember_state
> +	cfi_restore (15)
> +	cfi_restore (14)
> +	cfi_restore (13)
> +	cfi_restore (12)
> +	cfi_restore (11)
> +	cfi_restore (10)
> +	cfi_restore (9)
> +	cfi_restore (8)
> +	cfi_restore (7)
> +	cfi_restore (6)
> +	cfi_def_cfa_offset (160)
> +	br	%r4
> +
> +1:
> +	cfi_restore_state
> +	brasl	%r14, __syscall_do_cancel
We can just use jg __syscall_do_cancel for this not-returning-tail-call.
(For the s390-32 part: Both instructions are z900 instructions, but as 
gcc has removed support for g5 and g6, this is okay at this point of time)
> +END (__syscall_cancel_arch)
> +libc_hidden_def (__syscall_cancel_arch)
>
Adhemerval Zanella Oct. 17, 2019, 1:53 p.m. UTC | #2
On 16/10/2019 12:46, Stefan Liebler wrote:
> Hi Adhemerval,
> 
> I've added some notes below to the s390-64 file, but the same applies also for s390-32. I've also attached a diff.
> 
> I've also recognized that a call starting from e.g. write () involves various shuffling of the argument registers at each level:
> write (ARGS in r2-r4)
> -> __syscall_cancel (r2=nr, ARGS in r3-r6 and two stack-slots)
> --> __syscall_cancel_arch (r2=*ch, r3=nr, ARGS in r4-r6 and three stack-slots)
> ---> "syscall-instruction" (ARGS in r2-r7)
> 
> Just as a quick idea (I don't know if there are other limitations), those shuffling instructions could perhaps be omitted if the nr / ch arguments of the __syscall_cancel / __syscall_cancel_arch functions would be the last arguments instead of the first ones.
> I assume that also other archs could benefit from such an ordering.

Thanks, I have applied your changes.  Indeed for some architectures the 
syscall_cancel.S might not be the most optimized one, I used the reference 
C implementation as base and gcc might not generate the best code in some
cases.

> 
> Bye,
> Stefan
> 
> On 10/14/19 10:56 PM, Adhemerval Zanella wrote:
>> This patch adds the s390 modifications required for the BZ#12683 fix
>> by adding the arch-specific cancellation syscall bridge.
>>
>> Checked on s390-linux-gnu and s390x-linux-gnu.
>> ---
>>   .../sysv/linux/s390/s390-32/syscall_cancel.S  | 83 +++++++++++++++++++
>>   .../sysv/linux/s390/s390-64/syscall_cancel.S  | 83 +++++++++++++++++++
>>   2 files changed, 166 insertions(+)
>>   create mode 100644 sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>>   create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>>
>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>> new file mode 100644
>> index 0000000000..3c934addbd
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>> @@ -0,0 +1,83 @@
>> +/* Cancellable syscall wrapper.  Linux/s390 version.
>> +   Copyright (C) 2019 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 <sysdep.h>
>> +
>> +/* long int __syscall_cancel_arch (int *cancelhandling,
>> +                   __syscall_arg_t nr,
>> +                   __syscall_arg_t arg1,
>> +                   __syscall_arg_t arg2,
>> +                   __syscall_arg_t arg3,
>> +                   __syscall_arg_t arg4,
>> +                   __syscall_arg_t arg5,
>> +                   __syscall_arg_t arg6)  */
>> +
>> +ENTRY (__syscall_cancel_arch)
>> +    stm    %r6,%r15,24(%r15)
>> +    cfi_offset (6, -72)
>> +    cfi_offset (7, -68)
>> +    cfi_offset (8, -64)
>> +    cfi_offset (9, -60)
>> +    cfi_offset (10, -56)
>> +    cfi_offset (11, -52)
>> +    cfi_offset (12, -48)
>> +    cfi_offset (13, -44)
>> +    cfi_offset (14, -40)
>> +    cfi_offset (15, -36)
>> +    ahi    %r15, -96
>> +    cfi_def_cfa_offset (192)
>> +
>> +    .globl __syscall_cancel_arch_start
>> +    .type  __syscall_cancel_arch_start,@function
>> +__syscall_cancel_arch_start:
>> +    l    %r0, 0(%r2)
>> +    tml    %r0, 4
>> +    jne    1f
>> +    lr    %r1, %r3
>> +    lr    %r2, %r4
>> +    lr    %r3, %r5
>> +    lr    %r4, %r6
>> +    l    %r5, 192(%r15)
>> +    l    %r6, 196(%r15)
>> +    l    %r7, 200(%r15)
>> +    svc    0
>> +
>> +    .globl __syscall_cancel_arch_end
>> +    .type  __syscall_cancel_arch_end,@function
>> +__syscall_cancel_arch_end:
>> +    l    %r4, 152(%r15)
>> +    lm    %r6, %r15, 120(%r15)
>> +    cfi_remember_state
>> +    cfi_restore (15)
>> +    cfi_restore (14)
>> +    cfi_restore (13)
>> +    cfi_restore (12)
>> +    cfi_restore (11)
>> +    cfi_restore (10)
>> +    cfi_restore (9)
>> +    cfi_restore (8)
>> +    cfi_restore (7)
>> +    cfi_restore (6)
>> +    cfi_def_cfa_offset (96)
>> +    br    %r4
>> +
>> +1:
>> +    cfi_restore_state
>> +    brasl    %r14, __syscall_do_cancel
>> +END (__syscall_cancel_arch)
>> +libc_hidden_def (__syscall_cancel_arch)
>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>> new file mode 100644
>> index 0000000000..3480020fbb
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>> @@ -0,0 +1,83 @@
>> +/* Cancellable syscall wrapper.  Linux/s390x version.
>> +   Copyright (C) 2019 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 <sysdep.h>
>> +
>> +/* long int __syscall_cancel_arch (int *cancelhandling,
>> +                   __syscall_arg_t nr,
>> +                   __syscall_arg_t arg1,
>> +                   __syscall_arg_t arg2,
>> +                   __syscall_arg_t arg3,
>> +                   __syscall_arg_t arg4,
>> +                   __syscall_arg_t arg5,
>> +                   __syscall_arg_t arg6)  */
>> +
>> +ENTRY (__syscall_cancel_arch)
>> +    stmg    %r6, %r15, 48(%r15)
> Please omit the spaces between the operands to have the same style as below (the same applies to multiple instructions in the s390-32 file.
> 

Ack.

>> +    cfi_offset (6, -112)
> Please use e.g. %r6 instead of just 6 for the cfi statements here and below for cfi_restore.
>> +    cfi_offset (7, -104)
>> +    cfi_offset (8, -96)
>> +    cfi_offset (9, -88)
>> +    cfi_offset (10, -80)
>> +    cfi_offset (11, -72)
>> +    cfi_offset (12, -64)
>> +    cfi_offset (13, -56)
>> +    cfi_offset (14, -48)
>> +    cfi_offset (15, -40)
>> +    aghi    %r15,-160
>> +    cfi_def_cfa_offset (320)
> I think the new stack frame is not needed at all.  The kernel does not clobber any registers and also does not need an own stack frame.
> (I've recognized that syscall.S also contains an extra frame and I will work on a patch)
> 
> Then r8-r15 are not clobbered here and we just have to store and restore r6 and r7.

ack.

>> +
>> +    .globl __syscall_cancel_arch_start
>> +    .type  __syscall_cancel_arch_start,@function
>> +__syscall_cancel_arch_start:
> Could you add the comments like e.g. done in the x86_64 patch?
>     /* if (*cancelhandling & CANCELED_BITMASK)
>          __syscall_do_cancel()  */

Certainly, I also changes to use TCB_CANCELED_BITMASK from tcb-offsets.h
instead of hard-code the values.

>> +    l    %r0,0(%r2)
>> +    tmll    %r0,4
> We can combine l and tmll here:
> tm 3(%r2),4
> 

Ack.

>> +    jne    1f
>     /* Issue a 6 argument syscall, the nr [%r1] being the syscall
>        number.  */
>> +    lgr    %r1,%r3
>> +    lgr    %r2,%r4
>> +    lgr    %r3,%r5
>> +    lgr    %r4,%r6
>> +    lg    %r5,320(%r15)
>> +    lg    %r6,328(%r15)
>> +    lg    %r7,336(%r15)
> We can use lmg in order to load r5-r7.

Ack.

>> +    svc    0
>> +
>> +    .globl __syscall_cancel_arch_end
>> +    .type  __syscall_cancel_arch_end,@function
>> +__syscall_cancel_arch_end:
>> +    lg    %r4,272(%r15)
>> +    lmg    %r6,%r15,208(%r15)
>> +    cfi_remember_state
>> +    cfi_restore (15)
>> +    cfi_restore (14)
>> +    cfi_restore (13)
>> +    cfi_restore (12)
>> +    cfi_restore (11)
>> +    cfi_restore (10)
>> +    cfi_restore (9)
>> +    cfi_restore (8)
>> +    cfi_restore (7)
>> +    cfi_restore (6)
>> +    cfi_def_cfa_offset (160)
>> +    br    %r4
>> +
>> +1:
>> +    cfi_restore_state
>> +    brasl    %r14, __syscall_do_cancel
> We can just use jg __syscall_do_cancel for this not-returning-tail-call.
> (For the s390-32 part: Both instructions are z900 instructions, but as gcc has removed support for g5 and g6, this is okay at this point of time)
>> +END (__syscall_cancel_arch)
>> +libc_hidden_def (__syscall_cancel_arch)
>>
> 

Ack.
Stefan Liebler Oct. 17, 2019, 3 p.m. UTC | #3
On 10/17/19 3:53 PM, Adhemerval Zanella wrote:
> 
> 
> On 16/10/2019 12:46, Stefan Liebler wrote:
>> Hi Adhemerval,
>>
>> I've added some notes below to the s390-64 file, but the same applies also for s390-32. I've also attached a diff.
>>
>> I've also recognized that a call starting from e.g. write () involves various shuffling of the argument registers at each level:
>> write (ARGS in r2-r4)
>> -> __syscall_cancel (r2=nr, ARGS in r3-r6 and two stack-slots)
>> --> __syscall_cancel_arch (r2=*ch, r3=nr, ARGS in r4-r6 and three stack-slots)
>> ---> "syscall-instruction" (ARGS in r2-r7)
>>
>> Just as a quick idea (I don't know if there are other limitations), those shuffling instructions could perhaps be omitted if the nr / ch arguments of the __syscall_cancel / __syscall_cancel_arch functions would be the last arguments instead of the first ones.
>> I assume that also other archs could benefit from such an ordering.
> 
> Thanks, I have applied your changes.  Indeed for some architectures the
> syscall_cancel.S might not be the most optimized one, I used the reference
> C implementation as base and gcc might not generate the best code in some
> cases.
> 
The implementation in syscall_cancel.S contains just one of the three 
parts of the mentioned register move instructions. Those needs also to 
be generated in write() and __syscall_cancel().
My point was, if we could preserve the values passed in registers to 
e.g. write() in the same registers until the syscall is invoked in 
__syscall_cancel_arch, we won't need to move them from registers to 
registers at all:
write (ARGS in r2-r4)
-> __syscall_cancel (ARGS in r2-r6 and first stack-slot, nr in second 
stack-slot)
--> __syscall_cacnel_arch (ARGS in r2-r6 and first stack-slot, nr in 
second stack-slot, ch in third stack-slot)
---> "syscall-instruction" (ARGS in r2-r7)

>>
>> Bye,
>> Stefan
>>
>> On 10/14/19 10:56 PM, Adhemerval Zanella wrote:
>>> This patch adds the s390 modifications required for the BZ#12683 fix
>>> by adding the arch-specific cancellation syscall bridge.
>>>
>>> Checked on s390-linux-gnu and s390x-linux-gnu.
>>> ---
>>>    .../sysv/linux/s390/s390-32/syscall_cancel.S  | 83 +++++++++++++++++++
>>>    .../sysv/linux/s390/s390-64/syscall_cancel.S  | 83 +++++++++++++++++++
>>>    2 files changed, 166 insertions(+)
>>>    create mode 100644 sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>>>    create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>>> new file mode 100644
>>> index 0000000000..3c934addbd
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>>> @@ -0,0 +1,83 @@
>>> +/* Cancellable syscall wrapper.  Linux/s390 version.
>>> +   Copyright (C) 2019 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 <sysdep.h>
>>> +
>>> +/* long int __syscall_cancel_arch (int *cancelhandling,
>>> +                   __syscall_arg_t nr,
>>> +                   __syscall_arg_t arg1,
>>> +                   __syscall_arg_t arg2,
>>> +                   __syscall_arg_t arg3,
>>> +                   __syscall_arg_t arg4,
>>> +                   __syscall_arg_t arg5,
>>> +                   __syscall_arg_t arg6)  */
>>> +
>>> +ENTRY (__syscall_cancel_arch)
>>> +    stm    %r6,%r15,24(%r15)
>>> +    cfi_offset (6, -72)
>>> +    cfi_offset (7, -68)
>>> +    cfi_offset (8, -64)
>>> +    cfi_offset (9, -60)
>>> +    cfi_offset (10, -56)
>>> +    cfi_offset (11, -52)
>>> +    cfi_offset (12, -48)
>>> +    cfi_offset (13, -44)
>>> +    cfi_offset (14, -40)
>>> +    cfi_offset (15, -36)
>>> +    ahi    %r15, -96
>>> +    cfi_def_cfa_offset (192)
>>> +
>>> +    .globl __syscall_cancel_arch_start
>>> +    .type  __syscall_cancel_arch_start,@function
>>> +__syscall_cancel_arch_start:
>>> +    l    %r0, 0(%r2)
>>> +    tml    %r0, 4
>>> +    jne    1f
>>> +    lr    %r1, %r3
>>> +    lr    %r2, %r4
>>> +    lr    %r3, %r5
>>> +    lr    %r4, %r6
>>> +    l    %r5, 192(%r15)
>>> +    l    %r6, 196(%r15)
>>> +    l    %r7, 200(%r15)
>>> +    svc    0
>>> +
>>> +    .globl __syscall_cancel_arch_end
>>> +    .type  __syscall_cancel_arch_end,@function
>>> +__syscall_cancel_arch_end:
>>> +    l    %r4, 152(%r15)
>>> +    lm    %r6, %r15, 120(%r15)
>>> +    cfi_remember_state
>>> +    cfi_restore (15)
>>> +    cfi_restore (14)
>>> +    cfi_restore (13)
>>> +    cfi_restore (12)
>>> +    cfi_restore (11)
>>> +    cfi_restore (10)
>>> +    cfi_restore (9)
>>> +    cfi_restore (8)
>>> +    cfi_restore (7)
>>> +    cfi_restore (6)
>>> +    cfi_def_cfa_offset (96)
>>> +    br    %r4
>>> +
>>> +1:
>>> +    cfi_restore_state
>>> +    brasl    %r14, __syscall_do_cancel
>>> +END (__syscall_cancel_arch)
>>> +libc_hidden_def (__syscall_cancel_arch)
>>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>>> new file mode 100644
>>> index 0000000000..3480020fbb
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>>> @@ -0,0 +1,83 @@
>>> +/* Cancellable syscall wrapper.  Linux/s390x version.
>>> +   Copyright (C) 2019 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 <sysdep.h>
>>> +
>>> +/* long int __syscall_cancel_arch (int *cancelhandling,
>>> +                   __syscall_arg_t nr,
>>> +                   __syscall_arg_t arg1,
>>> +                   __syscall_arg_t arg2,
>>> +                   __syscall_arg_t arg3,
>>> +                   __syscall_arg_t arg4,
>>> +                   __syscall_arg_t arg5,
>>> +                   __syscall_arg_t arg6)  */
>>> +
>>> +ENTRY (__syscall_cancel_arch)
>>> +    stmg    %r6, %r15, 48(%r15)
>> Please omit the spaces between the operands to have the same style as below (the same applies to multiple instructions in the s390-32 file.
>>
> 
> Ack.
> 
>>> +    cfi_offset (6, -112)
>> Please use e.g. %r6 instead of just 6 for the cfi statements here and below for cfi_restore.
>>> +    cfi_offset (7, -104)
>>> +    cfi_offset (8, -96)
>>> +    cfi_offset (9, -88)
>>> +    cfi_offset (10, -80)
>>> +    cfi_offset (11, -72)
>>> +    cfi_offset (12, -64)
>>> +    cfi_offset (13, -56)
>>> +    cfi_offset (14, -48)
>>> +    cfi_offset (15, -40)
>>> +    aghi    %r15,-160
>>> +    cfi_def_cfa_offset (320)
>> I think the new stack frame is not needed at all.  The kernel does not clobber any registers and also does not need an own stack frame.
>> (I've recognized that syscall.S also contains an extra frame and I will work on a patch)
>>
>> Then r8-r15 are not clobbered here and we just have to store and restore r6 and r7.
> 
> ack.
> 
>>> +
>>> +    .globl __syscall_cancel_arch_start
>>> +    .type  __syscall_cancel_arch_start,@function
>>> +__syscall_cancel_arch_start:
>> Could you add the comments like e.g. done in the x86_64 patch?
>>      /* if (*cancelhandling & CANCELED_BITMASK)
>>           __syscall_do_cancel()  */
> 
> Certainly, I also changes to use TCB_CANCELED_BITMASK from tcb-offsets.h
> instead of hard-code the values.
> 
>>> +    l    %r0,0(%r2)
>>> +    tmll    %r0,4
>> We can combine l and tmll here:
>> tm 3(%r2),4
>>
> 
> Ack.
> 
>>> +    jne    1f
>>      /* Issue a 6 argument syscall, the nr [%r1] being the syscall
>>         number.  */
>>> +    lgr    %r1,%r3
>>> +    lgr    %r2,%r4
>>> +    lgr    %r3,%r5
>>> +    lgr    %r4,%r6
>>> +    lg    %r5,320(%r15)
>>> +    lg    %r6,328(%r15)
>>> +    lg    %r7,336(%r15)
>> We can use lmg in order to load r5-r7.
> 
> Ack.
> 
>>> +    svc    0
>>> +
>>> +    .globl __syscall_cancel_arch_end
>>> +    .type  __syscall_cancel_arch_end,@function
>>> +__syscall_cancel_arch_end:
>>> +    lg    %r4,272(%r15)
>>> +    lmg    %r6,%r15,208(%r15)
>>> +    cfi_remember_state
>>> +    cfi_restore (15)
>>> +    cfi_restore (14)
>>> +    cfi_restore (13)
>>> +    cfi_restore (12)
>>> +    cfi_restore (11)
>>> +    cfi_restore (10)
>>> +    cfi_restore (9)
>>> +    cfi_restore (8)
>>> +    cfi_restore (7)
>>> +    cfi_restore (6)
>>> +    cfi_def_cfa_offset (160)
>>> +    br    %r4
>>> +
>>> +1:
>>> +    cfi_restore_state
>>> +    brasl    %r14, __syscall_do_cancel
>> We can just use jg __syscall_do_cancel for this not-returning-tail-call.
>> (For the s390-32 part: Both instructions are z900 instructions, but as gcc has removed support for g5 and g6, this is okay at this point of time)
>>> +END (__syscall_cancel_arch)
>>> +libc_hidden_def (__syscall_cancel_arch)
>>>
>>
> 
> Ack.
>
Adhemerval Zanella Oct. 17, 2019, 7:46 p.m. UTC | #4
On 17/10/2019 12:00, Stefan Liebler wrote:
> On 10/17/19 3:53 PM, Adhemerval Zanella wrote:
>>
>>
>> On 16/10/2019 12:46, Stefan Liebler wrote:
>>> Hi Adhemerval,
>>>
>>> I've added some notes below to the s390-64 file, but the same applies also for s390-32. I've also attached a diff.
>>>
>>> I've also recognized that a call starting from e.g. write () involves various shuffling of the argument registers at each level:
>>> write (ARGS in r2-r4)
>>> -> __syscall_cancel (r2=nr, ARGS in r3-r6 and two stack-slots)
>>> --> __syscall_cancel_arch (r2=*ch, r3=nr, ARGS in r4-r6 and three stack-slots)
>>> ---> "syscall-instruction" (ARGS in r2-r7)
>>>
>>> Just as a quick idea (I don't know if there are other limitations), those shuffling instructions could perhaps be omitted if the nr / ch arguments of the __syscall_cancel / __syscall_cancel_arch functions would be the last arguments instead of the first ones.
>>> I assume that also other archs could benefit from such an ordering.
>>
>> Thanks, I have applied your changes.  Indeed for some architectures the
>> syscall_cancel.S might not be the most optimized one, I used the reference
>> C implementation as base and gcc might not generate the best code in some
>> cases.
>>
> The implementation in syscall_cancel.S contains just one of the three parts of the mentioned register move instructions. Those needs also to be generated in write() and __syscall_cancel().
> My point was, if we could preserve the values passed in registers to e.g. write() in the same registers until the syscall is invoked in __syscall_cancel_arch, we won't need to move them from registers to registers at all:
> write (ARGS in r2-r4)
> -> __syscall_cancel (ARGS in r2-r6 and first stack-slot, nr in second stack-slot)
> --> __syscall_cacnel_arch (ARGS in r2-r6 and first stack-slot, nr in second stack-slot, ch in third stack-slot)
> ---> "syscall-instruction" (ARGS in r2-r7)

Well I think it might be feasible, so for instance write would call:

ssize_t _libc_write (int fd, const void *buf, size_t nbytes)
\_ __syscall_cancel (fd, buf, nbytes, 0, 0, 0, __NR_write)
   \_ __syscall_cancel_arch (fd, buf, nbytes, 0, 0, 0, __NR_write, &pd->cancellation)
      ...

I don't have a strong opinion here, I would expect that since the
cancellation points are potentially blocking syscall both the
latency of the syscall itself and the potentially unbounded time
spent in kernel might shadow any potentially micro-optimization in
argument shuffling. 

Also, this patch has the advantage of removing the atomic operation
from __pthread_{enable,disable}_asynccancel, which for some 
architectures are way costly than register shuffling or spilling.
Stefan Liebler Oct. 18, 2019, 12:58 p.m. UTC | #5
On 10/17/19 9:46 PM, Adhemerval Zanella wrote:
> 
> 
> On 17/10/2019 12:00, Stefan Liebler wrote:
>> On 10/17/19 3:53 PM, Adhemerval Zanella wrote:
>>>
>>>
>>> On 16/10/2019 12:46, Stefan Liebler wrote:
>>>> Hi Adhemerval,
>>>>
>>>> I've added some notes below to the s390-64 file, but the same applies also for s390-32. I've also attached a diff.
>>>>
>>>> I've also recognized that a call starting from e.g. write () involves various shuffling of the argument registers at each level:
>>>> write (ARGS in r2-r4)
>>>> -> __syscall_cancel (r2=nr, ARGS in r3-r6 and two stack-slots)
>>>> --> __syscall_cancel_arch (r2=*ch, r3=nr, ARGS in r4-r6 and three stack-slots)
>>>> ---> "syscall-instruction" (ARGS in r2-r7)
>>>>
>>>> Just as a quick idea (I don't know if there are other limitations), those shuffling instructions could perhaps be omitted if the nr / ch arguments of the __syscall_cancel / __syscall_cancel_arch functions would be the last arguments instead of the first ones.
>>>> I assume that also other archs could benefit from such an ordering.
>>>
>>> Thanks, I have applied your changes.  Indeed for some architectures the
>>> syscall_cancel.S might not be the most optimized one, I used the reference
>>> C implementation as base and gcc might not generate the best code in some
>>> cases.
>>>
>> The implementation in syscall_cancel.S contains just one of the three parts of the mentioned register move instructions. Those needs also to be generated in write() and __syscall_cancel().
>> My point was, if we could preserve the values passed in registers to e.g. write() in the same registers until the syscall is invoked in __syscall_cancel_arch, we won't need to move them from registers to registers at all:
>> write (ARGS in r2-r4)
>> -> __syscall_cancel (ARGS in r2-r6 and first stack-slot, nr in second stack-slot)
>> --> __syscall_cacnel_arch (ARGS in r2-r6 and first stack-slot, nr in second stack-slot, ch in third stack-slot)
>> ---> "syscall-instruction" (ARGS in r2-r7)
> 
> Well I think it might be feasible, so for instance write would call:
> 
> ssize_t _libc_write (int fd, const void *buf, size_t nbytes)
> \_ __syscall_cancel (fd, buf, nbytes, 0, 0, 0, __NR_write)
>     \_ __syscall_cancel_arch (fd, buf, nbytes, 0, 0, 0, __NR_write, &pd->cancellation)
>        ...
> 
> I don't have a strong opinion here, I would expect that since the
> cancellation points are potentially blocking syscall both the
> latency of the syscall itself and the potentially unbounded time
> spent in kernel might shadow any potentially micro-optimization in
> argument shuffling.
Sure. The syscall spends the most time.
Beneath the timing aspect, this also affects the code-size in various 
places. And if somebody reads / debugs the code starting from e.g. write 
until the syscall, the instruction sequence looks confusing. (Not only 
on s390)

> 
> Also, this patch has the advantage of removing the atomic operation
> from __pthread_{enable,disable}_asynccancel, which for some
> architectures are way costly than register shuffling or spilling.

Patch
diff mbox series

diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
new file mode 100644
index 0000000000..3c934addbd
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
@@ -0,0 +1,83 @@ 
+/* Cancellable syscall wrapper.  Linux/s390 version.
+   Copyright (C) 2019 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 <sysdep.h>
+
+/* long int __syscall_cancel_arch (int *cancelhandling,
+				   __syscall_arg_t nr,
+				   __syscall_arg_t arg1,
+				   __syscall_arg_t arg2,
+				   __syscall_arg_t arg3,
+				   __syscall_arg_t arg4,
+				   __syscall_arg_t arg5,
+				   __syscall_arg_t arg6)  */
+
+ENTRY (__syscall_cancel_arch)
+	stm	%r6,%r15,24(%r15)
+	cfi_offset (6, -72)
+	cfi_offset (7, -68)
+	cfi_offset (8, -64)
+	cfi_offset (9, -60)
+	cfi_offset (10, -56)
+	cfi_offset (11, -52)
+	cfi_offset (12, -48)
+	cfi_offset (13, -44)
+	cfi_offset (14, -40)
+	cfi_offset (15, -36)
+	ahi	%r15, -96
+	cfi_def_cfa_offset (192)
+
+	.globl __syscall_cancel_arch_start
+	.type  __syscall_cancel_arch_start,@function
+__syscall_cancel_arch_start:
+	l	%r0, 0(%r2)
+	tml	%r0, 4
+	jne	1f
+	lr	%r1, %r3
+	lr	%r2, %r4
+	lr	%r3, %r5
+	lr	%r4, %r6
+	l	%r5, 192(%r15)
+	l	%r6, 196(%r15)
+	l	%r7, 200(%r15)
+	svc	0
+
+	.globl __syscall_cancel_arch_end
+	.type  __syscall_cancel_arch_end,@function
+__syscall_cancel_arch_end:
+	l	%r4, 152(%r15)
+	lm	%r6, %r15, 120(%r15)
+	cfi_remember_state
+	cfi_restore (15)
+	cfi_restore (14)
+	cfi_restore (13)
+	cfi_restore (12)
+	cfi_restore (11)
+	cfi_restore (10)
+	cfi_restore (9)
+	cfi_restore (8)
+	cfi_restore (7)
+	cfi_restore (6)
+	cfi_def_cfa_offset (96)
+	br	%r4
+
+1:
+	cfi_restore_state
+	brasl	%r14, __syscall_do_cancel
+END (__syscall_cancel_arch)
+libc_hidden_def (__syscall_cancel_arch)
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
new file mode 100644
index 0000000000..3480020fbb
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
@@ -0,0 +1,83 @@ 
+/* Cancellable syscall wrapper.  Linux/s390x version.
+   Copyright (C) 2019 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 <sysdep.h>
+
+/* long int __syscall_cancel_arch (int *cancelhandling,
+				   __syscall_arg_t nr,
+				   __syscall_arg_t arg1,
+				   __syscall_arg_t arg2,
+				   __syscall_arg_t arg3,
+				   __syscall_arg_t arg4,
+				   __syscall_arg_t arg5,
+				   __syscall_arg_t arg6)  */
+
+ENTRY (__syscall_cancel_arch)
+	stmg	%r6, %r15, 48(%r15)
+	cfi_offset (6, -112)
+	cfi_offset (7, -104)
+	cfi_offset (8, -96)
+	cfi_offset (9, -88)
+	cfi_offset (10, -80)
+	cfi_offset (11, -72)
+	cfi_offset (12, -64)
+	cfi_offset (13, -56)
+	cfi_offset (14, -48)
+	cfi_offset (15, -40)
+	aghi	%r15,-160
+	cfi_def_cfa_offset (320)
+
+	.globl __syscall_cancel_arch_start
+	.type  __syscall_cancel_arch_start,@function
+__syscall_cancel_arch_start:
+	l	%r0,0(%r2)
+	tmll	%r0,4
+	jne	1f
+	lgr	%r1,%r3
+	lgr	%r2,%r4
+	lgr	%r3,%r5
+	lgr	%r4,%r6
+	lg	%r5,320(%r15)
+	lg	%r6,328(%r15)
+	lg	%r7,336(%r15)
+	svc	0
+
+	.globl __syscall_cancel_arch_end
+	.type  __syscall_cancel_arch_end,@function
+__syscall_cancel_arch_end:
+	lg	%r4,272(%r15)
+	lmg	%r6,%r15,208(%r15)
+	cfi_remember_state
+	cfi_restore (15)
+	cfi_restore (14)
+	cfi_restore (13)
+	cfi_restore (12)
+	cfi_restore (11)
+	cfi_restore (10)
+	cfi_restore (9)
+	cfi_restore (8)
+	cfi_restore (7)
+	cfi_restore (6)
+	cfi_def_cfa_offset (160)
+	br	%r4
+
+1:
+	cfi_restore_state
+	brasl	%r14, __syscall_do_cancel
+END (__syscall_cancel_arch)
+libc_hidden_def (__syscall_cancel_arch)