From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: libc-alpha@sourceware.org
Subject: Re: [PATCH v3 18/21] nptl: s390: Fix Race conditions in pthread cancellation (BZ#12683)
Date: Thu, 17 Oct 2019 13:54:00 -0000 [thread overview]
Message-ID: <e667c845-f4b5-8b02-d2f3-fe0b311ecf96@linaro.org> (raw)
In-Reply-To: <7092d7c2-71fb-b130-9923-d80a9786c6b9@linux.ibm.com>
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.
next prev parent reply other threads:[~2019-10-17 13:54 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-14 20:57 [PATCH v3 00/21] nptl: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 18/21] nptl: s390: " Adhemerval Zanella
2019-10-16 15:46 ` Stefan Liebler
2019-10-17 13:54 ` Adhemerval Zanella [this message]
2019-10-17 15:01 ` Stefan Liebler
2019-10-17 19:46 ` Adhemerval Zanella
2019-10-18 12:58 ` Stefan Liebler
2019-10-14 20:57 ` [PATCH v3 11/21] nptl: microblaze: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 12/21] nptl: sparc: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 15/21] nptl: alpha: " Adhemerval Zanella
2019-10-17 3:01 ` Matt Turner
2019-10-14 20:57 ` [PATCH v3 20/21] nptl: csky: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 04/21] nptl: x32: " Adhemerval Zanella
2019-10-15 11:06 ` Florian Weimer
2019-10-14 20:57 ` [PATCH v3 19/21] nptl: nios2: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 03/21] nptl: x86_64: " Adhemerval Zanella
2019-10-15 11:03 ` Florian Weimer
2019-10-16 21:22 ` Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 01/21] nptl: Handle EPIPE on tst-cancel2 Adhemerval Zanella
2019-10-15 9:03 ` Florian Weimer
2019-10-14 20:57 ` [PATCH v3 13/21] nptl: hppa: Fix Race conditions in pthread cancellation (BZ#12683) Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 10/21] nptl: powerpc: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 16/21] nptl: sh: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 07/21] nptl: i386: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 09/21] nptl: arm: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 06/21] nptl: mips: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 08/21] nptl: aarch64: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 05/21] nptl: ia64: " Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 02/21] nptl: " Adhemerval Zanella
2019-10-15 10:56 ` Florian Weimer
2019-10-16 20:42 ` Adhemerval Zanella
2019-10-18 12:38 ` Internal SIGTIMER use (was: Re: [PATCH v3 02/21] nptl: Fix Race conditions in pthread cancellation (BZ#12683)) Florian Weimer
2019-10-21 13:29 ` Adhemerval Zanella
2019-10-14 20:57 ` [PATCH v3 17/21] nptl: riscv: Fix Race conditions in pthread cancellation (BZ#12683) Adhemerval Zanella
2019-10-14 23:22 ` Andrew Waterman
2019-10-14 20:57 ` [PATCH v3 14/21] nptl: m68k: " Adhemerval Zanella
2019-10-14 21:13 ` Andreas Schwab
2019-10-14 20:58 ` [PATCH v3 21/21] Remove sysdep-cancel header 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=e667c845-f4b5-8b02-d2f3-fe0b311ecf96@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
/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).