[PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

Hi,

The goal is this patch series is to decouple TWA_SIGNAL based task_work
from real signals and signal delivery. The motivation is speeding up
TWA_SIGNAL based task_work, particularly for threaded setups where
->sighand is shared across threads.

Patch 1 is just a cleanup patchs, since I noticed that notify_resume
handling has some arch redundancy..

Patch 2 provides an abstraction around signal_pending(), in preparation
for allowing TIF_NOTIFY_SIGNAL to break scheduling loops.

Patch 3 just splits system call restart handling from the arch signal
delivery. Only the generic entry code is updated.

Patch 4 adds generic support for TIF_NOTIFY_SIGNAL, calling the
appropriate tracehook_notify_signal() if TIF_NOTIFY_SIGNAL is set.

Patch 5 just defines TIF_NOTIFY_SIGNAL for x86, since the generic code
is now ready for it.

Patch 6 finally allows task_work to use notify_signal to handle
TWA_SIGNAL based task_work.

Changes since v2:
- notify resume cleanup
- split patch series
- improve commit messages and comments
- kvm TIF_NOTIFY_SIGNAL handling

-- 
Jens Axboe

[PATCH 1/6] tracehook: clear TIF_NOTIFY_RESUME in tracehook_notify_resume()

[Jens Axboe]

All the callers currently do this, clean it up and move the clearing
into tracehook_notify_resume() instead.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 arch/alpha/kernel/signal.c      | 1 -
 arch/arc/kernel/signal.c        | 2 +-
 arch/arm/kernel/signal.c        | 1 -
 arch/arm64/kernel/signal.c      | 1 -
 arch/c6x/kernel/signal.c        | 4 +---
 arch/csky/kernel/signal.c       | 1 -
 arch/h8300/kernel/signal.c      | 4 +---
 arch/hexagon/kernel/process.c   | 1 -
 arch/ia64/kernel/process.c      | 2 +-
 arch/m68k/kernel/signal.c       | 2 +-
 arch/microblaze/kernel/signal.c | 2 +-
 arch/mips/kernel/signal.c       | 1 -
 arch/nds32/kernel/signal.c      | 4 +---
 arch/nios2/kernel/signal.c      | 2 +-
 arch/openrisc/kernel/signal.c   | 1 -
 arch/parisc/kernel/signal.c     | 4 +---
 arch/powerpc/kernel/signal.c    | 1 -
 arch/riscv/kernel/signal.c      | 4 +---
 arch/s390/kernel/signal.c       | 1 -
 arch/sh/kernel/signal_32.c      | 4 +---
 arch/sparc/kernel/signal_32.c   | 4 +---
 arch/sparc/kernel/signal_64.c   | 4 +---
 arch/um/kernel/process.c        | 2 +-
 arch/xtensa/kernel/signal.c     | 2 +-
 include/linux/tracehook.h       | 4 ++--
 kernel/entry/common.c           | 1 -
 kernel/entry/kvm.c              | 4 +---
 27 files changed, 18 insertions(+), 46 deletions(-)

diff --git a/arch/alpha/kernel/signal.c b/arch/alpha/kernel/signal.c
index 15bc9d1e79f4..3739efce1ec0 100644
--- a/arch/alpha/kernel/signal.c
+++ b/arch/alpha/kernel/signal.c
@@ -531,7 +531,6 @@ do_work_pending(struct pt_regs *regs, unsigned long thread_flags,
 				do_signal(regs, r0, r19);
 				r0 = 0;
 			} else {
-				clear_thread_flag(TIF_NOTIFY_RESUME);
 				tracehook_notify_resume(regs);
 			}
 		}
diff --git a/arch/arc/kernel/signal.c b/arch/arc/kernel/signal.c
index 8222f8c54690..2be55fb96d87 100644
--- a/arch/arc/kernel/signal.c
+++ b/arch/arc/kernel/signal.c
@@ -394,6 +394,6 @@ void do_notify_resume(struct pt_regs *regs)
 	 * ASM glue gaurantees that this is only called when returning to
 	 * user mode
 	 */
-	if (test_and_clear_thread_flag(TIF_NOTIFY_RESUME))
+	if (test_thread_flag(TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
 }
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index c1892f733f20..585edbfccf6d 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -669,7 +669,6 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 			} else if (thread_flags & _TIF_UPROBE) {
 				uprobe_notify_resume(regs);
 			} else {
-				clear_thread_flag(TIF_NOTIFY_RESUME);
 				tracehook_notify_resume(regs);
 				rseq_handle_notify_resume(NULL, regs);
 			}
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 3b4f31f35e45..4a6e1dc480c1 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -936,7 +936,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
 				do_signal(regs);
 
 			if (thread_flags & _TIF_NOTIFY_RESUME) {
-				clear_thread_flag(TIF_NOTIFY_RESUME);
 				tracehook_notify_resume(regs);
 				rseq_handle_notify_resume(NULL, regs);
 			}
diff --git a/arch/c6x/kernel/signal.c b/arch/c6x/kernel/signal.c
index d05c78eace1b..a3f15b9a79da 100644
--- a/arch/c6x/kernel/signal.c
+++ b/arch/c6x/kernel/signal.c
@@ -316,8 +316,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, u32 thread_info_flags,
 	if (thread_info_flags & (1 << TIF_SIGPENDING))
 		do_signal(regs, syscall);
 
-	if (thread_info_flags & (1 << TIF_NOTIFY_RESUME)) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (thread_info_flags & (1 << TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
-	}
 }
diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
index 970895df75ec..8b068cf37447 100644
--- a/arch/csky/kernel/signal.c
+++ b/arch/csky/kernel/signal.c
@@ -261,7 +261,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
 		do_signal(regs);
 
 	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
 		tracehook_notify_resume(regs);
 		rseq_handle_notify_resume(NULL, regs);
 	}
diff --git a/arch/h8300/kernel/signal.c b/arch/h8300/kernel/signal.c
index 69e68949787f..75d9b7e626b2 100644
--- a/arch/h8300/kernel/signal.c
+++ b/arch/h8300/kernel/signal.c
@@ -282,8 +282,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, u32 thread_info_flags)
 	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs);
 
-	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (thread_info_flags & _TIF_NOTIFY_RESUME)
 		tracehook_notify_resume(regs);
-	}
 }
diff --git a/arch/hexagon/kernel/process.c b/arch/hexagon/kernel/process.c
index dfd322c5ce83..5a0a95d93ddb 100644
--- a/arch/hexagon/kernel/process.c
+++ b/arch/hexagon/kernel/process.c
@@ -180,7 +180,6 @@ int do_work_pending(struct pt_regs *regs, u32 thread_info_flags)
 	}
 
 	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
 		tracehook_notify_resume(regs);
 		return 1;
 	}
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index f19cb97c0098..8f96cdda2f09 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -189,7 +189,7 @@ do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long in_syscall)
 		ia64_do_signal(scr, in_syscall);
 	}
 
-	if (test_and_clear_thread_flag(TIF_NOTIFY_RESUME)) {
+	if (test_thread_flag(TIF_NOTIFY_RESUME)) {
 		local_irq_enable();	/* force interrupt enable */
 		tracehook_notify_resume(&scr->pt);
 	}
diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
index a98fca977073..29e174a80bf6 100644
--- a/arch/m68k/kernel/signal.c
+++ b/arch/m68k/kernel/signal.c
@@ -1134,6 +1134,6 @@ void do_notify_resume(struct pt_regs *regs)
 	if (test_thread_flag(TIF_SIGPENDING))
 		do_signal(regs);
 
-	if (test_and_clear_thread_flag(TIF_NOTIFY_RESUME))
+	if (test_thread_flag(TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
 }
diff --git a/arch/microblaze/kernel/signal.c b/arch/microblaze/kernel/signal.c
index 4a96b59f0bee..f11a0ccccabc 100644
--- a/arch/microblaze/kernel/signal.c
+++ b/arch/microblaze/kernel/signal.c
@@ -316,6 +316,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, int in_syscall)
 	if (test_thread_flag(TIF_SIGPENDING))
 		do_signal(regs, in_syscall);
 
-	if (test_and_clear_thread_flag(TIF_NOTIFY_RESUME))
+	if (test_thread_flag(TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
 }
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index a0262729cd4c..77d40126b8a9 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -901,7 +901,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, void *unused,
 		do_signal(regs);
 
 	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
 		tracehook_notify_resume(regs);
 		rseq_handle_notify_resume(NULL, regs);
 	}
diff --git a/arch/nds32/kernel/signal.c b/arch/nds32/kernel/signal.c
index 36e25a410bb0..2acb94812af9 100644
--- a/arch/nds32/kernel/signal.c
+++ b/arch/nds32/kernel/signal.c
@@ -379,8 +379,6 @@ do_notify_resume(struct pt_regs *regs, unsigned int thread_flags)
 	if (thread_flags & _TIF_SIGPENDING)
 		do_signal(regs);
 
-	if (thread_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (thread_flags & _TIF_NOTIFY_RESUME)
 		tracehook_notify_resume(regs);
-	}
 }
diff --git a/arch/nios2/kernel/signal.c b/arch/nios2/kernel/signal.c
index d8a087cf2b42..cf2dca2ac7c3 100644
--- a/arch/nios2/kernel/signal.c
+++ b/arch/nios2/kernel/signal.c
@@ -317,7 +317,7 @@ asmlinkage int do_notify_resume(struct pt_regs *regs)
 			 */
 			return restart;
 		}
-	} else if (test_and_clear_thread_flag(TIF_NOTIFY_RESUME))
+	} else if (test_thread_flag(TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
 
 	return 0;
diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index c779364f0cd0..af66f968dd45 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -311,7 +311,6 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 				}
 				syscall = 0;
 			} else {
-				clear_thread_flag(TIF_NOTIFY_RESUME);
 				tracehook_notify_resume(regs);
 			}
 		}
diff --git a/arch/parisc/kernel/signal.c b/arch/parisc/kernel/signal.c
index 3c037fc96038..9f43eaeb0b0a 100644
--- a/arch/parisc/kernel/signal.c
+++ b/arch/parisc/kernel/signal.c
@@ -606,8 +606,6 @@ void do_notify_resume(struct pt_regs *regs, long in_syscall)
 	if (test_thread_flag(TIF_SIGPENDING))
 		do_signal(regs, in_syscall);
 
-	if (test_thread_flag(TIF_NOTIFY_RESUME)) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (test_thread_flag(TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
-	}
 }
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index d15a98c758b8..74a94a125f0d 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -327,7 +327,6 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 	}
 
 	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
 		tracehook_notify_resume(regs);
 		rseq_handle_notify_resume(NULL, regs);
 	}
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
index e996e08f1061..bc6841867b51 100644
--- a/arch/riscv/kernel/signal.c
+++ b/arch/riscv/kernel/signal.c
@@ -313,8 +313,6 @@ asmlinkage __visible void do_notify_resume(struct pt_regs *regs,
 	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs);
 
-	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (thread_info_flags & _TIF_NOTIFY_RESUME)
 		tracehook_notify_resume(regs);
-	}
 }
diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
index b295090e2ce6..9e900a8977bd 100644
--- a/arch/s390/kernel/signal.c
+++ b/arch/s390/kernel/signal.c
@@ -535,7 +535,6 @@ void do_signal(struct pt_regs *regs)
 
 void do_notify_resume(struct pt_regs *regs)
 {
-	clear_thread_flag(TIF_NOTIFY_RESUME);
 	tracehook_notify_resume(regs);
 	rseq_handle_notify_resume(NULL, regs);
 }
diff --git a/arch/sh/kernel/signal_32.c b/arch/sh/kernel/signal_32.c
index 4fe3f00137bc..1add47fd31f6 100644
--- a/arch/sh/kernel/signal_32.c
+++ b/arch/sh/kernel/signal_32.c
@@ -502,8 +502,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, unsigned int save_r0,
 	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs, save_r0);
 
-	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (thread_info_flags & _TIF_NOTIFY_RESUME)
 		tracehook_notify_resume(regs);
-	}
 }
diff --git a/arch/sparc/kernel/signal_32.c b/arch/sparc/kernel/signal_32.c
index d0e0025ee3ba..741d0701003a 100644
--- a/arch/sparc/kernel/signal_32.c
+++ b/arch/sparc/kernel/signal_32.c
@@ -523,10 +523,8 @@ void do_notify_resume(struct pt_regs *regs, unsigned long orig_i0,
 {
 	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs, orig_i0);
-	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (thread_info_flags & _TIF_NOTIFY_RESUME)
 		tracehook_notify_resume(regs);
-	}
 }
 
 asmlinkage int do_sys_sigstack(struct sigstack __user *ssptr,
diff --git a/arch/sparc/kernel/signal_64.c b/arch/sparc/kernel/signal_64.c
index 255264bcb46a..f7ef7edcd5c1 100644
--- a/arch/sparc/kernel/signal_64.c
+++ b/arch/sparc/kernel/signal_64.c
@@ -551,10 +551,8 @@ void do_notify_resume(struct pt_regs *regs, unsigned long orig_i0, unsigned long
 		uprobe_notify_resume(regs);
 	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs, orig_i0);
-	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
+	if (thread_info_flags & _TIF_NOTIFY_RESUME)
 		tracehook_notify_resume(regs);
-	}
 	user_enter();
 }
 
diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
index 26b5e243d3fc..3bed09538dd9 100644
--- a/arch/um/kernel/process.c
+++ b/arch/um/kernel/process.c
@@ -101,7 +101,7 @@ void interrupt_end(void)
 		schedule();
 	if (test_thread_flag(TIF_SIGPENDING))
 		do_signal(regs);
-	if (test_and_clear_thread_flag(TIF_NOTIFY_RESUME))
+	if (test_thread_flag(TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
 }
 
diff --git a/arch/xtensa/kernel/signal.c b/arch/xtensa/kernel/signal.c
index b3b17d6c50f0..1fb1047f905c 100644
--- a/arch/xtensa/kernel/signal.c
+++ b/arch/xtensa/kernel/signal.c
@@ -501,6 +501,6 @@ void do_notify_resume(struct pt_regs *regs)
 	if (test_thread_flag(TIF_SIGPENDING))
 		do_signal(regs);
 
-	if (test_and_clear_thread_flag(TIF_NOTIFY_RESUME))
+	if (test_thread_flag(TIF_NOTIFY_RESUME))
 		tracehook_notify_resume(regs);
 }
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 36fb3bbed6b2..b480e1a07ed8 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -178,9 +178,9 @@ static inline void set_notify_resume(struct task_struct *task)
  */
 static inline void tracehook_notify_resume(struct pt_regs *regs)
 {
+	clear_thread_flag(TIF_NOTIFY_RESUME);
 	/*
-	 * The caller just cleared TIF_NOTIFY_RESUME. This barrier
-	 * pairs with task_work_add()->set_notify_resume() after
+	 * This barrier pairs with task_work_add()->set_notify_resume() after
 	 * hlist_add_head(task->task_works);
 	 */
 	smp_mb__after_atomic();
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 6fdb6105e6d6..d20ab4ac7183 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -161,7 +161,6 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 			arch_do_signal(regs);
 
 		if (ti_work & _TIF_NOTIFY_RESUME) {
-			clear_thread_flag(TIF_NOTIFY_RESUME);
 			tracehook_notify_resume(regs);
 			rseq_handle_notify_resume(NULL, regs);
 		}
diff --git a/kernel/entry/kvm.c b/kernel/entry/kvm.c
index eb1a8a4c867c..b6678a5e3cf6 100644
--- a/kernel/entry/kvm.c
+++ b/kernel/entry/kvm.c
@@ -16,10 +16,8 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
 		if (ti_work & _TIF_NEED_RESCHED)
 			schedule();
 
-		if (ti_work & _TIF_NOTIFY_RESUME) {
-			clear_thread_flag(TIF_NOTIFY_RESUME);
+		if (ti_work & _TIF_NOTIFY_RESUME)
 			tracehook_notify_resume(NULL);
-		}
 
 		ret = arch_xfer_to_guest_mode_handle_work(vcpu, ti_work);
 		if (ret)
-- 
2.28.0

Re: [PATCH 1/6] tracehook: clear TIF_NOTIFY_RESUME in tracehook_notify_resume()

[Oleg Nesterov]

Jens, sorry for delay..

On 10/05, Jens Axboe wrote:
>
> All the callers currently do this, clean it up and move the clearing
> into tracehook_notify_resume() instead.

To me this looks like a good cleanup regardless.

Oleg.

Re: [PATCH 1/6] tracehook: clear TIF_NOTIFY_RESUME in tracehook_notify_resume()

[Jens Axboe]

On 10/8/20 6:37 AM, Oleg Nesterov wrote:
> Jens, sorry for delay..

No worries, thanks for looking at it!

> On 10/05, Jens Axboe wrote:
>>
>> All the callers currently do this, clean it up and move the clearing
>> into tracehook_notify_resume() instead.
> 
> To me this looks like a good cleanup regardless.

Thanks.

-- 
Jens Axboe

[PATCH 2/6] kernel: add task_sigpending() helper

[Jens Axboe]

This is in preparation for maintaining signal_pending() as the decider
of whether or not a schedule() loop should be broken, or continue
sleeping. This is different than the core signal use cases, where we
really want to know if an actual signal is pending or not.
task_sigpending() returns non-zero if TIF_SIGPENDING is set.

Only core kernel use cases should care about the distinction between
the two, make sure those use the task_sigpending() helper.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/sched/signal.h | 13 +++++++++----
 kernel/events/uprobes.c      |  2 +-
 kernel/ptrace.c              |  2 +-
 kernel/signal.c              | 12 ++++++------
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 1bad18a1d8ba..e6f34d8fbf4d 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -353,11 +353,16 @@ static inline int restart_syscall(void)
 	return -ERESTARTNOINTR;
 }
 
