Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

[Tejun Heo <tj@kernel.org>]

Hello, Oleg.

On Mon, Apr 11, 2011 at 07:21:37PM +0200, Oleg Nesterov wrote:
> --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget	2011-04-06 21:33:50.000000000 +0200
> +++ sigprocmask/include/linux/signal.h	2011-04-11 18:16:51.000000000 +0200
> @@ -126,10 +126,14 @@ _SIG_SET_BINOP(sigandsets, _sig_and)
>  #define _sig_nand(x,y)	((x) & ~(y))
>  _SIG_SET_BINOP(signandsets, _sig_nand)
>  
> +#define _sig_nor(x,y)	((x) | ~(y))
> +_SIG_SET_BINOP(signorsets, _sig_nor)
> +
>  #undef _SIG_SET_BINOP
>  #undef _sig_or
>  #undef _sig_and
>  #undef _sig_nand
> +#undef _sig_nor

I'm confused.  Isn't nand ^(A&B) and nor ^(A|B)?

>  #define _SIG_SET_OP(name, op)						\
>  static inline void name(sigset_t *set)					\
> --- sigprocmask/kernel/signal.c~4_sigprocmask_retarget	2011-04-10 21:57:42.000000000 +0200
> +++ sigprocmask/kernel/signal.c	2011-04-11 18:02:22.000000000 +0200
> @@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set, 
>  	}
>  
>  	spin_lock_irq(&tsk->sighand->siglock);
> +	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
> +		sigset_t not_newblocked;
> +		signorsets(&not_newblocked, &current->blocked, &newset);
> +		retarget_shared_pending(tsk, &not_newblocked);

I think it would be much easier to follow the logic if
retarget_shared_pending() took target mask instead of blocked
(ie. negation of blocked) and there were more comments.  Combined with
the confusing definition of nor, I had to spend quite some time trying
to wrap my head around it but the logic here isn't all that complex
and it should have been easier.

Other than that, I agree with the proposed changes, but I think we
really need to do retargeting (and the initial targeting too) more
efficiently as you noted in the earlier commit message.  Altering
signal mask is far hotter path than actual signal delivery.  Although
the thread walking code wouldn't get activated unless signal is
already pending, I still think we better avoid looping threads
unnecessarily.

Thanks.

-- 
tejun