Messages in this thread | | | Date | Mon, 20 Feb 2006 18:13:02 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] introduce sig_needs_tasklist() helper |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |