* [PATCH] introduce sig_needs_tasklist() helper
@ 2006-02-18 18:12 Oleg Nesterov
2006-02-21 2:13 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2006-02-18 18:12 UTC (permalink / raw)
To: Paul E. McKenney, Ingo Molnar; +Cc: linux-kernel, Andrew Morton
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;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] introduce sig_needs_tasklist() helper
2006-02-18 18:12 [PATCH] introduce sig_needs_tasklist() helper Oleg Nesterov
@ 2006-02-21 2:13 ` Paul E. McKenney
2006-02-21 18:25 ` Oleg Nesterov
0 siblings, 1 reply; 4+ messages in thread
From: Paul E. McKenney @ 2006-02-21 2:13 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Ingo Molnar, linux-kernel, Andrew Morton
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] introduce sig_needs_tasklist() helper
2006-02-21 2:13 ` Paul E. McKenney
@ 2006-02-21 18:25 ` Oleg Nesterov
2006-02-21 18:40 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2006-02-21 18:25 UTC (permalink / raw)
To: paulmck; +Cc: Ingo Molnar, linux-kernel, Andrew Morton
"Paul E. McKenney" wrote:
>
> On Sat, Feb 18, 2006 at 09:12:04PM +0300, Oleg Nesterov wrote:
> > +#define sig_needs_tasklist(sig) \
> > + (((sig) < SIGRTMIN) && T(sig, SIG_KERNEL_STOP_MASK | M(SIGCONT)))
> > +
>
> 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);
> }
I hope we will have
#define sig_needs_tasklist(sig) (sig == SIGCONT)
really soon (I planned to submit the final bits today, but
for some stupid reasons I can't do anything till weekend),
so I think it's better to kill 'acquired_tasklist_lock' and
just do:
void sig_tasklist_lock(sig)
{
if (sig_needs_tasklist(sig))
read_lock(&tasklist_lock);
}
void sig_tasklist_unlock(sig)
{
if (sig_needs_tasklist(sig));
read_unlock(&tasklist_lock);
}
Oleg.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] introduce sig_needs_tasklist() helper
2006-02-21 18:25 ` Oleg Nesterov
@ 2006-02-21 18:40 ` Paul E. McKenney
0 siblings, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2006-02-21 18:40 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Ingo Molnar, linux-kernel, Andrew Morton
On Tue, Feb 21, 2006 at 09:25:25PM +0300, Oleg Nesterov wrote:
> "Paul E. McKenney" wrote:
> >
> > On Sat, Feb 18, 2006 at 09:12:04PM +0300, Oleg Nesterov wrote:
> > > +#define sig_needs_tasklist(sig) \
> > > + (((sig) < SIGRTMIN) && T(sig, SIG_KERNEL_STOP_MASK | M(SIGCONT)))
> > > +
> >
> > 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);
> > }
>
> I hope we will have
>
> #define sig_needs_tasklist(sig) (sig == SIGCONT)
>
> really soon (I planned to submit the final bits today, but
> for some stupid reasons I can't do anything till weekend),
> so I think it's better to kill 'acquired_tasklist_lock' and
> just do:
>
> void sig_tasklist_lock(sig)
> {
> if (sig_needs_tasklist(sig))
> read_lock(&tasklist_lock);
> }
>
> void sig_tasklist_unlock(sig)
> {
> if (sig_needs_tasklist(sig));
> read_unlock(&tasklist_lock);
> }
Even better!
Thanx, Paul
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-02-21 18:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-18 18:12 [PATCH] introduce sig_needs_tasklist() helper Oleg Nesterov
2006-02-21 2:13 ` Paul E. McKenney
2006-02-21 18:25 ` Oleg Nesterov
2006-02-21 18:40 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox