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

[Oleg Nesterov]

On 04/13, Tejun Heo wrote:
>
> 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)?

Well, I don't know the common definition... But please note that
signandsets() does ((x) & ~(y)), so I defined nor as (x | ~y) by analogy.

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

Indeed.  But currently we only have has_pending_signals() which needs
blocked, not mask.

> but I think we
> really need to do retargeting (and the initial targeting too) more
> efficiently as you noted in the earlier commit message.

Yes, will do. And to do this we need the mask, not blocked.

But I'd prefer to do this after this series to make the first patches
simpler.

The main optimization is the first has_pending_signals() check which
can likely avoid while_each_thread() altogether. Once we start looping
we already lost. But anyway I agree, we should do this. Perhaps we can
add more optimizations later... say, perhaps we can add something like
TIF_SIGPENDING_SHARED, I am not sure.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/