2020-11-02 17:01:45

by Qian Cai

[permalink] [raw]
Subject: Re: [PATCH] s390: add support for TIF_NOTIFY_SIGNAL

On Sun, 2020-11-01 at 17:31 +0000, Heiko Carstens wrote:
> On Thu, Oct 29, 2020 at 10:21:11AM -0600, Jens Axboe wrote:
> > Wire up TIF_NOTIFY_SIGNAL handling for s390.
> >
> > Cc: [email protected]
> > Signed-off-by: Jens Axboe <[email protected]>

Even though I did confirm that today's linux-next contains this additional patch
from Heiko below, a z10 guest is still unable to boot. Reverting the whole
series (reverting only "s390: add support for TIF_NOTIFY_SIGNAL" introduced
compiling errors) fixed the problem, i.e., git revert --no-edit
af0dd809f3d3..7b074c15374c [1]

.config: https://cailca.coding.net/public/linux/mm/git/files/master/s390.config

01: [ 3.284902] systemd[1]: systemd 239 (239-40.el8) running in system mode.
01: (+PAM +AUDIT +SELINUX +IMA -APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +
01: GCRYPT +GNUTLS +ACL +XZ +LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD +IDN2 -IDN +PCR
01: E2 default-hierarchy=legacy)
01: [ 3.285558] systemd[1]: Detected virtualization zvm.
01: [ 3.285585] systemd[1]: Detected architecture s390x.
01: [ 3.285618] systemd[1]: Running in initial RAM disk.
01: [ 3.376459] systemd[1]: Set hostname to <ibm-z-137.rhts.eng.bos.redhat.co
01: m>.
01: [ 3.464950] mkdir (45) used greatest stack depth: 57824 bytes left
01:
01: Welcome to [0;34mRed Hat Enterprise Linux 8.3 (Ootpa) dracut-049-95.git20200
01: 804.el8 (Initramfs)[0m!
01:
00: [ 87.908107] random: crng init done

01: [ 490.492263] INFO: task (sd-executor):42 can't die for more than 368 secon
01: ds.
01: [ 490.492303] task:(sd-executor) state:R running task stack:58984 pi
01: d: 42 ppid: 1 flags:0x00000002
01: [ 490.492359] Call Trace:
01: [ 490.492382] [<00000000163f0652>] __schedule+0xa12/0x1840
01: [ 490.492391] [<00000000163f1562>] schedule+0xe2/0x310
(inlined by) __preempt_count_add at arch/s390/include/asm/preempt.h:56
(discriminator 1)
(inlined by) __preempt_count_sub at arch/s390/include/asm/preempt.h:63
(discriminator 1)
(inlined by) schedule at kernel/sched/core.c:4602 (discriminator 1)
01: [ 490.492399] [<000000001640390a>] system_call+0xe2/0x278
system_call at arch/s390/kernel/entry.S:424
01: [ 490.492407] no locks held by (sd-executor)/42.
01: [ 490.492420]
01: [ 490.492420] Showing all locks held in the system:
01: [ 490.492438] 1 lock held by khungtaskd/25:
01: [ 490.492445] #0: 0000000016b92c80 (rcu_read_lock){....}-{1:2}, at: rcu_lo
01: ck_acquire.constprop.54+0x0/0x50
01: [ 490.492481]
01: [ 490.492488] =============================================
01: [ 490.492488]

[1]:
7b074c15374c io_uring: remove 'twa_signal_ok' deadlock work-around
eb48a0f216fa kernel: remove checking for TIF_NOTIFY_SIGNAL
c634e6b63a81 signal: kill JOBCTL_TASK_WORK
f8b667db31a3 io_uring: JOBCTL_TASK_WORK is no longer used by task_work
c50eb9d59bb1 task_work: remove legacy TWA_SIGNAL path
1d48c8d6d71e xtensa: add support for TIF_NOTIFY_SIGNAL
8ef9c750c5a1 um: add support for TIF_NOTIFY_SIGNAL
3f242a158b7c sparc: add support for TIF_NOTIFY_SIGNAL
40c7ac5c4790 sh: add support for TIF_NOTIFY_SIGNAL
5e59963ed1ac riscv: add support for TIF_NOTIFY_SIGNAL
9333d15595e8 openrisc: add support for TIF_NOTIFY_SIGNAL
c34f87ae2e81 nds32: add support for TIF_NOTIFY_SIGNAL
27af2ca0cdda microblaze: add support for TIF_NOTIFY_SIGNAL
ef1863c4081e ia64: add support for TIF_NOTIFY_SIGNAL
58d670021acc hexagon: add support for TIF_NOTIFY_SIGNAL
1facd6bf079c h8300: add support for TIF_NOTIFY_SIGNAL
1b81145fc28d csky: add support for TIF_NOTIFY_SIGNAL
bbc8d03c0bf3 c6x: add support for TIF_NOTIFY_SIGNAL
6cbc413682ac arm: add support for TIF_NOTIFY_SIGNAL
e9822185daa1 alpha: add support for TIF_NOTIFY_SIGNAL
4c3d9c3b415a s390: add support for TIF_NOTIFY_SIGNAL
d0772a4d9367 mips: add support for TIF_NOTIFY_SIGNAL
07246df9ebe4 powerpc: add support for TIF_NOTIFY_SIGNAL
9edbc08ce909 parisc: add support for TIF_NOTIFY_SIGNAL
c96152dd9c01 nios32: add support for TIF_NOTIFY_SIGNAL
89d22e3adff3 m68k: add support for TIF_NOTIFY_SIGNAL
3db7550a998c arm64: add support for TIF_NOTIFY_SIGNAL
9161d936d1ff arc: add support for TIF_NOTIFY_SIGNAL
fdb5f027ce66 task_work: use TIF_NOTIFY_SIGNAL if available
323b0fba756d x86: wire up TIF_NOTIFY_SIGNAL
a1a5bc3e8659 kernel: add support for TIF_NOTIFY_SIGNAL
c0947f6b6ed2 kernel: add task_sigpending() helper

> > ---
> >
> > 5.11 has support queued up for TIF_NOTIFY_SIGNAL, see this posting
> > for details:
> >
> > https://lore.kernel.org/io-uring/[email protected]/
> >
> > As part of that work, I'm adding TIF_NOTIFY_SIGNAL support to all archs,
> > as that will enable a set of cleanups once all of them support it. I'm
> > happy carrying this patch if need be, or it can be funelled through the
> > arch tree. Let me know.
> >
> > arch/s390/include/asm/thread_info.h | 2 ++
> > arch/s390/kernel/entry.S | 7 ++++++-
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/s390/include/asm/thread_info.h
> > b/arch/s390/include/asm/thread_info.h
> > index 13a04fcf7762..0045341ade48 100644
> > --- a/arch/s390/include/asm/thread_info.h
> > +++ b/arch/s390/include/asm/thread_info.h
> > @@ -65,6 +65,7 @@ void arch_setup_new_exec(void);
> > #define TIF_GUARDED_STORAGE 4 /* load guarded storage control
> > block */
> > #define TIF_PATCH_PENDING 5 /* pending live patching update */
> > #define TIF_PGSTE 6 /* New mm's will use 4K page tables */
> > +#define TIF_NOTIFY_SIGNAL 7 /* signal notifications exist */
> > #define TIF_ISOLATE_BP 8 /* Run process with isolated BP */
> > #define TIF_ISOLATE_BP_GUEST 9 /* Run KVM guests with isolated BP
> > */
> >
> > @@ -82,6 +83,7 @@ void arch_setup_new_exec(void);
> > #define TIF_SYSCALL_TRACEPOINT 27 /* syscall tracepoint
> > instrumentation */
> >
> > #define _TIF_NOTIFY_RESUME BIT(TIF_NOTIFY_RESUME)
> > +#define _TIF_NOTIFY_SIGNAL BIT(TIF_NOTIFY_SIGNAL)
> > #define _TIF_SIGPENDING BIT(TIF_SIGPENDING)
> > #define _TIF_NEED_RESCHED BIT(TIF_NEED_RESCHED)
> > #define _TIF_UPROBE BIT(TIF_UPROBE)
> > diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> > index 86235919c2d1..a30d891e8045 100644
> > --- a/arch/s390/kernel/entry.S
> > +++ b/arch/s390/kernel/entry.S
> > @@ -52,7 +52,8 @@ STACK_SIZE = 1 << STACK_SHIFT
> > STACK_INIT = STACK_SIZE - STACK_FRAME_OVERHEAD - __PT_SIZE
> >
> > _TIF_WORK = (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | _TIF_NEED_RESCHED | \
> > - _TIF_UPROBE | _TIF_GUARDED_STORAGE | _TIF_PATCH_PENDING)
> > + _TIF_UPROBE | _TIF_GUARDED_STORAGE | _TIF_PATCH_PENDING | \
> > + _TIF_NOTIFY_SIGNAL)
> > _TIF_TRACE = (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | _TIF_SECCOMP | \
> > _TIF_SYSCALL_TRACEPOINT)
> > _CIF_WORK = (_CIF_ASCE_PRIMARY | _CIF_ASCE_SECONDARY | _CIF_FPU)
> > @@ -463,6 +464,8 @@ ENTRY(system_call)
> > #endif
> > TSTMSK __PT_FLAGS(%r11),_PIF_SYSCALL_RESTART
> > jo .Lsysc_syscall_restart
> > + TSTMSK __TI_flags(%r12),_TIF_NOTIFY_SIGNAL
> > + jo .Lsysc_sigpending
> > TSTMSK __TI_flags(%r12),_TIF_SIGPENDING
> > jo .Lsysc_sigpending
> > TSTMSK __TI_flags(%r12),_TIF_NOTIFY_RESUME
> > @@ -857,6 +860,8 @@ ENTRY(io_int_handler)
> > #endif
> > TSTMSK __TI_flags(%r12),_TIF_SIGPENDING
> > jo .Lio_sigpending
> > + TSTMSK __TI_flags(%r12),_TIF_NOTIFY_SIGNAL
> > + jo .Lio_sigpending
> > TSTMSK __TI_flags(%r12),_TIF_NOTIFY_RESUME
> > jo .Lio_notify_resume
> > TSTMSK __TI_flags(%r12),_TIF_GUARDED_STORAGE
>
> (full quote so you can make sense of the patch below).
>
> Please merge the patch below into this one. With that:
>
> Acked-by: Heiko Carstens <[email protected]>
>
> diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> index a30d891e8045..31f16d903ef3 100644
> --- a/arch/s390/kernel/entry.S
> +++ b/arch/s390/kernel/entry.S
> @@ -464,9 +464,7 @@ ENTRY(system_call)
> #endif
> TSTMSK __PT_FLAGS(%r11),_PIF_SYSCALL_RESTART
> jo .Lsysc_syscall_restart
> - TSTMSK __TI_flags(%r12),_TIF_NOTIFY_SIGNAL
> - jo .Lsysc_sigpending
> - TSTMSK __TI_flags(%r12),_TIF_SIGPENDING
> + TSTMSK __TI_flags(%r12),(_TIF_SIGPENDING|_TIF_NOTIFY_SIGNAL)
> jo .Lsysc_sigpending
> TSTMSK __TI_flags(%r12),_TIF_NOTIFY_RESUME
> jo .Lsysc_notify_resume
> @@ -858,9 +856,7 @@ ENTRY(io_int_handler)
> TSTMSK __TI_flags(%r12),_TIF_PATCH_PENDING
> jo .Lio_patch_pending
> #endif
> - TSTMSK __TI_flags(%r12),_TIF_SIGPENDING
> - jo .Lio_sigpending
> - TSTMSK __TI_flags(%r12),_TIF_NOTIFY_SIGNAL
> + TSTMSK __TI_flags(%r12),(_TIF_SIGPENDING|_TIF_NOTIFY_SIGNAL)
> jo .Lio_sigpending
> TSTMSK __TI_flags(%r12),_TIF_NOTIFY_RESUME
> jo .Lio_notify_resum


2020-11-02 17:07:19

by Heiko Carstens

[permalink] [raw]
Subject: Re: [PATCH] s390: add support for TIF_NOTIFY_SIGNAL

On Mon, Nov 02, 2020 at 11:59:41AM -0500, Qian Cai wrote:
> On Sun, 2020-11-01 at 17:31 +0000, Heiko Carstens wrote:
> > On Thu, Oct 29, 2020 at 10:21:11AM -0600, Jens Axboe wrote:
> > > Wire up TIF_NOTIFY_SIGNAL handling for s390.
> > >
> > > Cc: [email protected]
> > > Signed-off-by: Jens Axboe <[email protected]>
>
> Even though I did confirm that today's linux-next contains this additional patch
> from Heiko below, a z10 guest is still unable to boot. Reverting the whole
> series (reverting only "s390: add support for TIF_NOTIFY_SIGNAL" introduced
> compiling errors) fixed the problem, i.e., git revert --no-edit
> af0dd809f3d3..7b074c15374c [1]
>
> .config: https://cailca.coding.net/public/linux/mm/git/files/master/s390.config

I'll take a look at it, but probably not today anymore.

2020-11-02 17:09:48

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH] s390: add support for TIF_NOTIFY_SIGNAL

On 11/2/20 9:59 AM, Qian Cai wrote:
> On Sun, 2020-11-01 at 17:31 +0000, Heiko Carstens wrote:
>> On Thu, Oct 29, 2020 at 10:21:11AM -0600, Jens Axboe wrote:
>>> Wire up TIF_NOTIFY_SIGNAL handling for s390.
>>>
>>> Cc: [email protected]
>>> Signed-off-by: Jens Axboe <[email protected]>
>
> Even though I did confirm that today's linux-next contains this additional patch
> from Heiko below, a z10 guest is still unable to boot. Reverting the whole
> series (reverting only "s390: add support for TIF_NOTIFY_SIGNAL" introduced
> compiling errors) fixed the problem, i.e., git revert --no-edit
> af0dd809f3d3..7b074c15374c [1]

That's odd, it should build fine without that patch. How did it fail for you?

Can you try and add this on top? Looks like I forgot the signal change for
s390, though that shouldn't really cause any issues.


diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
index 9e900a8977bd..a68c3796a1bf 100644
--- a/arch/s390/kernel/signal.c
+++ b/arch/s390/kernel/signal.c
@@ -472,7 +472,7 @@ void do_signal(struct pt_regs *regs)
current->thread.system_call =
test_pt_regs_flag(regs, PIF_SYSCALL) ? regs->int_code : 0;

