lkml.org 
[lkml]   [2011]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH 2/5] signals: Introduce per-thread siglock and action rwlock
On Thu, 14 Apr 2011 21:00:12 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

[Added Chris to Cc. Chris, check out point 3 below]

> No, tsk->signal can't go away, ->sighand can. This means that
> prepare_signal()->sig_ignored() is not safe.

Sorry, that was a typo - I meant tsk->sighand.

> Another problem is, __send_signal() shouldn't add the new sigqueue to
> tsk->pending if this tsk has already passed __exit_signal()->flush_sigqueue().

Right.

> > While we're discussing the technique for stopping tasks from exiting...
> >
> > Is there a reason that a short-term reference counter isn't used to
> > prevent this, instead of taking the siglock?
>
> Well, sighand->count is the reference counter. The problem is, ->sighand
> is not per-process, we can share it with abother CLONE_SIGHAND process
> and de_thread() can change ->sighand during exec.

What I meant was, a reference to say "You can't change/free ->sighand
because I'm reading/modifying it". So you'd have two new functions,
get_sighand() and put_sighand(), which would protect the sighand from
changing while you were looking at it. Obviously, you'd still need to
see if sighand = NULL, but you wouldn't need to grab the shared
siglock.

Note how this is different from sighand->count. sighand->count is a
much longer term reference which stops it being freed while a task is
using it, kinda like a "Don't free _MY_ sighand" reference, whereas
what I'm talking about is a "I'm touching YOUR sighand, so don't
change/free it" reference, e.g. a short term ref for when we're
operating on a target task. It could be that the two references can
really be just one atomic_t, I would have to write the code to figure
that out.

(Note I haven't thought this through all the way yet. There may be a
reason that the above idea is the most stupid thing anyone's ever heard)

Now, at the moment that suggestion just seems like needless overhead
because siglock already provides the features we want. But, my problem
with siglock is,

1. It needs to be acquired to stop a task passing through
__exit_signal().

2. It protects bits of signal_struct and that struct is getting
pretty bloated and siglock is being used to protect lots of
different things. Do we really need to completely prevent
signals being generated/delivered to a thread group while we
modify signal->it[] in kernel/itimer.c?

3. I suspect most people find the rules of ->sighand pretty
confusing. Just look at

arch/tile/kernel/hardwall.c:do_hardwall_trap()

the use of siglock there looks buggy to me. What if 'p' passes
through __exit_signal()? 'p' might not have reached free_task()
yet and so won't have called hardwall_deactivate(), which
means 'p' could still be on rect->task_head. Chris?

4. It's a heavily contended lock in thread groups (which was
the reason for this patch series).

> I was going to try to add some cleanups here after the scope of ->signal
> was changed, but right now I can't recall what I had in mind. Anyway,
> everything in sighand_struct needs ->siglock anyway, a lockless access
> doesn't buy too much currently.

Well, as I think my patch series shows, (in theory) not needing to
acquire ->siglock gets us decent scalability improvements (in practice
it was buggy as hell but that should be fixable ;-).

Do you have any recollection of the cleanups? signal_struct needs to be
put on a diet for sure.

--
Matt Fleming, Intel Open Source Technology Center


\
 
 \ /
  Last update: 2011-04-16 15:11    [W:0.762 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site