-static inline int signal_pending(struct task_struct *p)
+static inline int task_sigpending(struct task_struct *p)
 {
 	return unlikely(test_tsk_thread_flag(p,TIF_SIGPENDING));
 }
 
+static inline int signal_pending(struct task_struct *p)
+{
+	return task_sigpending(p);
+}
+
 static inline int __fatal_signal_pending(struct task_struct *p)
 {
 	return unlikely(sigismember(&p->pending.signal, SIGKILL));
@@ -365,14 +370,14 @@ static inline int __fatal_signal_pending(struct task_struct *p)
 
 static inline int fatal_signal_pending(struct task_struct *p)
 {
-	return signal_pending(p) && __fatal_signal_pending(p);
+	return task_sigpending(p) && __fatal_signal_pending(p);
 }
 
 static inline int signal_pending_state(long state, struct task_struct *p)
 {
 	if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
 		return 0;
-	if (!signal_pending(p))
+	if (!task_sigpending(p))
 		return 0;
 
 	return (state & TASK_INTERRUPTIBLE) || __fatal_signal_pending(p);
@@ -389,7 +394,7 @@ static inline bool fault_signal_pending(vm_fault_t fault_flags,
 {
 	return unlikely((fault_flags & VM_FAULT_RETRY) &&
 			(fatal_signal_pending(current) ||
-			 (user_mode(regs) && signal_pending(current))));
+			 (user_mode(regs) && task_sigpending(current))));
 }
 
 /*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 0e18aaf23a7b..8bb26a338e06 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1973,7 +1973,7 @@ bool uprobe_deny_signal(void)
 
 	WARN_ON_ONCE(utask->state != UTASK_SSTEP);
 
-	if (signal_pending(t)) {
+	if (task_sigpending(t)) {
 		spin_lock_irq(&t->sighand->siglock);
 		clear_tsk_thread_flag(t, TIF_SIGPENDING);
 		spin_unlock_irq(&t->sighand->siglock);
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 43d6179508d6..583b8da4c207 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -773,7 +773,7 @@ static int ptrace_peek_siginfo(struct task_struct *child,
 		data += sizeof(siginfo_t);
 		i++;
 
-		if (signal_pending(current))
+		if (task_sigpending(current))
 			break;
 
 		cond_resched();
diff --git a/kernel/signal.c b/kernel/signal.c
index a38b3edc6851..d57aafd9116c 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -983,7 +983,7 @@ static inline bool wants_signal(int sig, struct task_struct *p)
 	if (task_is_stopped_or_traced(p))
 		return false;
 
-	return task_curr(p) || !signal_pending(p);
+	return task_curr(p) || !task_sigpending(p);
 }
 
 static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
@@ -2822,7 +2822,7 @@ static void retarget_shared_pending(struct task_struct *tsk, sigset_t *which)
 		/* Remove the signals this thread can handle. */
 		sigandsets(&retarget, &retarget, &t->blocked);
 
-		if (!signal_pending(t))
+		if (!task_sigpending(t))
 			signal_wake_up(t, 0);
 
 		if (sigisemptyset(&retarget))
@@ -2856,7 +2856,7 @@ void exit_signals(struct task_struct *tsk)
 
 	cgroup_threadgroup_change_end(tsk);
 
-	if (!signal_pending(tsk))
+	if (!task_sigpending(tsk))
 		goto out;
 
 	unblocked = tsk->blocked;
@@ -2900,7 +2900,7 @@ long do_no_restart_syscall(struct restart_block *param)
 
 static void __set_task_blocked(struct task_struct *tsk, const sigset_t *newset)
 {
-	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+	if (task_sigpending(tsk) && !thread_group_empty(tsk)) {
 		sigset_t newblocked;
 		/* A set of now blocked but previously unblocked signals. */
 		sigandnsets(&newblocked, newset, &current->blocked);
@@ -4447,7 +4447,7 @@ SYSCALL_DEFINE0(pause)
 		__set_current_state(TASK_INTERRUPTIBLE);
 		schedule();
 	}
-	return -ERESTARTNOHAND;
+	return task_sigpending(current) ? -ERESTARTNOHAND : -ERESTARTSYS;
 }
 
 #endif
@@ -4462,7 +4462,7 @@ static int sigsuspend(sigset_t *set)
 		schedule();
 	}
 	set_restore_sigmask();
-	return -ERESTARTNOHAND;
+	return task_sigpending(current) ? -ERESTARTNOHAND : -ERESTARTSYS;
 }
 
 /**
-- 
2.28.0

Re: [PATCH 2/6] kernel: add task_sigpending() helper

[Oleg Nesterov]

On 10/05, Jens Axboe wrote:
>
>  static inline int signal_pending_state(long state, struct task_struct *p)
>  {
>  	if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
>  		return 0;
> -	if (!signal_pending(p))
> +	if (!task_sigpending(p))
>  		return 0;

This looks obviously wrong. Say, schedule() in TASK_INTERRUPTIBLE should
not block if TIF_NOTIFY_SIGNAL is set.

With this change set_notify_signal() will not force the task to return
from wait_event_interruptible, mutex_lock_interruptible, etc.

>  	return (state & TASK_INTERRUPTIBLE) || __fatal_signal_pending(p);
> @@ -389,7 +394,7 @@ static inline bool fault_signal_pending(vm_fault_t fault_flags,
>  {
>  	return unlikely((fault_flags & VM_FAULT_RETRY) &&
>  			(fatal_signal_pending(current) ||
> -			 (user_mode(regs) && signal_pending(current))));
> +			 (user_mode(regs) && task_sigpending(current))));

This looks unnecessary,

> @@ -773,7 +773,7 @@ static int ptrace_peek_siginfo(struct task_struct *child,
>  		data += sizeof(siginfo_t);
>  		i++;
>  
> -		if (signal_pending(current))
> +		if (task_sigpending(current))

This too.

IMO, this patch should do s/signal_pending/task_sigpending/ only if it is
strictly needed for correctness.

Oleg.

Re: [PATCH 2/6] kernel: add task_sigpending() helper

[Jens Axboe]

On 10/8/20 6:58 AM, Oleg Nesterov wrote:
> On 10/05, Jens Axboe wrote:
>>
>>  static inline int signal_pending_state(long state, struct task_struct *p)
>>  {
>>  	if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
>>  		return 0;
>> -	if (!signal_pending(p))
>> +	if (!task_sigpending(p))
>>  		return 0;
> 
> This looks obviously wrong. Say, schedule() in TASK_INTERRUPTIBLE should
> not block if TIF_NOTIFY_SIGNAL is set.
> 
> With this change set_notify_signal() will not force the task to return
> from wait_event_interruptible, mutex_lock_interruptible, etc.

True, not sure why I made the distinction there. I'll fix that one up.

> 
>>  	return (state & TASK_INTERRUPTIBLE) || __fatal_signal_pending(p);
>> @@ -389,7 +394,7 @@ static inline bool fault_signal_pending(vm_fault_t fault_flags,
>>  {
>>  	return unlikely((fault_flags & VM_FAULT_RETRY) &&
>>  			(fatal_signal_pending(current) ||
>> -			 (user_mode(regs) && signal_pending(current))));
>> +			 (user_mode(regs) && task_sigpending(current))));
> 
> This looks unnecessary,

Dropped

>> @@ -773,7 +773,7 @@ static int ptrace_peek_siginfo(struct task_struct *child,
>>  		data += sizeof(siginfo_t);
>>  		i++;
>>  
>> -		if (signal_pending(current))
>> +		if (task_sigpending(current))
> 
> This too.
> 
> IMO, this patch should do s/signal_pending/task_sigpending/ only if it is
> strictly needed for correctness.

Agree, I'll kill the ones you identified.

-- 
Jens Axboe

Re: [PATCH 2/6] kernel: add task_sigpending() helper

[Oleg Nesterov]

On 10/05, Jens Axboe wrote:
>
> @@ -4447,7 +4447,7 @@ SYSCALL_DEFINE0(pause)
>  		__set_current_state(TASK_INTERRUPTIBLE);
>  		schedule();
>  	}
> -	return -ERESTARTNOHAND;
> +	return task_sigpending(current) ? -ERESTARTNOHAND : -ERESTARTSYS;
>  }
>
>  #endif
> @@ -4462,7 +4462,7 @@ static int sigsuspend(sigset_t *set)
>  		schedule();
>  	}
>  	set_restore_sigmask();
> -	return -ERESTARTNOHAND;
> +	return task_sigpending(current) ? -ERESTARTNOHAND : -ERESTARTSYS;
>  }

Both changes are equally wrong. Why do you think sigsuspend() should ever
return -ERESTARTSYS ?

If get_signal() deques a signal, handle_signal() will restart this syscall
if ERESTARTSYS, this is wrong.

Oleg.

Re: [PATCH 2/6] kernel: add task_sigpending() helper

[Jens Axboe]

On 10/8/20 7:24 AM, Oleg Nesterov wrote:
> On 10/05, Jens Axboe wrote:
>>
>> @@ -4447,7 +4447,7 @@ SYSCALL_DEFINE0(pause)
>>  		__set_current_state(TASK_INTERRUPTIBLE);
>>  		schedule();
>>  	}
>> -	return -ERESTARTNOHAND;
>> +	return task_sigpending(current) ? -ERESTARTNOHAND : -ERESTARTSYS;
>>  }
>>
>>  #endif
>> @@ -4462,7 +4462,7 @@ static int sigsuspend(sigset_t *set)
>>  		schedule();
>>  	}
>>  	set_restore_sigmask();
>> -	return -ERESTARTNOHAND;
>> +	return task_sigpending(current) ? -ERESTARTNOHAND : -ERESTARTSYS;
>>  }
> 
> Both changes are equally wrong. Why do you think sigsuspend() should ever
> return -ERESTARTSYS ?
> 
> If get_signal() deques a signal, handle_signal() will restart this syscall
> if ERESTARTSYS, this is wrong.

The intent was that if we get woken up and signal_pending() is true, then
we want to restart it if we're just doing TIF_SIGNAL_NOTIFY. But I guess
it can't be 100% reliable, even if TIF_SIGPENDING isn't set at this point,
but it is by the time a signal is attempted dequeued.

I'll drop these too, thanks Oleg.

-- 
Jens Axboe

[PATCH 3/6] kernel: split syscall restart from signal handling

[Jens Axboe]

Move the restart syscall logic into a separate generic entry helper,
and handle that part separately from signal checking and delivery.

This is in preparation for being able to do syscall restarting
independently from handling signals.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 arch/x86/kernel/signal.c     | 32 ++++++++++++++++++--------------
 include/linux/entry-common.h | 14 ++++++++++++--
 kernel/entry/common.c        | 11 ++++++++---
 3 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index be0d7d4152ec..5dc1eeaf0866 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -799,21 +799,8 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 #endif
 }
 
-/*
- * Note that 'init' is a special process: it doesn't get signals it doesn't
- * want to handle. Thus you cannot kill init even with a SIGKILL even by
- * mistake.
- */
-void arch_do_signal(struct pt_regs *regs)
+void arch_restart_syscall(struct pt_regs *regs)
 {
-	struct ksignal ksig;
-
-	if (get_signal(&ksig)) {
-		/* Whee! Actually deliver the signal.  */
-		handle_signal(&ksig, regs);
-		return;
-	}
-
 	/* Did we come from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* Restart the system call - no handlers present */
@@ -831,12 +818,29 @@ void arch_do_signal(struct pt_regs *regs)
 			break;
 		}
 	}
+}
+
+/*
+ * Note that 'init' is a special process: it doesn't get signals it doesn't
+ * want to handle. Thus you cannot kill init even with a SIGKILL even by
+ * mistake.
+ */
+bool arch_do_signal(struct pt_regs *regs)
+{
+	struct ksignal ksig;
+
+	if (get_signal(&ksig)) {
+		/* Whee! Actually deliver the signal.  */
+		handle_signal(&ksig, regs);
+		return true;
+	}
 
 	/*
 	 * If there's no signal to deliver, we just put the saved sigmask
 	 * back.
 	 */
 	restore_saved_sigmask();
+	return false;
 }
 
 void signal_fault(struct pt_regs *regs, void __user *frame, char *where)
diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
index 159c7476b11b..ccfcc4769925 100644
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -262,9 +262,19 @@ static __always_inline void arch_exit_to_user_mode(void) { }
  * arch_do_signal -  Architecture specific signal delivery function
  * @regs:	Pointer to currents pt_regs
  *
- * Invoked from exit_to_user_mode_loop().
+ * Invoked from exit_to_user_mode_loop(). Returns true if a signal was
+ * handled.
  */
-void arch_do_signal(struct pt_regs *regs);
+bool arch_do_signal(struct pt_regs *regs);
+
+/**
+ * arch_restart_syscall -  Architecture specific syscall restarting
+ * @regs:	Pointer to currents pt_regs
+ *
+ * Invoked from exit_to_user_mode_loop(), if we need to restart the current
+ * system call.
+ */
+void arch_restart_syscall(struct pt_regs *regs);
 
 /**
  * arch_syscall_exit_tracehook - Wrapper around tracehook_report_syscall_exit()
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index d20ab4ac7183..0c0cc3cf3eba 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -135,11 +135,13 @@ static __always_inline void exit_to_user_mode(void)
 }
 
 /* Workaround to allow gradual conversion of architecture code */
-void __weak arch_do_signal(struct pt_regs *regs) { }
+bool __weak arch_do_signal(struct pt_regs *regs) { return true; }
 
 static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 					    unsigned long ti_work)
 {
+	bool restart_sys = ti_work & _TIF_SIGPENDING;
+
 	/*
 	 * Before returning to user space ensure that all pending work
 	 * items have been completed.
@@ -157,8 +159,8 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 		if (ti_work & _TIF_PATCH_PENDING)
 			klp_update_patch_state(current);
 
-		if (ti_work & _TIF_SIGPENDING)
-			arch_do_signal(regs);
+		if ((ti_work & _TIF_SIGPENDING) && arch_do_signal(regs))
+			restart_sys = false;
 
 		if (ti_work & _TIF_NOTIFY_RESUME) {
 			tracehook_notify_resume(regs);
@@ -177,6 +179,9 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 		ti_work = READ_ONCE(current_thread_info()->flags);
 	}
 
+	if (restart_sys)
+		arch_restart_syscall(regs);
+
 	/* Return the latest work state for arch_exit_to_user_mode() */
 	return ti_work;
 }
-- 
2.28.0

Re: [PATCH 3/6] kernel: split syscall restart from signal handling

[Oleg Nesterov]

On 10/05, Jens Axboe wrote:
>
> Move the restart syscall logic into a separate generic entry helper,
> and handle that part separately from signal checking and delivery.
>
> This is in preparation for being able to do syscall restarting
> independently from handling signals.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  arch/x86/kernel/signal.c     | 32 ++++++++++++++++++--------------
>  include/linux/entry-common.h | 14 ++++++++++++--
>  kernel/entry/common.c        | 11 ++++++++---
>  3 files changed, 38 insertions(+), 19 deletions(-)

Can't we avoid this patch and the and simplify the change in
exit_to_user_mode_loop() from the next patch? Can't the much more simple
patch below work?

Then later we can even change arch_do_signal() to accept the additional
argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
instead of test_thread_flag/task_sigpending.

Oleg.

--- x/arch/x86/kernel/signal.c
+++ x/arch/x86/kernel/signal.c
@@ -808,7 +808,10 @@ void arch_do_signal(struct pt_regs *regs
 {
 	struct ksignal ksig;
 
-	if (get_signal(&ksig)) {
+	if (test_thread_flag(TIF_NOTIFY_SIGNAL))
+		tracehook_notify_signal();
+
+	if (task_sigpending(current) && get_signal(&ksig)) {
 		/* Whee! Actually deliver the signal.  */
 		handle_signal(&ksig, regs);
 		return;
--- x/kernel/entry/common.c
+++ x/kernel/entry/common.c
@@ -155,7 +155,7 @@ static unsigned long exit_to_user_mode_l
 		if (ti_work & _TIF_PATCH_PENDING)
 			klp_update_patch_state(current);
 
-		if (ti_work & _TIF_SIGPENDING)
+		if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)
 			arch_do_signal(regs);
 
 		if (ti_work & _TIF_NOTIFY_RESUME) {

Re: [PATCH 3/6] kernel: split syscall restart from signal handling

[Jens Axboe]

On 10/8/20 8:21 AM, Oleg Nesterov wrote:
> On 10/05, Jens Axboe wrote:
>>
>> Move the restart syscall logic into a separate generic entry helper,
>> and handle that part separately from signal checking and delivery.
>>
>> This is in preparation for being able to do syscall restarting
>> independently from handling signals.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  arch/x86/kernel/signal.c     | 32 ++++++++++++++++++--------------
>>  include/linux/entry-common.h | 14 ++++++++++++--
>>  kernel/entry/common.c        | 11 ++++++++---
>>  3 files changed, 38 insertions(+), 19 deletions(-)
> 
> Can't we avoid this patch and the and simplify the change in
> exit_to_user_mode_loop() from the next patch? Can't the much more simple
> patch below work?
> 
> Then later we can even change arch_do_signal() to accept the additional
> argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
> instead of test_thread_flag/task_sigpending.

Yeah I guess that would be a bit simpler, maybe I'm too focused on
decoupling the two. But if we go this route, and avoid sighand->lock for
just having TIF_NOTIFY_SIGNAL set, then that should be functionally
equivalent as far as I'm concerned.

I'll make the reduction, I'd prefer to keep this as small/simple as
possible initially.

-- 
Jens Axboe

Re: [PATCH 3/6] kernel: split syscall restart from signal handling

[Jens Axboe]

On 10/8/20 8:31 AM, Jens Axboe wrote:
> On 10/8/20 8:21 AM, Oleg Nesterov wrote:
>> On 10/05, Jens Axboe wrote:
>>>
>>> Move the restart syscall logic into a separate generic entry helper,
>>> and handle that part separately from signal checking and delivery.
>>>
>>> This is in preparation for being able to do syscall restarting
>>> independently from handling signals.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>  arch/x86/kernel/signal.c     | 32 ++++++++++++++++++--------------
>>>  include/linux/entry-common.h | 14 ++++++++++++--
>>>  kernel/entry/common.c        | 11 ++++++++---
>>>  3 files changed, 38 insertions(+), 19 deletions(-)
>>
>> Can't we avoid this patch and the and simplify the change in
>> exit_to_user_mode_loop() from the next patch? Can't the much more simple
>> patch below work?
>>
>> Then later we can even change arch_do_signal() to accept the additional
>> argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
>> instead of test_thread_flag/task_sigpending.
> 
> Yeah I guess that would be a bit simpler, maybe I'm too focused on
> decoupling the two. But if we go this route, and avoid sighand->lock for
> just having TIF_NOTIFY_SIGNAL set, then that should be functionally
> equivalent as far as I'm concerned.
> 
> I'll make the reduction, I'd prefer to keep this as small/simple as
> possible initially.

FWIW, then we should also just integrate the x86 define change into
that patch, so we can drop patches 3 + 5 with this change.

-- 
Jens Axboe

Re: [PATCH 3/6] kernel: split syscall restart from signal handling

[Oleg Nesterov]

On 10/08, Jens Axboe wrote:
>
> On 10/8/20 8:21 AM, Oleg Nesterov wrote:
> > 
> > Can't we avoid this patch and the and simplify the change in
> > exit_to_user_mode_loop() from the next patch? Can't the much more simple
> > patch below work?
> > 
> > Then later we can even change arch_do_signal() to accept the additional
> > argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
> > instead of test_thread_flag/task_sigpending.
> 
> Yeah I guess that would be a bit simpler, maybe I'm too focused on
> decoupling the two. But if we go this route, and avoid sighand->lock for
> just having TIF_NOTIFY_SIGNAL set, then that should be functionally
> equivalent as far as I'm concerned.

Not sure I understand... I think that the change I propose is functionally
equivalent or I missed something.

> I'll make the reduction, I'd prefer to keep this as small/simple as
> possible initially.

Great, thanks.

Oleg.

Re: [PATCH 3/6] kernel: split syscall restart from signal handling

[Jens Axboe]

On 10/8/20 8:45 AM, Oleg Nesterov wrote:
> On 10/08, Jens Axboe wrote:
>>
>> On 10/8/20 8:21 AM, Oleg Nesterov wrote:
>>>
>>> Can't we avoid this patch and the and simplify the change in
>>> exit_to_user_mode_loop() from the next patch? Can't the much more simple
>>> patch below work?
>>>
>>> Then later we can even change arch_do_signal() to accept the additional
>>> argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
>>> instead of test_thread_flag/task_sigpending.
>>
>> Yeah I guess that would be a bit simpler, maybe I'm too focused on
>> decoupling the two. But if we go this route, and avoid sighand->lock for
>> just having TIF_NOTIFY_SIGNAL set, then that should be functionally
>> equivalent as far as I'm concerned.
> 
> Not sure I understand... I think that the change I propose is functionally
> equivalent or I missed something.

Sorry, maybe my phrasing wasn't good, I'm totally agreeing with you :-)
Was just noting that the task_sigpending() is key for not calling
get_signal(), to avoid hitting the sighand->lock again.

-- 
Jens Axboe

[PATCH 4/6] kernel: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

This adds TIF_NOTIFY_SIGNAL handling in the generic code, which if set,
will return true if signal_pending() is used in a wait loop. That causes
an exit of the loop so that notify_signal tracehooks can be run. If the
wait loop is currently inside a system call, the system call is restarted
once task_work has been processed.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/entry-common.h |  6 +++++-
 include/linux/entry-kvm.h    |  4 ++--
 include/linux/sched/signal.h | 19 +++++++++++++++++--
 include/linux/tracehook.h    | 27 +++++++++++++++++++++++++++
 kernel/entry/common.c        |  5 ++++-
 kernel/entry/kvm.c           |  3 +++
 6 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
index ccfcc4769925..0929385b9d8d 100644
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -37,6 +37,10 @@
 # define _TIF_UPROBE			(0)
 #endif
 
+#ifndef _TIF_NOTIFY_SIGNAL
+# define _TIF_NOTIFY_SIGNAL		(0)
+#endif
+
 /*
  * TIF flags handled in syscall_enter_from_usermode()
  */
@@ -69,7 +73,7 @@
 
 #define EXIT_TO_USER_MODE_WORK						\
 	(_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | _TIF_UPROBE |		\
-	 _TIF_NEED_RESCHED | _TIF_PATCH_PENDING |			\
+	 _TIF_NEED_RESCHED | _TIF_PATCH_PENDING | _TIF_NOTIFY_SIGNAL |	\
 	 ARCH_EXIT_TO_USER_MODE_WORK)
 
 /**
diff --git a/include/linux/entry-kvm.h b/include/linux/entry-kvm.h
index 0cef17afb41a..9b93f8584ff7 100644
--- a/include/linux/entry-kvm.h
+++ b/include/linux/entry-kvm.h
@@ -11,8 +11,8 @@
 # define ARCH_XFER_TO_GUEST_MODE_WORK	(0)
 #endif
 
-#define XFER_TO_GUEST_MODE_WORK					\
-	(_TIF_NEED_RESCHED | _TIF_SIGPENDING |			\
+#define XFER_TO_GUEST_MODE_WORK						\
+	(_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL |	\
 	 _TIF_NOTIFY_RESUME | ARCH_XFER_TO_GUEST_MODE_WORK)
 
 struct kvm_vcpu;
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index e6f34d8fbf4d..3117ff205a14 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -360,6 +360,15 @@ static inline int task_sigpending(struct task_struct *p)
 
 static inline int signal_pending(struct task_struct *p)
 {
+#ifdef TIF_NOTIFY_SIGNAL
+	/*
+	 * TIF_NOTIFY_SIGNAL isn't really a signal, but it requires the same
+	 * behavior in terms of ensuring that we break out of wait loops
+	 * so that notify signal callbacks can be processed.
+	 */
+	if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
+		return 1;
+#endif
 	return task_sigpending(p);
 }
 
@@ -506,10 +515,16 @@ extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize);
 
 static inline void restore_saved_sigmask_unless(bool interrupted)
 {
-	if (interrupted)
+	if (interrupted) {
+#ifdef TIF_NOTIFY_SIGNAL
+		WARN_ON(!test_thread_flag(TIF_SIGPENDING) &&
+			!test_thread_flag(TIF_NOTIFY_SIGNAL));
+#else
 		WARN_ON(!test_thread_flag(TIF_SIGPENDING));
-	else
+#endif
+	} else {
 		restore_saved_sigmask();
+	}
 }
 
 static inline sigset_t *sigmask_to_save(void)
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index b480e1a07ed8..bec952f51439 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -198,4 +198,31 @@ static inline void tracehook_notify_resume(struct pt_regs *regs)
 	blkcg_maybe_throttle_current();
 }
 
+/*
+ * called by exit_to_user_mode_loop() if ti_work & _TIF_NOTIFY_SIGNAL. This
+ * is currently used by TWA_SIGNAL based task_work, which requires breaking
+ * wait loops to ensure that task_work is noticed and run.
+ */
+static inline void tracehook_notify_signal(void)
+{
+#ifdef TIF_NOTIFY_SIGNAL
+	clear_thread_flag(TIF_NOTIFY_SIGNAL);
+	smp_mb__after_atomic();
+	if (current->task_works)
+		task_work_run();
+#endif
+}
+
+/*
+ * Called when we have work to process from exit_to_user_mode_loop()
+ */
+static inline void set_notify_signal(struct task_struct *task)
+{
+#ifdef TIF_NOTIFY_SIGNAL
+	if (!test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL) &&
+	    !wake_up_state(task, TASK_INTERRUPTIBLE))
+		kick_process(task);
+#endif
+}
+
 #endif	/* <linux/tracehook.h> */
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 0c0cc3cf3eba..6cf9f4fa0f6e 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -140,7 +140,7 @@ bool __weak arch_do_signal(struct pt_regs *regs) { return true; }
 static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 					    unsigned long ti_work)
 {
-	bool restart_sys = ti_work & _TIF_SIGPENDING;
+	bool restart_sys = ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL);
 
 	/*
 	 * Before returning to user space ensure that all pending work
@@ -159,6 +159,9 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 		if (ti_work & _TIF_PATCH_PENDING)
 			klp_update_patch_state(current);
 
+		if (ti_work & _TIF_NOTIFY_SIGNAL)
+			tracehook_notify_signal();
+
 		if ((ti_work & _TIF_SIGPENDING) && arch_do_signal(regs))
 			restart_sys = false;
 
diff --git a/kernel/entry/kvm.c b/kernel/entry/kvm.c
index b6678a5e3cf6..49972ee99aff 100644
--- a/kernel/entry/kvm.c
+++ b/kernel/entry/kvm.c
@@ -8,6 +8,9 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
 	do {
 		int ret;
 
+		if (ti_work & _TIF_NOTIFY_SIGNAL)
+			tracehook_notify_signal();
+
 		if (ti_work & _TIF_SIGPENDING) {
 			kvm_handle_signal_exit(vcpu);
 			return -EINTR;
-- 
2.28.0

Re: [PATCH 4/6] kernel: add support for TIF_NOTIFY_SIGNAL

[Oleg Nesterov]

On 10/05, Jens Axboe wrote:
>
>  static inline int signal_pending(struct task_struct *p)
>  {
> +#ifdef TIF_NOTIFY_SIGNAL
> +	/*
> +	 * TIF_NOTIFY_SIGNAL isn't really a signal, but it requires the same
> +	 * behavior in terms of ensuring that we break out of wait loops
> +	 * so that notify signal callbacks can be processed.
> +	 */
> +	if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
> +		return 1;
> +#endif
>  	return task_sigpending(p);
>  }

perhaps we can add test_tsk_thread_mask() later...

