2006-02-18 16:54:27

by Oleg Nesterov

Subject: [PATCH] introduce sig_needs_tasklist() helper

In my opinion this patch cleanups the code. Please
say 'nack' if you think differently.

Signed-off-by: Oleg Nesterov <[email protected]>

--- 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;
}

2006-02-21 02:12:32

by 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 <[email protected]>
>
> --- 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

2006-02-21 17:08:32

by Oleg Nesterov

Subject: Re: [PATCH] introduce sig_needs_tasklist() helper

"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.

2006-02-21 18:39:46

by Paul E. McKenney

Subject: Re: [PATCH] introduce sig_needs_tasklist() helper

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