Re: [PATCH] introduce sig_needs_tasklist() helper

["Paul E. McKenney" <paulmck@us.ibm.com>]

On Sat, Feb 18, 2006 at 09:12:04PM +0300, Oleg Nesterov wrote:
> In my opinion this patch cleanups the code. Please
> say 'nack' if you think differently.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- 2.6.16-rc3/kernel/signal.c~4_SNT	2006-02-18 23:26:51.000000000 +0300
> +++ 2.6.16-rc3/kernel/signal.c	2006-02-18 23:43:23.000000000 +0300
> @@ -147,6 +147,9 @@ static kmem_cache_t *sigqueue_cachep;
>  #define sig_kernel_stop(sig) \
>  		(((sig) < SIGRTMIN)  && T(sig, SIG_KERNEL_STOP_MASK))
>  
> +#define sig_needs_tasklist(sig) \
> +		(((sig) < SIGRTMIN)  && T(sig, SIG_KERNEL_STOP_MASK | M(SIGCONT)))
> +
>  #define sig_user_defined(t, signr) \
>  	(((t)->sighand->action[(signr)-1].sa.sa_handler != SIG_DFL) &&	\
>  	 ((t)->sighand->action[(signr)-1].sa.sa_handler != SIG_IGN))
> @@ -1202,7 +1205,7 @@ kill_proc_info(int sig, struct siginfo *
>  	struct task_struct *p;
>  
>  	rcu_read_lock();
> -	if (unlikely(sig_kernel_stop(sig) || sig == SIGCONT)) {
> +	if (unlikely(sig_needs_tasklist(sig))) {
>  		read_lock(&tasklist_lock);
>  		acquired_tasklist_lock = 1;
>  	}

Seems to me to be an improvement, but why not also encapsulate the
lock acquisition, something like:

	static inline int sig_tasklist_lock(int sig)
	{
		if (unlikely(sig_needs_tasklist(sig)) {
			read_lock(&tasklist_lock);
			return 1;
		}
		return 0;
	}

	static inline void sig_tasklist_unlock(int acquired_tasklist_lock)
	{
		if (acquired_tasklist_lock)
			read_unlock(&tasklist_lock);
	}

	...

	rcu_read_lock();
	acquired_tasklist_lock = sig_tasklist_lock(sig);

	...

	sig_tasklist_unlock(acquired_tasklist_lock);

Seem reasonable?

						Thanx, Paul