lkml.org 
[lkml]   [2011]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC][PATCH 2/5] signals: Introduce per-thread siglock and action rwlock
Matt, sorry for huge delay.

On 04/05, Matt Fleming wrote:
>
> Try to reduce the amount of time we hold the shared siglock by
> introducing a per-thread siglock that protects each thread's 'pending'
> queue. Currently, the shared siglock protects both the shared and
> private signal queues. As such, it is a huge point of contention when
> generating and delivering signals to single threads in a multithreaded
> process. For example, even if a thread is delivering a signal to
> itself (thereby putting a signal on its private signal queue) it must
> acquire the shared siglock.
>
> To improve signal generation scalability we only acquire the shared
> siglock when generating a shared signal. If we're generating a private
> signal which will be delivered to a specific thread (and that signal
> does not affect an entire thread group) then we can optimise that case
> by only taking the per-thread siglock. However, the case where we're
> sending a fatal signal to a specific thread is special because we need
> to modify tsk->signal->flags, so we need to be holding the shared
> siglock.
>
> Note that we now hold both the shared and per-thread siglock when
> delivering a private signal. That will be fixed in the next patch so
> that signal delivery scales with signal generation.
>
> Also, because we now run signal code paths without holding the shared
> siglock at all, it can no longer be used to protect tsk->sighand->action.
> So we introduce a rwlock for protecting tsk->sighand->action. A rwlock
> is better than a spinlock in this case because there are many more
> readers than writers and a rwlock allows us to scale much better than
> a spinlock.

So. This complicates the locking enormously. I must admit, I dislike
this very much. Yes, the signals are slow... But this returns us to the
original question: do we really care? Of course, it is always nice to
makes the things faster, but I am not sure the added complexity worth
the trouble.

Random example,

int
force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
{
unsigned long int flags;
int ret, blocked, ignored;
struct k_sigaction *action;
int shared_siglock;

write_lock_irqsave(&t->sighand->action_lock, flags);
spin_lock(&t->sighand->siglock);
action = &t->sighand->action[sig-1];
ignored = action->sa.sa_handler == SIG_IGN;
blocked = sigismember(&t->blocked, sig);
if (blocked || ignored) {
action->sa.sa_handler = SIG_DFL;
if (blocked) {
sigdelset(&t->blocked, sig);
recalc_sigpending_and_wake(t);
}
}
if (action->sa.sa_handler == SIG_DFL)
t->signal->flags &= ~SIGNAL_UNKILLABLE;

shared_siglock = sig_shared(sig) || sig_fatal(t, sig);
if (!shared_siglock) {
spin_unlock(&t->sighand->siglock);
spin_lock(&t->siglock);
}

ret = specific_send_sig_info(sig, info, t);

if (!shared_siglock)
spin_unlock(&t->siglock);
else
spin_unlock(&t->sighand->siglock);

write_unlock_irqrestore(&t->sighand->action_lock, flags);
return ret;
}

This doesn't look very nice ;)

To me personally, the only "real" performance problem is kill-from-irq
(posix timers is the worst case), this should be solved somehow but
these changes can't help...

Anyway, the patches do not look right.

> +/* Should the signal be placed on shared_pending? */
> +#define sig_shared(sig) \
> + (((sig) < SIGRTMIN) && \
> + siginmask(sig, SIG_KERNEL_STOP_MASK | rt_sigmask(SIGCONT)))

OK, this is wrong but we already discussed this.

> @@ -142,7 +142,9 @@ static void __exit_signal(struct task_struct *tsk)
> * Do this under ->siglock, we can race with another thread
> * doing sigqueue_free() if we have SIGQUEUE_PREALLOC signals.
> */
> + spin_lock(&tsk->siglock);
> flush_sigqueue(&tsk->pending);
> + spin_unlock(&tsk->siglock);

This only protects flush_sigqueue(), but this is not enough.

tkill() can run without ->siglock held, how can it ensure the target
can't exit before we take task->siglock?


> @@ -976,7 +1022,12 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
> * (and therefore avoid the locking that would be necessary to
> * do that safely).
> */
> - if (group || sig_kernel_stop(sig) || sig == SIGCONT)
> + if (group || sig_shared(sig) || sig_fatal(t, sig))
> + assert_spin_locked(&t->sighand->siglock);
> + else
> + assert_spin_locked(&t->siglock);
> +
> + if (group || sig_shared(sig))
> pending = &t->signal->shared_pending;
> else
> pending = &t->pending;

Well. But later then we call complete_signal()->signal_wake_up(). And this
needs ->siglock. Now I recall why it is needed. Obviously, to serialize with
recalc_sigpending().

Note also that if you use task->siglock for sigaddset(t->pending), then
recalc_sigpending() should take this lock as well.

> @@ -1123,11 +1175,34 @@ int do_send_sig_info(int sig, struct siginfo *info, struct task_struct *p,
> unsigned long flags;
> int ret = -ESRCH;
>
> - if (lock_task_sighand(p, &flags)) {
> + read_lock_irqsave(&p->sighand->action_lock, flags);

How so? This is unsafe, p can exit, ->sighand can be NULL. Or exec can
change ->sighand, we can take the wrong lock.

And there are more wrong changes like this...

> @@ -1485,14 +1580,24 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
> {
> int sig = q->info.si_signo;
> struct sigpending *pending;
> - unsigned long flags;
> + unsigned long flags, _flags;
> + int shared_siglock;
> int ret;
>
> BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>
> ret = -1;
> - if (!likely(lock_task_sighand(t, &flags)))
> - goto ret;
> + read_lock_irqsave(&t->sighand->action_lock, flags);
> + shared_siglock = group || sig_shared(sig) || sig_fatal(t, sig);
> + if (!shared_siglock) {
> + spin_lock(&t->siglock);
> + pending = &t->pending;
> + } else {
> + if (!likely(lock_task_sighand(t, &_flags)))
> + goto ret;
> +
> + pending = &t->signal->shared_pending;

This looks wrong wrong. We shouldn't do s/pending/shared_pending/ if
sig_fatal(). The signal can be blocked the this task can be ptraced.

> @@ -1666,7 +1779,8 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
> }
>
> sighand = parent->sighand;
> - spin_lock_irqsave(&sighand->siglock, flags);
> + read_lock_irqsave(&sighand->action_lock, flags);
> + spin_lock(&sighand->siglock);

Why do we need both? (the same for do_notify_parent)

Oleg.



\
 
 \ /
  Last update: 2011-04-13 21:45    [W:0.139 / U:6.520 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site