>  static inline void restore_saved_sigmask_unless(bool interrupted)
>  {
> -	if (interrupted)
> +	if (interrupted) {
> +#ifdef TIF_NOTIFY_SIGNAL
> +		WARN_ON(!test_thread_flag(TIF_SIGPENDING) &&
> +			!test_thread_flag(TIF_NOTIFY_SIGNAL));
> +#else
>  		WARN_ON(!test_thread_flag(TIF_SIGPENDING));
> -	else
> +#endif
> +	} else {
>  		restore_saved_sigmask();
> +	}

I'd suggest to simply do

	-	WARN_ON(!test_thread_flag(TIF_SIGPENDING));
	+	WARN_ON(!signal_pending(current);


> --- a/kernel/entry/kvm.c
> +++ b/kernel/entry/kvm.c
> @@ -8,6 +8,9 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
>  	do {
>  		int ret;
>  
> +		if (ti_work & _TIF_NOTIFY_SIGNAL)
> +			tracehook_notify_signal();

Can't really comment this change, but to me it would be more safe to
simply return -EINTR.

Or perhaps even better, treat _TIF_NOTIFY_SIGNAL and _TIF_SIGPENDING
equally:

	-	if (ti_work & _TIF_SIGPENDING) {
	+	if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) {
			kvm_handle_signal_exit(vcpu);
			return -EINTR;

Oleg.

Re: [PATCH 4/6] kernel: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/8/20 7:53 AM, Oleg Nesterov wrote:
> On 10/05, Jens Axboe wrote:
>>
>>  static inline int signal_pending(struct task_struct *p)
>>  {
>> +#ifdef TIF_NOTIFY_SIGNAL
>> +	/*
>> +	 * TIF_NOTIFY_SIGNAL isn't really a signal, but it requires the same
>> +	 * behavior in terms of ensuring that we break out of wait loops
>> +	 * so that notify signal callbacks can be processed.
>> +	 */
>> +	if (unlikely(test_tsk_thread_flag(p, TIF_NOTIFY_SIGNAL)))
>> +		return 1;
>> +#endif
>>  	return task_sigpending(p);
>>  }
> 
> perhaps we can add test_tsk_thread_mask() later...

Yeah would be nice, and I bet there are a lot of cases in the kernel
that test multiple bits like that.

>>  static inline void restore_saved_sigmask_unless(bool interrupted)
>>  {
>> -	if (interrupted)
>> +	if (interrupted) {
>> +#ifdef TIF_NOTIFY_SIGNAL
>> +		WARN_ON(!test_thread_flag(TIF_SIGPENDING) &&
>> +			!test_thread_flag(TIF_NOTIFY_SIGNAL));
>> +#else
>>  		WARN_ON(!test_thread_flag(TIF_SIGPENDING));
>> -	else
>> +#endif
>> +	} else {
>>  		restore_saved_sigmask();
>> +	}
> 
> I'd suggest to simply do
> 
> 	-	WARN_ON(!test_thread_flag(TIF_SIGPENDING));
> 	+	WARN_ON(!signal_pending(current);

Ah yes, that's much better. I'll make the edit.

>> --- a/kernel/entry/kvm.c
>> +++ b/kernel/entry/kvm.c
>> @@ -8,6 +8,9 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
>>  	do {
>>  		int ret;
>>  
>> +		if (ti_work & _TIF_NOTIFY_SIGNAL)
>> +			tracehook_notify_signal();
> 
> Can't really comment this change, but to me it would be more safe to
> simply return -EINTR.
> 
> Or perhaps even better, treat _TIF_NOTIFY_SIGNAL and _TIF_SIGPENDING
> equally:
> 
> 	-	if (ti_work & _TIF_SIGPENDING) {
> 	+	if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) {
> 			kvm_handle_signal_exit(vcpu);
> 			return -EINTR;

Not sure I follow your logic here. Why treat it any different than
NOTIFY_RESUME from this perspective?

-- 
Jens Axboe

Re: [PATCH 4/6] kernel: add support for TIF_NOTIFY_SIGNAL

[Oleg Nesterov]

On 10/08, Jens Axboe wrote:
>
> On 10/8/20 7:53 AM, Oleg Nesterov wrote:
> >> --- a/kernel/entry/kvm.c
> >> +++ b/kernel/entry/kvm.c
> >> @@ -8,6 +8,9 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
> >>  	do {
> >>  		int ret;
> >>
> >> +		if (ti_work & _TIF_NOTIFY_SIGNAL)
> >> +			tracehook_notify_signal();
> >
> > Can't really comment this change, but to me it would be more safe to
> > simply return -EINTR.
> >
> > Or perhaps even better, treat _TIF_NOTIFY_SIGNAL and _TIF_SIGPENDING
> > equally:
> >
> > 	-	if (ti_work & _TIF_SIGPENDING) {
> > 	+	if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) {
> > 			kvm_handle_signal_exit(vcpu);
> > 			return -EINTR;
>
> Not sure I follow your logic here. Why treat it any different than
> NOTIFY_RESUME from this perspective?

Ah, good point, I din't notice that xfer_to_guest_mode_work() handles
TIF_NOTIFY_RESUME.

Thanks, then I think this change is fine.

Oleg.

[PATCH 5/6] x86: define _TIF_NOTIFY_SIGNAL

[Jens Axboe]

The entry code is now ready for it, define _TIF_NOTIFY_SIGNAL for
x86 to enable processing of signal based notifications.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 arch/x86/include/asm/thread_info.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 267701ae3d86..86ade67f21b7 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -93,6 +93,7 @@ struct thread_info {
 #define TIF_NOTSC		16	/* TSC is not accessible in userland */
 #define TIF_IA32		17	/* IA32 compatibility process */
 #define TIF_SLD			18	/* Restore split lock detection on context switch */
+#define TIF_NOTIFY_SIGNAL	19	/* signal notifications exist */
 #define TIF_MEMDIE		20	/* is terminating due to OOM killer */
 #define TIF_POLLING_NRFLAG	21	/* idle is polling for TIF_NEED_RESCHED */
 #define TIF_IO_BITMAP		22	/* uses I/O bitmap */
@@ -123,6 +124,7 @@ struct thread_info {
 #define _TIF_NOTSC		(1 << TIF_NOTSC)
 #define _TIF_IA32		(1 << TIF_IA32)
 #define _TIF_SLD		(1 << TIF_SLD)
+#define _TIF_NOTIFY_SIGNAL	(1 << TIF_NOTIFY_SIGNAL)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_IO_BITMAP		(1 << TIF_IO_BITMAP)
 #define _TIF_FORCED_TF		(1 << TIF_FORCED_TF)
-- 
2.28.0

[PATCH 6/6] task_work: use TIF_NOTIFY_SIGNAL if available

[Jens Axboe]

If the arch supports TIF_NOTIFY_SIGNAL, then use that for TWA_SIGNAL as
it's more efficient than using the signal delivery method. This is
especially true on threaded applications, where ->sighand is shared
across threads, but it's also lighter weight on non-shared cases.

io_uring is a heavy consumer of TWA_SIGNAL based task_work. On my test
box, even just using 16 threads shows a nice improvement running an
io_uring based echo server.

stock kernel:
0.01% <= 0.1 milliseconds
95.86% <= 0.2 milliseconds
98.27% <= 0.3 milliseconds
99.71% <= 0.4 milliseconds
100.00% <= 0.5 milliseconds
100.00% <= 0.6 milliseconds
100.00% <= 0.7 milliseconds
100.00% <= 0.8 milliseconds
100.00% <= 0.9 milliseconds
100.00% <= 1.0 milliseconds
100.00% <= 1.1 milliseconds
100.00% <= 2 milliseconds
100.00% <= 3 milliseconds
100.00% <= 3 milliseconds
1378930.00 requests per second
~1600% CPU

1.38M requests/second, and all 16 CPUs are maxed out.

patched kernel:
0.01% <= 0.1 milliseconds
98.24% <= 0.2 milliseconds
99.47% <= 0.3 milliseconds
99.99% <= 0.4 milliseconds
100.00% <= 0.5 milliseconds
100.00% <= 0.6 milliseconds
100.00% <= 0.7 milliseconds
100.00% <= 0.8 milliseconds
100.00% <= 0.9 milliseconds
100.00% <= 1.2 milliseconds
1666111.38 requests per second
~1450% CPU

1.67M requests/second, and we're no longer just hammering on the sighand
lock. The original reporter states:

"For 5.7.15 my benchmark achieves 1.6M qps and system cpu is at ~80%.
 for 5.7.16 or later it achieves only 1M qps and the system cpu is is
 at ~100%"

with the only difference there being that TWA_SIGNAL is used
unconditionally in 5.7.16, since we need it to be able to solve an
inability to run task_work if the application is waiting in the kernel
already on an event that needs task_work run to be satisfied. Also
see commit 0ba9c9edcd15.

Reported-by: Roman Gershman <romger@amazon.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 kernel/task_work.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/kernel/task_work.c b/kernel/task_work.c
index 613b2d634af8..95604e57af46 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -5,6 +5,34 @@
 
 static struct callback_head work_exited; /* all we need is ->next == NULL */
 
+/*
+ * TWA_SIGNAL signaling - use TIF_NOTIFY_SIGNAL, if available, as it's faster
+ * than TIF_SIGPENDING as there's no dependency on ->sighand. The latter is
+ * shared for threads, and can cause contention on sighand->lock. Even for
+ * the non-threaded case TIF_NOTIFY_SIGNAL is more efficient, as no locking
+ * or IRQ disabling is involved for notification (or running) purposes.
+ */
+static void task_work_notify_signal(struct task_struct *task)
+{
+#ifdef TIF_NOTIFY_SIGNAL
+	set_notify_signal(task);
+#else
+	unsigned long flags;
+
+	/*
+	 * Only grab the sighand lock if we don't already have some
+	 * task_work pending. This pairs with the smp_store_mb()
+	 * in get_signal(), see comment there.
+	 */
+	if (!(READ_ONCE(task->jobctl) & JOBCTL_TASK_WORK) &&
+	    lock_task_sighand(task, &flags)) {
+		task->jobctl |= JOBCTL_TASK_WORK;
+		signal_wake_up(task, 0);
+		unlock_task_sighand(task, &flags);
+	}
+#endif
+}
+
 /**
  * task_work_add - ask the @task to execute @work->func()
  * @task: the task which should run the callback
@@ -28,7 +56,6 @@ int
 task_work_add(struct task_struct *task, struct callback_head *work, int notify)
 {
 	struct callback_head *head;
-	unsigned long flags;
 
 	do {
 		head = READ_ONCE(task->task_works);
@@ -42,17 +69,7 @@ task_work_add(struct task_struct *task, struct callback_head *work, int notify)
 		set_notify_resume(task);
 		break;
 	case TWA_SIGNAL:
-		/*
-		 * Only grab the sighand lock if we don't already have some
-		 * task_work pending. This pairs with the smp_store_mb()
-		 * in get_signal(), see comment there.
-		 */
-		if (!(READ_ONCE(task->jobctl) & JOBCTL_TASK_WORK) &&
-		    lock_task_sighand(task, &flags)) {
-			task->jobctl |= JOBCTL_TASK_WORK;
-			signal_wake_up(task, 0);
-			unlock_task_sighand(task, &flags);
-		}
+		task_work_notify_signal(task);
 		break;
 	}
 
-- 
2.28.0

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Oleg Nesterov]

On 10/05, Jens Axboe wrote:
>
> Hi,
>
> The goal is this patch series is to decouple TWA_SIGNAL based task_work
> from real signals and signal delivery.

I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
fake_signal_wake_up(), and remove freezing() from recalc_sigpending().

Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
set_notify_signal() rather than signal_wake_up().

Oleg.

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/8/20 8:56 AM, Oleg Nesterov wrote:
> On 10/05, Jens Axboe wrote:
>>
>> Hi,
>>
>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
>> from real signals and signal delivery.
> 
> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
> 
> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
> set_notify_signal() rather than signal_wake_up().

Totally agree, which is why I liked your suggestion of turning it into a
tracehook.

I've rebased and collapsed the series with the changes, initial tests
look good here. I'll run it through some more testing and send out a v4.
I really like that it's down to 3 core patches now, instead of 5, and
the last one is just wiring up task_work. The changes you suggested also
means it's a lot easier to wire up new archs, so we could potentially
have full support for TIF_NOTIFY_SIGNAL very quickly and can drop the
JOBCTL etc parts.

I'll work on that next, if we have agreement that v4 is sound. Thanks a
lot for your reviews, Oleg! It might've started out a bit nasty on the
RFC front, but with the current direction, we'll end up deleting a lot
of extra code on top.

-- 
Jens Axboe

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Miroslav Benes]

On Thu, 8 Oct 2020, Oleg Nesterov wrote:

> On 10/05, Jens Axboe wrote:
> >
> > Hi,
> >
> > The goal is this patch series is to decouple TWA_SIGNAL based task_work
> > from real signals and signal delivery.
> 
> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
> 
> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
> set_notify_signal() rather than signal_wake_up().

Yes, that was my impression from the patch set too, when I accidentally 
noticed it.

Jens, could you CC our live patching ML when you submit v4, please? It 
would be a nice cleanup.

Thanks
Miroslav

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/9/20 2:01 AM, Miroslav Benes wrote:
> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
> 
>> On 10/05, Jens Axboe wrote:
>>>
>>> Hi,
>>>
>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
>>> from real signals and signal delivery.
>>
>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
>>
>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
>> set_notify_signal() rather than signal_wake_up().
> 
> Yes, that was my impression from the patch set too, when I accidentally 
> noticed it.
> 
> Jens, could you CC our live patching ML when you submit v4, please? It 
> would be a nice cleanup.

Definitely, though it'd be v5 at this point. But we really need to get
all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
a whole slew of cleanups that'll fall out naturally:

- Removal of JOBCTL_TASK_WORK
- Removal of special path for TWA_SIGNAL in task_work
- TIF_PATCH_PENDING can be converted and then removed
- try_to_freeze() cleanup that Oleg mentioned

And probably more I'm not thinking of right now :-)

-- 
Jens Axboe

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/9/20 9:21 AM, Jens Axboe wrote:
> On 10/9/20 2:01 AM, Miroslav Benes wrote:
>> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
>>
>>> On 10/05, Jens Axboe wrote:
>>>>
>>>> Hi,
>>>>
>>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
>>>> from real signals and signal delivery.
>>>
>>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
>>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
>>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
>>>
>>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
>>> set_notify_signal() rather than signal_wake_up().
>>
>> Yes, that was my impression from the patch set too, when I accidentally 
>> noticed it.
>>
>> Jens, could you CC our live patching ML when you submit v4, please? It 
>> would be a nice cleanup.
> 
> Definitely, though it'd be v5 at this point. But we really need to get
> all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
> a whole slew of cleanups that'll fall out naturally:
> 
> - Removal of JOBCTL_TASK_WORK
> - Removal of special path for TWA_SIGNAL in task_work
> - TIF_PATCH_PENDING can be converted and then removed
> - try_to_freeze() cleanup that Oleg mentioned
> 
> And probably more I'm not thinking of right now :-)

Here's the current series, I took a stab at converting all archs to
support TIF_NOTIFY_SIGNAL so we have a base to build on top of. Most
of them were straight forward, but I need someone to fixup powerpc,
verify arm and s390.

But it's a decent start I think, and means that we can drop various
bits as is done at the end of the series. I could swap things around
a bit and avoid having the intermediate step, but I envision that
getting this in all archs will take a bit longer than just signing off
on the generic/x86 bits. So probably best to keep the series as it is
for now, and work on getting the arch bits verified/fixed/tested.

https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work

-- 
Jens Axboe

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Miroslav Benes]

On Sat, 10 Oct 2020, Jens Axboe wrote:

> On 10/9/20 9:21 AM, Jens Axboe wrote:
> > On 10/9/20 2:01 AM, Miroslav Benes wrote:
> >> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
> >>
> >>> On 10/05, Jens Axboe wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
> >>>> from real signals and signal delivery.
> >>>
> >>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
> >>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
> >>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
> >>>
> >>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
> >>> set_notify_signal() rather than signal_wake_up().
> >>
> >> Yes, that was my impression from the patch set too, when I accidentally 
> >> noticed it.
> >>
> >> Jens, could you CC our live patching ML when you submit v4, please? It 
> >> would be a nice cleanup.
> > 
> > Definitely, though it'd be v5 at this point. But we really need to get
> > all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
> > a whole slew of cleanups that'll fall out naturally:
> > 
> > - Removal of JOBCTL_TASK_WORK
> > - Removal of special path for TWA_SIGNAL in task_work
> > - TIF_PATCH_PENDING can be converted and then removed
> > - try_to_freeze() cleanup that Oleg mentioned
> > 
> > And probably more I'm not thinking of right now :-)
> 
> Here's the current series, I took a stab at converting all archs to
> support TIF_NOTIFY_SIGNAL so we have a base to build on top of. Most
> of them were straight forward, but I need someone to fixup powerpc,
> verify arm and s390.
> 
> But it's a decent start I think, and means that we can drop various
> bits as is done at the end of the series. I could swap things around
> a bit and avoid having the intermediate step, but I envision that
> getting this in all archs will take a bit longer than just signing off
> on the generic/x86 bits. So probably best to keep the series as it is
> for now, and work on getting the arch bits verified/fixed/tested.
> 
> https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work

Thanks, Jens.

Crude diff for live patching on top of the series is below. Tested only on 
x86_64, but it passes the tests without an issue.

Miroslav

---
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
 
 #include <linux/cpu.h>
 #include <linux/stacktrace.h>
+#include <linux/tracehook.h>
 #include "core.h"
 #include "patch.h"
 #include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
                         * Send fake signal to all non-kthread tasks which are
                         * still not migrated.
                         */
-                       spin_lock_irq(&task->sighand->siglock);
-                       signal_wake_up(task, 0);
-                       spin_unlock_irq(&task->sighand->siglock);
+                       set_notify_signal(task);
                }
        }
        read_unlock(&tasklist_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index a15c584a0455..b7cf4eda8611 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -181,8 +181,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
 
 void recalc_sigpending(void)
 {
-       if (!recalc_sigpending_tsk(current) && !freezing(current) &&
-           !klp_patch_pending(current))
+       if (!recalc_sigpending_tsk(current) && !freezing(current))
                clear_thread_flag(TIF_SIGPENDING);
 
 }

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/12/20 11:27 AM, Miroslav Benes wrote:
> On Sat, 10 Oct 2020, Jens Axboe wrote:
> 
>> On 10/9/20 9:21 AM, Jens Axboe wrote:
>>> On 10/9/20 2:01 AM, Miroslav Benes wrote:
>>>> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
>>>>
>>>>> On 10/05, Jens Axboe wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
>>>>>> from real signals and signal delivery.
>>>>>
>>>>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
>>>>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
>>>>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
>>>>>
>>>>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
>>>>> set_notify_signal() rather than signal_wake_up().
>>>>
>>>> Yes, that was my impression from the patch set too, when I accidentally 
>>>> noticed it.
>>>>
>>>> Jens, could you CC our live patching ML when you submit v4, please? It 
>>>> would be a nice cleanup.
>>>
>>> Definitely, though it'd be v5 at this point. But we really need to get
>>> all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
>>> a whole slew of cleanups that'll fall out naturally:
>>>
>>> - Removal of JOBCTL_TASK_WORK
>>> - Removal of special path for TWA_SIGNAL in task_work
>>> - TIF_PATCH_PENDING can be converted and then removed
>>> - try_to_freeze() cleanup that Oleg mentioned
>>>
>>> And probably more I'm not thinking of right now :-)
>>
>> Here's the current series, I took a stab at converting all archs to
>> support TIF_NOTIFY_SIGNAL so we have a base to build on top of. Most
>> of them were straight forward, but I need someone to fixup powerpc,
>> verify arm and s390.
>>
>> But it's a decent start I think, and means that we can drop various
>> bits as is done at the end of the series. I could swap things around
>> a bit and avoid having the intermediate step, but I envision that
>> getting this in all archs will take a bit longer than just signing off
>> on the generic/x86 bits. So probably best to keep the series as it is
>> for now, and work on getting the arch bits verified/fixed/tested.
>>
>> https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work
> 
> Thanks, Jens.
> 
> Crude diff for live patching on top of the series is below. Tested only on 
> x86_64, but it passes the tests without an issue.

Nice, thanks!

I'm continuing to hone the series, what's really missing so far is arch
review. Most conversions are straight forward, some I need folks to
definitely take a look at (arm, s390). powerpc is also a bit hair right
now, but I'm told that 5.10 will kill a TIF flag there, so that'll make
it trivial once I rebase on that.

Did a few more cleanups on top, series is in the same spot. I'll repost
once the merge window settles down.

-- 
Jens Axboe

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Thomas Gleixner]

Jens,

On Tue, Oct 13 2020 at 13:39, Jens Axboe wrote:
> On 10/12/20 11:27 AM, Miroslav Benes wrote:
> I'm continuing to hone the series, what's really missing so far is arch
> review. Most conversions are straight forward, some I need folks to
> definitely take a look at (arm, s390). powerpc is also a bit hair right
> now, but I'm told that 5.10 will kill a TIF flag there, so that'll make
> it trivial once I rebase on that.

can you pretty please not add that to anything which is not going
through kernel/entry/ ?

The amount of duplicated and differently buggy, inconsistent and
incomplete code in syscall and exception handling is just annoying.

It's perfectly fine if we keep that #ifdeffery around for a while and
encourage arch folks to move over to the generic infrastructure instead
of proliferating the status quo by adding this to their existing pile.

The #ifdef guarding this in set_notify_signal() and other core code
places wants to be:

    #if defined(CONFIG_GENERIC_ENTRY) && defined(TIF_NOTIFY_SIGNAL)

Thanks,

        tglx

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/13/20 5:34 PM, Thomas Gleixner wrote:
> Jens,
> 
> On Tue, Oct 13 2020 at 13:39, Jens Axboe wrote:
>> On 10/12/20 11:27 AM, Miroslav Benes wrote:
>> I'm continuing to hone the series, what's really missing so far is arch
>> review. Most conversions are straight forward, some I need folks to
>> definitely take a look at (arm, s390). powerpc is also a bit hair right
>> now, but I'm told that 5.10 will kill a TIF flag there, so that'll make
>> it trivial once I rebase on that.
> 
> can you pretty please not add that to anything which is not going
> through kernel/entry/ ?

Certainly, tif-task_work is just a holding ground for all of it so
far. Once the core bits are agreed upon and merged, then I'll bounce
the rest through the arch maintainers. So please don't view it as
on cohesive series, it only is up until (and including):

commit 8dc14b576a9bf13dba4993e4ddb4068d39dee1e9
Author: Jens Axboe <axboe@kernel.dk>
Date:   Thu Oct 1 13:29:21 2020 -0600

    task_work: use TIF_NOTIFY_SIGNAL if available


> The amount of duplicated and differently buggy, inconsistent and
> incomplete code in syscall and exception handling is just annoying.
> 
> It's perfectly fine if we keep that #ifdeffery around for a while and
> encourage arch folks to move over to the generic infrastructure instead
> of proliferating the status quo by adding this to their existing pile.

Agree

> The #ifdef guarding this in set_notify_signal() and other core code
> places wants to be:
> 
>     #if defined(CONFIG_GENERIC_ENTRY) && defined(TIF_NOTIFY_SIGNAL)

OK no problem, I'll fix that up.

-- 
Jens Axboe