dtrace discuss - Jun 2006 - implementation of dtrace_action_raise()

Hi,

Looking at dtrace_action_raise(), I''m trying to understand why the  
t_dtrace_sig field is used.  What prevents the the signal being set  
up sooner (eg. with a call to sigtoproc).  Is it a locking, or design  
issue?  Maybe it''s not allowed for some other reason?

http://cvs.opensolaris.org/source/xref/on/usr/src/uts/common/dtrace/ 
dtrace.c#4470

    4470 static void
    4471 dtrace_action_raise(uint64_t sig)
    4472 {
...
    4481 	/*
    4482 	 * raise() has a queue depth of 1 -- we ignore all subsequent
    4483 	 * invocations of the raise() action.
    4484 	 */
    4485 	if (curthread->t_dtrace_sig == 0)
    4486 		curthread->t_dtrace_sig = (uint8_t)sig;
    4487

On 14/06/2006, at 12:55 PM, Sam Lawrance wrote:
> Hi,
>
> Looking at dtrace_action_raise(), I''m trying to understand why the
> t_dtrace_sig field is used.  What prevents the the signal being set  
> up sooner (eg. with a call to sigtoproc).  Is it a locking, or  
> design issue?  Maybe it''s not allowed for some other reason?
Replying to myself,

It seems that things might be done this way so that the resulting  
probes can be correctly instrumented, without any recursive weirdness.

On Wed, Jun 14, 2006 at 12:55:28PM +1000, Sam Lawrance
wrote:
> Hi,
> 
> Looking at dtrace_action_raise(), I''m trying to understand why the
> t_dtrace_sig field is used.  What prevents the the signal being set  
> up sooner (eg. with a call to sigtoproc).  Is it a locking, or design  
> issue?  Maybe it''s not allowed for some other reason?
> 
> http://cvs.opensolaris.org/source/xref/on/usr/src/uts/common/dtrace/ 
> dtrace.c#4470
> 
>    4470 static void
>    4471 dtrace_action_raise(uint64_t sig)
>    4472 {
> ...
>    4481 	/*
>    4482 	 * raise() has a queue depth of 1 -- we ignore all subsequent
>    4483 	 * invocations of the raise() action.
>    4484 	 */
>    4485 	if (curthread->t_dtrace_sig == 0)
>    4486 		curthread->t_dtrace_sig = (uint8_t)sig;
>    4487
For two reasons:

	1. sending a signal requires that you hold p_lock, and probe context
	   does not support grabbing locks.

	2. calling sigtoproc() would involve calling a function which is
	   also not safe to call in probe context.  (the recursive issue you
	   mention in your next e-mail)

In any case, the signal is being send to the *thread*, not the process,
and the thread has to go through issig_forreal() to check its signal
status anyway.  So it''s sensible to wait to "send" the signal
until that
time, when it is safe to do so.

Make sense?

Cheers,
- jonathan

-- 
Jonathan Adams, Solaris Kernel Development

On 15/06/2006, at 1:07 AM, Jonathan Adams wrote:
> On Wed, Jun 14, 2006 at 12:55:28PM +1000, Sam Lawrance wrote:
>> Hi,
>>
>> Looking at dtrace_action_raise(), I''m trying to understand why
the
>> t_dtrace_sig field is used.  What prevents the the signal being set
>> up sooner (eg. with a call to sigtoproc).  Is it a locking, or design
>> issue?  Maybe it''s not allowed for some other reason?
>>
>> http://cvs.opensolaris.org/source/xref/on/usr/src/uts/common/dtrace/
>> dtrace.c#4470
>>
>>    4470 static void
>>    4471 dtrace_action_raise(uint64_t sig)
>>    4472 {
>> ...
>>    4481 	/*
>>    4482 	 * raise() has a queue depth of 1 -- we ignore all  
>> subsequent
>>    4483 	 * invocations of the raise() action.
>>    4484 	 */
>>    4485 	if (curthread->t_dtrace_sig == 0)
>>    4486 		curthread->t_dtrace_sig = (uint8_t)sig;
>>    4487
>
> For two reasons:
>
> 	1. sending a signal requires that you hold p_lock, and probe context
> 	   does not support grabbing locks.
>
> 	2. calling sigtoproc() would involve calling a function which is
> 	   also not safe to call in probe context.  (the recursive issue you
> 	   mention in your next e-mail)
>
> In any case, the signal is being send to the *thread*, not the  
> process,
> and the thread has to go through issig_forreal() to check its signal
> status anyway.  So it''s sensible to wait to "send" the
signal until
> that
> time, when it is safe to do so.
>
> Make sense?
Thanks for your response.  Yes, it does make sense :-)

For the mailing list record, I found some more information about this  
at Bryan Cantrill''s blog:
http://blogs.sun.com/roller/page/bmc?entry=dtrace_safety