| Message ID | 6b89e805-c645-e738-794f-05ba6be68d60@kernel.dk |
|---|---|
| State | New |
| Headers | show |
| Series | arc: add support for TIF_NOTIFY_SIGNAL | expand |
On 10/29/20 9:09 AM, Jens Axboe wrote: > Wire up TIF_NOTIFY_SIGNAL handling for arc. > > Cc:linux-arm-kernel@lists.infradead.org Just to be clear, ARC and ARM seem to differ in 1 letter, they are in no way related :-) > Signed-off-by: Jens Axboe<axboe@kernel.dk> > --- > > 5.11 has support queued up for TIF_NOTIFY_SIGNAL, see this posting > for details: > > https://urldefense.com/v3/__https://lore.kernel.org/io-uring/20201026203230.386348-1-axboe@kernel.dk/__;!!A4F2R9G_pg!Oo4kwr6Yy8y5ByC8jIONjU28gLsqpyfzT10u5kfdsj2E-U2VIXQs7SvTmBRIkupZ$ TL'DR ignore this url rewrite. Corporate mailer fudging the urls and I can't seem to find this msg in my mailing group subscription in Thunderbird (although it shows up in web achieve) hence need to reply from office account rather than the newsgroup account. > 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. I'm fine either ways too, best to do whatever you are doing for other arches. Although this patch per se is broken, please see below. > > GET_CURR_THR_INFO_FLAGS r9 > - bbit0 r9, TIF_SIGPENDING, .Lchk_notify_resume > + and.f 0, r9, TIF_SIGPENDING|TIF_NOTIFY_SIGNAL, .Lchk_notify_resume This is not correct, AND is a simple ALU instruction with format: AND dest, src2, src1 With dest 0, it won't update any register. With .f suffix it would set CC flag based on ALU operation which can subsequent used for a predicated instruction such as BZ. So you need something like and.f 0, r9, TIF_SIGPENDING|TIF_NOTIFY_SIGNAL bz .Lchk_notify_resume > > ; Normal Trap/IRQ entry only saves Scratch (caller-saved) regs > ; in pt_reg since the "C" ABI (kernel code) will automatically > diff --git a/arch/arc/kernel/signal.c b/arch/arc/kernel/signal.c > index 2be55fb96d87..a78d8f745a67 100644 > --- a/arch/arc/kernel/signal.c > +++ b/arch/arc/kernel/signal.c > @@ -362,7 +362,7 @@ void do_signal(struct pt_regs *regs) > > restart_scall = in_syscall(regs) && syscall_restartable(regs); > > - if (get_signal(&ksig)) { > + if (test_thread_flag(TIF_SIGPENDING) && get_signal(&ksig)) { > if (restart_scall) { > arc_restart_syscall(&ksig.ka, regs); > syscall_wont_restart(regs); /* No more restarts */ I've not seen your entire patchset, but it seems we are now hitting do_signal() for either of TIF_{SIGPENDING|NOTIFY_SIGNAL} but then only handling signal for TIF_SIGPENDING, so why even bother to come here. Do you plan to add additional arch handling later ? Thx, -Vineet
On 10/30/20 12:47 PM, Vineet Gupta wrote: > On 10/29/20 9:09 AM, Jens Axboe wrote: >> Wire up TIF_NOTIFY_SIGNAL handling for arc. >> >> Cc:linux-arm-kernel@lists.infradead.org > > > Just to be clear, ARC and ARM seem to differ in 1 letter, they are in no > way related :-) Oops, fat fingered that one. Will update :-) >> 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. > > > I'm fine either ways too, best to do whatever you are doing for other > arches. Although this patch per se is broken, please see below. > > >> >> GET_CURR_THR_INFO_FLAGS r9 >> - bbit0 r9, TIF_SIGPENDING, .Lchk_notify_resume >> + and.f 0, r9, TIF_SIGPENDING|TIF_NOTIFY_SIGNAL, .Lchk_notify_resume > > This is not correct, AND is a simple ALU instruction with format: AND > dest, src2, src1 > With dest 0, it won't update any register. With .f suffix it would set > CC flag based on ALU operation which can subsequent used for a > predicated instruction such as BZ. > > So you need something like > > and.f 0, r9, TIF_SIGPENDING|TIF_NOTIFY_SIGNAL > bz .Lchk_notify_resume Ah thanks, I'll make that change. Hard for me to test a lot of these, so I really appreciate someone knowledgable taking a look at it. >> >> ; Normal Trap/IRQ entry only saves Scratch (caller-saved) regs >> ; in pt_reg since the "C" ABI (kernel code) will automatically >> diff --git a/arch/arc/kernel/signal.c b/arch/arc/kernel/signal.c >> index 2be55fb96d87..a78d8f745a67 100644 >> --- a/arch/arc/kernel/signal.c >> +++ b/arch/arc/kernel/signal.c >> @@ -362,7 +362,7 @@ void do_signal(struct pt_regs *regs) >> >> restart_scall = in_syscall(regs) && syscall_restartable(regs); >> >> - if (get_signal(&ksig)) { >> + if (test_thread_flag(TIF_SIGPENDING) && get_signal(&ksig)) { >> if (restart_scall) { >> arc_restart_syscall(&ksig.ka, regs); >> syscall_wont_restart(regs); /* No more restarts */ > > > I've not seen your entire patchset, but it seems we are now hitting > do_signal() for either of TIF_{SIGPENDING|NOTIFY_SIGNAL} but then only > handling signal for TIF_SIGPENDING, so why even bother to come here. Do > you plan to add additional arch handling later ? Nope, that's all that's needed for each arch. The behavior you describe is how it works. It makes it so that TIF_SIGPENDING will do the signal delivery and syscall restart as it always has done, but TIF_NOTIFY_SIGNAL will only do the syscall restart. The latter is the intent, hence TIF_NOTIFY_SIGNAL provides a way to interrupt a process and have it process task_work without going through signal delivery like task_work with TWA_SIGNAL does today. Updated version below: commit 3c6239647d95d03d1436bc826a004791c3f04617 Author: Jens Axboe <axboe@kernel.dk> Date: Mon Oct 12 07:15:37 2020 -0600 arc: add support for TIF_NOTIFY_SIGNAL Wire up TIF_NOTIFY_SIGNAL handling for arc. Cc: linux-snps-arc@lists.infradead.org Signed-off-by: Jens Axboe <axboe@kernel.dk> diff --git a/arch/arc/include/asm/thread_info.h b/arch/arc/include/asm/thread_info.h index f9eef0e8f0b7..c0942c24d401 100644 --- a/arch/arc/include/asm/thread_info.h +++ b/arch/arc/include/asm/thread_info.h @@ -79,6 +79,7 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void) #define TIF_SIGPENDING 2 /* signal pending */ #define TIF_NEED_RESCHED 3 /* rescheduling necessary */ #define TIF_SYSCALL_AUDIT 4 /* syscall auditing active */ +#define TIF_NOTIFY_SIGNAL 5 /* signal notifications exist */ #define TIF_SYSCALL_TRACE 15 /* syscall trace active */ /* true if poll_idle() is polling TIF_NEED_RESCHED */ @@ -89,11 +90,12 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void) #define _TIF_SIGPENDING (1<<TIF_SIGPENDING) #define _TIF_NEED_RESCHED (1<<TIF_NEED_RESCHED) #define _TIF_SYSCALL_AUDIT (1<<TIF_SYSCALL_AUDIT) +#define _TIF_NOTIFY_SIGNAL (1<<TIF_NOTIFY_SIGNAL) #define _TIF_MEMDIE (1<<TIF_MEMDIE) /* work to do on interrupt/exception return */ #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ - _TIF_NOTIFY_RESUME) + _TIF_NOTIFY_RESUME | _TIF_NOTIFY_SIGNAL) /* * _TIF_ALLWORK_MASK includes SYSCALL_TRACE, but we don't need it. diff --git a/arch/arc/kernel/entry.S b/arch/arc/kernel/entry.S index ea00c8a17f07..1f5308abf36d 100644 --- a/arch/arc/kernel/entry.S +++ b/arch/arc/kernel/entry.S @@ -307,7 +307,8 @@ resume_user_mode_begin: mov r0, sp ; pt_regs for arg to do_signal()/do_notify_resume() GET_CURR_THR_INFO_FLAGS r9 - bbit0 r9, TIF_SIGPENDING, .Lchk_notify_resume + and.f 0, r9, TIF_SIGPENDING|TIF_NOTIFY_SIGNAL + bz .Lchk_notify_resume ; Normal Trap/IRQ entry only saves Scratch (caller-saved) regs ; in pt_reg since the "C" ABI (kernel code) will automatically diff --git a/arch/arc/kernel/signal.c b/arch/arc/kernel/signal.c index 2be55fb96d87..a78d8f745a67 100644 --- a/arch/arc/kernel/signal.c +++ b/arch/arc/kernel/signal.c @@ -362,7 +362,7 @@ void do_signal(struct pt_regs *regs) restart_scall = in_syscall(regs) && syscall_restartable(regs); - if (get_signal(&ksig)) { + if (test_thread_flag(TIF_SIGPENDING) && get_signal(&ksig)) { if (restart_scall) { arc_restart_syscall(&ksig.ka, regs); syscall_wont_restart(regs); /* No more restarts */
On 10/30/20 11:53 AM, Jens Axboe wrote: > > Ah thanks, I'll make that change. Hard for me to test a lot of these, so > I really appreciate someone knowledgable taking a look at it. Sure, glad to help, thx to you for writing the arch patches in first place. It takes a bit of daring to venture in unchartered waters ;-) These days it is super easy to get your hands on a ARC cross toolchain. We don't have them shipping as regular distro packages just yet, but you can download a prebuilt @ https://github.com/foss-for-synopsys-dwc-arc-processors/toolchain/releases/download/arc-2020.09-release/arc_gnu_2020.09_prebuilt_glibc_le_archs_linux_install.tar.gz Or you can just build upstream gcc + binutils for ARC if you so prefer. >>> >>> ; Normal Trap/IRQ entry only saves Scratch (caller-saved) regs >>> ; in pt_reg since the "C" ABI (kernel code) will automatically >>> diff --git a/arch/arc/kernel/signal.c b/arch/arc/kernel/signal.c >>> index 2be55fb96d87..a78d8f745a67 100644 >>> --- a/arch/arc/kernel/signal.c >>> +++ b/arch/arc/kernel/signal.c >>> @@ -362,7 +362,7 @@ void do_signal(struct pt_regs *regs) >>> >>> restart_scall = in_syscall(regs) && syscall_restartable(regs); >>> >>> - if (get_signal(&ksig)) { >>> + if (test_thread_flag(TIF_SIGPENDING) && get_signal(&ksig)) { >>> if (restart_scall) { >>> arc_restart_syscall(&ksig.ka, regs); >>> syscall_wont_restart(regs); /* No more restarts */ >> >> I've not seen your entire patchset, but it seems we are now hitting >> do_signal() for either of TIF_{SIGPENDING|NOTIFY_SIGNAL} but then only >> handling signal for TIF_SIGPENDING, so why even bother to come here. Do >> you plan to add additional arch handling later ? > Nope, that's all that's needed for each arch. The behavior you describe > is how it works. It makes it so that TIF_SIGPENDING will do the signal > delivery and syscall restart as it always has done, but > TIF_NOTIFY_SIGNAL will only do the syscall restart. The latter is the > intent, hence TIF_NOTIFY_SIGNAL provides a way to interrupt a process > and have it process task_work without going through signal delivery like > task_work with TWA_SIGNAL does today. Nice, thx for explaining that. > > Updated version below: > > > commit 3c6239647d95d03d1436bc826a004791c3f04617 > Author: Jens Axboe <axboe@kernel.dk> > Date: Mon Oct 12 07:15:37 2020 -0600 > > arc: add support for TIF_NOTIFY_SIGNAL > > Wire up TIF_NOTIFY_SIGNAL handling for arc. > > Cc: linux-snps-arc@lists.infradead.org > Signed-off-by: Jens Axboe <axboe@kernel.dk> Acked-by: Vineet Gupta <vgupta@synopsys.com> > > diff --git a/arch/arc/include/asm/thread_info.h b/arch/arc/include/asm/thread_info.h > index f9eef0e8f0b7..c0942c24d401 100644 > --- a/arch/arc/include/asm/thread_info.h > +++ b/arch/arc/include/asm/thread_info.h > @@ -79,6 +79,7 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void) > #define TIF_SIGPENDING 2 /* signal pending */ > #define TIF_NEED_RESCHED 3 /* rescheduling necessary */ > #define TIF_SYSCALL_AUDIT 4 /* syscall auditing active */ > +#define TIF_NOTIFY_SIGNAL 5 /* signal notifications exist */ > #define TIF_SYSCALL_TRACE 15 /* syscall trace active */ > > /* true if poll_idle() is polling TIF_NEED_RESCHED */ > @@ -89,11 +90,12 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void) > #define _TIF_SIGPENDING (1<<TIF_SIGPENDING) > #define _TIF_NEED_RESCHED (1<<TIF_NEED_RESCHED) > #define _TIF_SYSCALL_AUDIT (1<<TIF_SYSCALL_AUDIT) > +#define _TIF_NOTIFY_SIGNAL (1<<TIF_NOTIFY_SIGNAL) > #define _TIF_MEMDIE (1<<TIF_MEMDIE) > > /* work to do on interrupt/exception return */ > #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ > - _TIF_NOTIFY_RESUME) > + _TIF_NOTIFY_RESUME | _TIF_NOTIFY_SIGNAL) > > /* > * _TIF_ALLWORK_MASK includes SYSCALL_TRACE, but we don't need it. > diff --git a/arch/arc/kernel/entry.S b/arch/arc/kernel/entry.S > index ea00c8a17f07..1f5308abf36d 100644 > --- a/arch/arc/kernel/entry.S > +++ b/arch/arc/kernel/entry.S > @@ -307,7 +307,8 @@ resume_user_mode_begin: > mov r0, sp ; pt_regs for arg to do_signal()/do_notify_resume() > > GET_CURR_THR_INFO_FLAGS r9 > - bbit0 r9, TIF_SIGPENDING, .Lchk_notify_resume > + and.f 0, r9, TIF_SIGPENDING|TIF_NOTIFY_SIGNAL > + bz .Lchk_notify_resume > > ; Normal Trap/IRQ entry only saves Scratch (caller-saved) regs > ; in pt_reg since the "C" ABI (kernel code) will automatically > diff --git a/arch/arc/kernel/signal.c b/arch/arc/kernel/signal.c > index 2be55fb96d87..a78d8f745a67 100644 > --- a/arch/arc/kernel/signal.c > +++ b/arch/arc/kernel/signal.c > @@ -362,7 +362,7 @@ void do_signal(struct pt_regs *regs) > > restart_scall = in_syscall(regs) && syscall_restartable(regs); > > - if (get_signal(&ksig)) { > + if (test_thread_flag(TIF_SIGPENDING) && get_signal(&ksig)) { > if (restart_scall) { > arc_restart_syscall(&ksig.ka, regs); > syscall_wont_restart(regs); /* No more restarts */ >
On 10/30/20 2:08 PM, Vineet Gupta wrote: > On 10/30/20 11:53 AM, Jens Axboe wrote: >> >> Ah thanks, I'll make that change. Hard for me to test a lot of these, so >> I really appreciate someone knowledgable taking a look at it. > > Sure, glad to help, thx to you for writing the arch patches in first > place. It takes a bit of daring to venture in unchartered waters ;-) Nice to do something different for a change! And another motivation is that I can't drop some old dead code before all archs support TIF_NOTIFY_SIGNAL. > These days it is super easy to get your hands on a ARC cross toolchain. > We don't have them shipping as regular distro packages just yet, but you > can download a prebuilt @ > https://github.com/foss-for-synopsys-dwc-arc-processors/toolchain/releases/download/arc-2020.09-release/arc_gnu_2020.09_prebuilt_glibc_le_archs_linux_install.tar.gz > > Or you can just build upstream gcc + binutils for ARC if you so prefer. Thanks, that's very helpful. Unfortunately my distro didn't have gcc+binutils packaged for arc, like it did for a lot of the other targets. >> commit 3c6239647d95d03d1436bc826a004791c3f04617 >> Author: Jens Axboe <axboe@kernel.dk> >> Date: Mon Oct 12 07:15:37 2020 -0600 >> >> arc: add support for TIF_NOTIFY_SIGNAL >> >> Wire up TIF_NOTIFY_SIGNAL handling for arc. >> >> Cc: linux-snps-arc@lists.infradead.org >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > Acked-by: Vineet Gupta <vgupta@synopsys.com> Added, thanks!
diff --git a/arch/arc/include/asm/thread_info.h b/arch/arc/include/asm/thread_info.h index f9eef0e8f0b7..c0942c24d401 100644 --- a/arch/arc/include/asm/thread_info.h +++ b/arch/arc/include/asm/thread_info.h @@ -79,6 +79,7 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void) #define TIF_SIGPENDING 2 /* signal pending */ #define TIF_NEED_RESCHED 3 /* rescheduling necessary */ #define TIF_SYSCALL_AUDIT 4 /* syscall auditing active */ +#define TIF_NOTIFY_SIGNAL 5 /* signal notifications exist */ #define TIF_SYSCALL_TRACE 15 /* syscall trace active */ /* true if poll_idle() is polling TIF_NEED_RESCHED */ @@ -89,11 +90,12 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void) #define _TIF_SIGPENDING (1<<TIF_SIGPENDING) #define _TIF_NEED_RESCHED (1<<TIF_NEED_RESCHED) #define _TIF_SYSCALL_AUDIT (1<<TIF_SYSCALL_AUDIT) +#define _TIF_NOTIFY_SIGNAL (1<<TIF_NOTIFY_SIGNAL) #define _TIF_MEMDIE (1<<TIF_MEMDIE) /* work to do on interrupt/exception return */ #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ - _TIF_NOTIFY_RESUME) + _TIF_NOTIFY_RESUME | _TIF_NOTIFY_SIGNAL) /* * _TIF_ALLWORK_MASK includes SYSCALL_TRACE, but we don't need it. diff --git a/arch/arc/kernel/entry.S b/arch/arc/kernel/entry.S index ea00c8a17f07..cb3a0dbdb795 100644 --- a/arch/arc/kernel/entry.S +++ b/arch/arc/kernel/entry.S @@ -307,7 +307,7 @@ resume_user_mode_begin: mov r0, sp ; pt_regs for arg to do_signal()/do_notify_resume() GET_CURR_THR_INFO_FLAGS r9 - bbit0 r9, TIF_SIGPENDING, .Lchk_notify_resume + and.f 0, r9, TIF_SIGPENDING|TIF_NOTIFY_SIGNAL, .Lchk_notify_resume ; Normal Trap/IRQ entry only saves Scratch (caller-saved) regs ; in pt_reg since the "C" ABI (kernel code) will automatically diff --git a/arch/arc/kernel/signal.c b/arch/arc/kernel/signal.c index 2be55fb96d87..a78d8f745a67 100644 --- a/arch/arc/kernel/signal.c +++ b/arch/arc/kernel/signal.c @@ -362,7 +362,7 @@ void do_signal(struct pt_regs *regs) restart_scall = in_syscall(regs) && syscall_restartable(regs); - if (get_signal(&ksig)) { + if (test_thread_flag(TIF_SIGPENDING) && get_signal(&ksig)) { if (restart_scall) { arc_restart_syscall(&ksig.ka, regs); syscall_wont_restart(regs); /* No more restarts */
Wire up TIF_NOTIFY_SIGNAL handling for arc. Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Jens Axboe <axboe@kernel.dk> --- 5.11 has support queued up for TIF_NOTIFY_SIGNAL, see this posting for details: https://lore.kernel.org/io-uring/20201026203230.386348-1-axboe@kernel.dk/ 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/arc/include/asm/thread_info.h | 4 +++- arch/arc/kernel/entry.S | 2 +- arch/arc/kernel/signal.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-)