Mailing List Archive

> collect_signal() checks sigismember(&list->signal, sig), this is not needed.
> This "sig" was just found by next_signal(), so it must be valid.

Acked-by: Roland McGrath <roland@redhat.com>

> We have a (completely broken) call to ->notifier in between, but it must not
> play with sigpending->signal bits or unlock ->siglock.

The only use of ->notifier/notifier_mask is for block_all_signals.
Its only user in the tree is drm_lock(), but it is exported too.

drm_lock is using block_all_signals to catch all the stop signals
for a purpose that is not immediately clear to me.


Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 20 May 2008, Roland McGrath wrote:
>
> drm_lock is using block_all_signals to catch all the stop signals
> for a purpose that is not immediately clear to me.

It's basically the equivalent of a "spin_lock_irq()" for DRM, where
signals are the "interrupts" that need to be blocked while holding the
lock.

In order for the user-level direct-rendering code to be able to do a lock
that is safe in the presense of signals, the locking code needs to have
some way to basically disable signals. And it could disable/re-enable them
all the time, but that would be very expensive and defeat the whole
purpose (which is that you can get the lock cheaply in user space and only
take a system call on a lock conflict).

So instead, when it gets the DRM lock, it also does that
"block_all_signals()" thing, which means that *if* a signal happens, it
can temporarily disable the signal so that the handler won't be invoced in
the critical region.

Now, I have to admit that I do not know if the current user-level code
needs that any more, and even if it does, if it could possibly be replaced
by better models. It's a hack, no question about it. The whole DRI locking
is something really odd. Don't even ask me how it's supposed to work.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

> It's basically the equivalent of a "spin_lock_irq()" for DRM, where
> signals are the "interrupts" that need to be blocked while holding the
> lock.

Well, sure, that makes sense. But that's not what it does. I can see what
it does, I just can't tell why it really makes any sense.

Despite the name, block_all_signals() in fact blocks no signals. What
it does is install notifier/notifier_mask, which makes the hook get
called for those particular signals and it can decide to delay the
signal (in a kooky fashion that's not really reliable).

drm_lock uses a mask containing only the stop signals (SIGSTOP, SIGTSTP,
SIGTTOU, SIGTTIN). So for other signals, its hook (drm_notifier) never
gets called, and signal_pending() stays set, handlers get run, etc.

So I can't tell what it's actually good for (that's at all reliable
now). But you just told me not to ask you how it's supposed to work,
and I respect that. ;-)

The best I can figure is that the user-level code in question really
does want to have signal handlers run, and to process fatal signals,
during those critical sections. It just wants to prevent a ^Z or
SIGSTOP at the critical spot leaving the lock held while the process
goes into TASK_STOPPED for an arbitrary period.


Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On 05/20, Roland McGrath wrote:
>
> > It's basically the equivalent of a "spin_lock_irq()" for DRM, where
> > signals are the "interrupts" that need to be blocked while holding the
> > lock.
>
> Well, sure, that makes sense. But that's not what it does. I can see what
> it does, I just can't tell why it really makes any sense.

As for me, I can't even see what it does,

> Despite the name, block_all_signals() in fact blocks no signals. What
> it does is install notifier/notifier_mask, which makes the hook get
> called for those particular signals and it can decide to delay the
> signal (in a kooky fashion that's not really reliable).

Suppose that the task has the pending SIG which is "blocked" by DRM.

dequeue_signal() calls ->notifier(), it nacks the signal, we clear
TIF_SIGPENDING. Then dequeue_signal() does recalc_sigpending() and
sets TIF_SIGPENDING again. We return 0 to get_signal_to_deliver(),
and then return to user-space with TIF_SIGPENDING. Endless loop ?

Even if it works somehow... "blocking" SIG means that in fact we
"block" SIG+1, SIG+2, etc if SIG is pending.

> drm_lock uses a mask containing only the stop signals (SIGSTOP, SIGTSTP,
> SIGTTOU, SIGTTIN).

and of course this can't work for multithread programs, another
thread can dequeue SIGTSTP and initiate a group stop.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/