- if (get_signal(&ksig)) {
+ if (test_thread_flag(TIF_NOTIFY_SIGNAL) && get_signal(&ksig)) {
/* Whee! Actually deliver the signal. */
if (current->thread.system_call) {
regs->int_code = current->thread.system_call;

--
Jens Axboe

2020-11-02 19:00:32

by Qian Cai

[permalink] [raw]
Subject: Re: [PATCH] s390: add support for TIF_NOTIFY_SIGNAL

On Mon, 2020-11-02 at 10:07 -0700, Jens Axboe wrote:
> On 11/2/20 9:59 AM, Qian Cai wrote:
> > On Sun, 2020-11-01 at 17:31 +0000, Heiko Carstens wrote:
> > > On Thu, Oct 29, 2020 at 10:21:11AM -0600, Jens Axboe wrote:
> > > > Wire up TIF_NOTIFY_SIGNAL handling for s390.
> > > >
> > > > Cc: [email protected]
> > > > Signed-off-by: Jens Axboe <[email protected]>
> >
> > Even though I did confirm that today's linux-next contains this additional
> > patch
> > from Heiko below, a z10 guest is still unable to boot. Reverting the whole
> > series (reverting only "s390: add support for TIF_NOTIFY_SIGNAL" introduced
> > compiling errors) fixed the problem, i.e., git revert --no-edit
> > af0dd809f3d3..7b074c15374c [1]
>
> That's odd, it should build fine without that patch. How did it fail for you?

In file included from ./arch/s390/include/asm/bug.h:5,
from ./include/linux/bug.h:5,
from ./include/linux/mmdebug.h:5,
from ./include/linux/percpu.h:5,
from ./include/linux/context_tracking_state.h:5,
from ./include/linux/hardirq.h:5,
from ./include/linux/kvm_host.h:7,
from arch/s390/kernel/asm-offsets.c:11:
./include/linux/sched/signal.h: In function ‘signal_pending’:
./include/linux/sched/signal.h:368:39: error: ‘TIF_NOTIFY_SIGNAL’ undeclared
(first use in this function); did you mean ‘TIF_NOTIFY_RESUME’?
if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
^~~~~~~~~~~~~~~~~
./include/linux/compiler.h:78:42: note: in definition of macro ‘unlikely’
# define unlikely(x) __builtin_expect(!!(x), 0)
^
./include/linux/sched/signal.h:368:39: note: each undeclared identifier is
reported only once for each function it appears in
if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
^~~~~~~~~~~~~~~~~
./include/linux/compiler.h:78:42: note: in definition of macro ‘unlikely’
# define unlikely(x) __builtin_expect(!!(x), 0)
^
make[1]: *** [scripts/Makefile.build:117: arch/s390/kernel/asm-offsets.s] Error
1
make: *** [Makefile:1198: prepare0] Error 2

>
> Can you try and add this on top? Looks like I forgot the signal change for
> s390, though that shouldn't really cause any issues.

It does not help with the boot issue at all.

>
>
> diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
> index 9e900a8977bd..a68c3796a1bf 100644
> --- a/arch/s390/kernel/signal.c
> +++ b/arch/s390/kernel/signal.c
> @@ -472,7 +472,7 @@ void do_signal(struct pt_regs *regs)
> current->thread.system_call =
> test_pt_regs_flag(regs, PIF_SYSCALL) ? regs->int_code : 0;
>
> - if (get_signal(&ksig)) {
> + if (test_thread_flag(TIF_NOTIFY_SIGNAL) && get_signal(&ksig)) {
> /* Whee! Actually deliver the signal. */
> if (current->thread.system_call) {
> regs->int_code = current->thread.system_call;
>

2020-11-02 19:52:38

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH] s390: add support for TIF_NOTIFY_SIGNAL

On 11/2/20 11:58 AM, Qian Cai wrote:
> On Mon, 2020-11-02 at 10:07 -0700, Jens Axboe wrote:
>> On 11/2/20 9:59 AM, Qian Cai wrote:
>>> On Sun, 2020-11-01 at 17:31 +0000, Heiko Carstens wrote:
>>>> On Thu, Oct 29, 2020 at 10:21:11AM -0600, Jens Axboe wrote:
>>>>> Wire up TIF_NOTIFY_SIGNAL handling for s390.
>>>>>
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Jens Axboe <[email protected]>
>>>
>>> Even though I did confirm that today's linux-next contains this additional
>>> patch
>>> from Heiko below, a z10 guest is still unable to boot. Reverting the whole
>>> series (reverting only "s390: add support for TIF_NOTIFY_SIGNAL" introduced
>>> compiling errors) fixed the problem, i.e., git revert --no-edit
>>> af0dd809f3d3..7b074c15374c [1]
>>
>> That's odd, it should build fine without that patch. How did it fail for you?
>
> In file included from ./arch/s390/include/asm/bug.h:5,
> from ./include/linux/bug.h:5,
> from ./include/linux/mmdebug.h:5,
> from ./include/linux/percpu.h:5,
> from ./include/linux/context_tracking_state.h:5,
> from ./include/linux/hardirq.h:5,
> from ./include/linux/kvm_host.h:7,
> from arch/s390/kernel/asm-offsets.c:11:
> ./include/linux/sched/signal.h: In function ‘signal_pending’:
> ./include/linux/sched/signal.h:368:39: error: ‘TIF_NOTIFY_SIGNAL’ undeclared
> (first use in this function); did you mean ‘TIF_NOTIFY_RESUME’?
> if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
> ^~~~~~~~~~~~~~~~~
> ./include/linux/compiler.h:78:42: note: in definition of macro ‘unlikely’
> # define unlikely(x) __builtin_expect(!!(x), 0)
> ^
> ./include/linux/sched/signal.h:368:39: note: each undeclared identifier is
> reported only once for each function it appears in
> if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
> ^~~~~~~~~~~~~~~~~
> ./include/linux/compiler.h:78:42: note: in definition of macro ‘unlikely’
> # define unlikely(x) __builtin_expect(!!(x), 0)
> ^
> make[1]: *** [scripts/Makefile.build:117: arch/s390/kernel/asm-offsets.s] Error
> 1
> make: *** [Makefile:1198: prepare0] Error 2

Ah, but that's because later patches assume that TIF_NOTIFY_SIGNAL is
always there once all archs have been converted. If you just want to back
out that patch, you'll need to just revert this one:

commit 82ef6998ed9d488e56bbfbcc2ec9adf62bf78f08
Author: Jens Axboe <[email protected]>
Date: Fri Oct 9 16:04:39 2020 -0600

kernel: remove checking for TIF_NOTIFY_SIGNAL

as well and I suspect it should build.

--
Jens Axboe

2020-11-02 21:17:42

by Qian Cai

[permalink] [raw]
Subject: Re: [PATCH] s390: add support for TIF_NOTIFY_SIGNAL

On Mon, 2020-11-02 at 12:50 -0700, Jens Axboe wrote:
> Ah, but that's because later patches assume that TIF_NOTIFY_SIGNAL is
> always there once all archs have been converted. If you just want to back
> out that patch, you'll need to just revert this one:
>
> commit 82ef6998ed9d488e56bbfbcc2ec9adf62bf78f08
> Author: Jens Axboe <[email protected]>
> Date: Fri Oct 9 16:04:39 2020 -0600
>
> kernel: remove checking for TIF_NOTIFY_SIGNAL
>
> as well and I suspect it should build.

No, at the minimal, I'll need to revert those to build successfully.

7b074c15374c io_uring: remove 'twa_signal_ok' deadlock work-around
eb48a0f216fa kernel: remove checking for TIF_NOTIFY_SIGNAL
c634e6b63a81 signal: kill JOBCTL_TASK_WORK
f8b667db31a3 io_uring: JOBCTL_TASK_WORK is no longer used by task_work
4c3d9c3b415a s390: add support for TIF_NOTIFY_SIGNAL

Then, it will fix the boot issue as well.