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.
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
