Messages in this thread Patch in this message | | | Date | Thu, 26 Apr 2012 20:37:42 +0200 | From | Oleg Nesterov <> | Subject | Re: [RFC] TIF_NOTIFY_RESUME, arch/*/*/*signal*.c and all such |
| |
On 04/24, Al Viro wrote: > > Untested variants pushed into signal.git#master; will test tomorrow. In > the meanwhile, any code review (and testing of the entire thing on as many > targets as possible) would be very welcome.
I started to read these patches today, will continue tomorrow. Somehow I got stuck at f1fcb14721b4f1e65387d4563311f15f0bd33684, please see the question below. And a couple of minor nits.
b4b620b87fd2f388cf4c13fea21f31bed7c9a1b0 new helper: sigsuspend()
Looks obviously correct but I do not understand this chunk in kernel.c,
+ #ifndef __ARCH_HAS_SYS_RT_SIGSUSPEND + /** + * sys_rt_sigsuspend - replace the signal mask for a value with the + #ifdef __ARCH_WANT_SYS_RT_SIGSUSPEND
So this checks the (never used/defined?) __ARCH_HAS_SYS_RT_SIGSUSPEND but comments out __ARCH_WANT_SYS_RT_SIGSUSPEND. Looks like a typo.
6b78370886e4f61187404b7737a831281bde35e8 xtensa: switch to generic rt_sigsuspend(2) and d978bf9dd41728dd60fe2269493fe8f21d28eef3 h8300: switch to saved_sigmask-based sigsuspend/rt_sigsuspend
(off-topic, but do_signal()->try_to_freeze() looks unneeded and wrong)
+ /* If there's no signal to deliver, we just restore the saved mask. */ + if (test_thread_flag(TIF_RESTORE_SIGMASK)) { + clear_thread_flag(TIF_RESTORE_SIGMASK); + sigprocmask(SIG_SETMASK, ¤t->saved_sigmask, NULL); ^^^^^^^^^^^
set_current_blocked(¤t->saved_sigmask) looks better.
f1fcb14721b4f1e65387d4563311f15f0bd33684 alpha: tidy signal delivery up
Everything looks fine, but I have the off-topic question. The changelog says:
* checking for TIF_SIGPENDING is enough; set_restart_sigmask() sets this one as well.
Agreed, but why set_restore_sigmask() sets TIF_SIGPENDING? It should be never used without signal_pending() == T.
IOW, do you know a reason why this patch
--- x/arch/x86/include/asm/thread_info.h +++ x/arch/x86/include/asm/thread_info.h @@ -264,7 +264,7 @@ static inline void set_restore_sigmask(v { struct thread_info *ti = current_thread_info(); ti->status |= TS_RESTORE_SIGMASK; - set_bit(TIF_SIGPENDING, (unsigned long *)&ti->flags); + WARN_ON(!test_bit(TIF_SIGPENDING, (unsigned long *)&ti->flags)); } static inline bool is_ia32_task(void)
is not correct?
OK, say, sys_sigsuspend() does
current->state = TASK_INTERRUPTIBLE; schedule(); set_restore_sigmask(); return -ERESTARTNOHAND;
so set_bit(TIF_SIGPENDING) saves us from the "spurious wakeup". But is it really possible?
We had the bugs in ptrace some time ago (and iirc this is why sys_pause checks signal_pending), but is there any reason today why the TASK_INTERRUPTIBLE task can return from schedule() without SIGPENDING? (of course, ignoring the case when this task was added to some wait_queue_head_t).
I am just curious. Perhaps set_restore_sigmask() sets SIGPENDING just to be safer, but otoh this can hide the problem.
Oleg.
| |