Re: [RFC] TIF_NOTIFY_RESUME, arch/*/*/*signal*.c and all such

[Oleg Nesterov <oleg@redhat.com>]

On 04/27, Al Viro wrote:
>
> On Thu, Apr 26, 2012 at 08:37:42PM +0200, Oleg Nesterov wrote:
>
> > 	+       /* 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, &current->saved_sigmask, NULL);
> > 			^^^^^^^^^^^
> >
> > set_current_blocked(&current->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(&current->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(&current->saved_mask);
> }
> static inline sigset_t *sigmask_to_save(void)
> {
> 	struct sigset *res = &current->blocked;
> 	if (unlikely(test_restore_sigmask()))
> 		res = current->saved_sigmask;
> 	return res;
> }

Perhaps... but test_*_restore_sigmask() depends on TIF_ or TS_

> Speaking of other helpers, pulling ppc restore_sigmask() into signal.h
> (as static inline) might be a good idea.

Agreed.

> Every sigreturn instance is
> open-coding it...

Not only sigreturn. Just look at sigprocmask(SIG_SETMASK) callers, almost
all should be converted to use set_current_blocked(). For example, pselect.

> We need saner names, though; this set is too easy to
> confuse with each other.

Say, set_current_blocked_careful().

> > 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.

WARN_ON(!test_bit(TIF_SIGPENDING)) looks like the perfect documentation ;)




OK, I have read the whole series. You do understand that I can't comment
the changes in asm, but I managed to convince myself I can more or less
understand them and I see nothing wrong.

The only comment I have,
38671a3e831ed7327affb24d715f98bb99c80e56 m68k: add TIF_NOTIFY_RESUME and handle it
forgets to unexport do_signal().



The last thing. Matt, could you please look at
git://git.kernel.org/pub/scm/linux/kernel/git/viro/signal.git ? It seems to me
you already sent some of these changes (use set_current_blocked/block_sigmask).
Perhaps there are alreay in -mm or linux-next?

Oleg.