On Thu, Apr 26, 2012 at 08:37:42PM +0200, Oleg Nesterov wrote:
> 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.
Buggered manual cherry-pick - earlier there was a patch inverting
__ARCH_WANT_SYS_RT_SIGSUSPEND (only mips did _not_ have it after the
whole series, and only because it has sys_rt_sigsuspend() of its own
with unusual prototype). I ended up dropping it and mishandled conflict
resolution. Fixed.
> 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)
Yes, get_signal_to_deliver() will do it. I'd killed some instances in the
last round of signal fixes (2010), but never got around to doing that for
all architectures.
> + /* 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.
In principle, yes. FWIW, I think that the entire thing should be a helper
to go along with set_restore_sigmask(). With
if (test_and_clear_thread_flag(TIF_RESTORE_SIGMASK))
set_current_blocked(¤t->saved_sigmask);
as default implementation. The only question is where should it go -
asm/thread_info.h is not a good place due to header dependencies.
Kinda-sorta solution - in thread_info.h
{set,clear,test,test_and_clear}_restore_sigmask()
and in linux/signal.h
#ifdef HAVE_SET_RESTORE_SIGMASK
static inline void restore_saved_sigmask(void)
{
if (test_and_clear_restore_sigmask())
set_current_blocked(¤t->saved_mask);
}
static inline sigset_t *sigmask_to_save(void)
{
struct sigset *res = ¤t->blocked;
if (unlikely(test_restore_sigmask()))
res = current->saved_sigmask;
return res;
}
#endif
Speaking of other helpers, pulling ppc restore_sigmask() into signal.h
(as static inline) might be a good idea. Every sigreturn instance is
open-coding it... We need saner names, though; this set is too easy to
confuse with each other.
> 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.
Umm... Probably, and as far as I can see all callers are only reached if
we have SIGPENDING, but that requires at least documenting what's going on.
--
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
