Re: [RFC][PATCH 2/5] signals: Introduce per-thread siglock and action rwlock

[Matt Fleming <matt@console-pimps.org>]

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