Re: [PATCH] arc: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe <axboe@kernel.dk>]

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 */

-- 
Jens Axboe


_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc