[Fwd: Re: [PATCH] Return probe]

[Hien Nguyen]

Hi Prasanna,

Here's the new return probe patch for Ananth's multiprobe take3. I 
addressed most of your concerns.  There is no changes in the existing 
kprobe layer (include kprobe data structure, un/register_kprobe) except 
for a small change to the arch/i386/post_kprobe_handler()

Per Vara's request, I cc'ed this message to the SystemTap mailing list.

Hien.

Prasanna S Panchamukhi wrote:

>Hien and Jim,
>
>Please see my comments below.
>
>Thanks
>Prasanna
>
>+
> struct kprobe {
> 	struct hlist_node hlist;
> 
>@@ -68,6 +71,9 @@
> 
> 	/* copy of the original instruction */
> 	struct arch_specific_insn ainsn;
>+
>+	/* point to kretprobe */
>+	struct kretprobe *rp;
> };
>^^^^^^^^^^^^^^^^^^^^^^^^
>	can we avoid adding this to the kprobes structure? since if every 
>feature requires adding one/two pointers, 5-10 features will make the kprobe 
>structure very big. And even tough to maintain/enhance the code as well in 
>future. We have long term advantages by avoiding changes to the existing code.
>  
>

Done. No more fussing with kprobe structure.

> 
>+struct kretprobe_instance {
>+	struct list_head list;
>+	struct hlist_node hlist;
>+	struct kretprobe *rp;
>+	void *ret_addr;
>+	void *stack_addr;
>+};
>+
>^^^^^^^^^^^^^^^^
>	can this be made per-cpu datastructure
>+int register_kretprobe(struct kretprobe *rp)
>+{
>+	int ret = 0;
>+	static int kretprobe_init_setup = 0;
>+	struct kretprobe_instance *inst;
>+	struct kprobe *p;
>+	int maxinst, i;
>+	
>+	if (!arch_supports_kretprobes) {
>+		return -ENOSYS;
>+	}
>+	if (kretprobe_init_setup == 0) {
>+		init_kretprobes();
>^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 
>		can we do this in init_kprobe() ???
>+	/* Create a kprobe obj to associcate with the kretprobe. If there is an
>+	 * existing probe at this addr, we will use that probe and free this.
>+	 */	
>+	p = kmalloc(sizeof(struct kprobe), GFP_KERNEL);
>^^^^^^^^^^^^^^^^^
>	since we need to register entrypoint probe for every exit probe, this 
>structure can be part of kretprobe structure itself.
>+	/* todo: we need to deal with probe that has been registered */
>^^^^^^^^^^^^^^^^^^^^^^^ we need not bother if the probe is registered or not,
>we need to register our handler at the entrypoint. It is the multiprobes headache
>to worry about. All that we know is after we register entrypoint kprobe control
>is passed to our defined handler. Multiprobe should assist us direclty allowing us
>to register the entrypoint handler along with exisiting probes at the same address.
>+	
>+	if ((ret = _register_kprobe(p)) != 0) {
>^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>	can we use multiprobe's interface without even modifiying it.
>
>We need to think, if return probe feature can be implemented without
>even changing the existing kprobe/multiprobe code and data structures.
>If something is unavoidable, then we can modify the kprobes code.
>Hien, please get in touch with me before you start making the changes,
>may be you can ST me. I need to discuss about this with you.
>On Wed, Apr 13, 2005 at 11:39:18AM -0700, Hien Nguyen wrote:
>  
>
>>Hi Prasanna,
>>
>>Please see my reply below.
>>
>>Annath, this is the return probe for your multiprobe take3. Please take 
>>a look.
>>
>>Thanks, Hien.
>>
>>Prasanna S Panchamukhi wrote:
>>
>>    
>>
>>>Hien, Jim,
>>>
>>>I have just started reviewing your code and I have few questions:
>>>
>>>1. Now multiprobe feature is avaliable, using this feature some code from 
>>>kreturn register/unregister routine can it be removed? Can you modify your
>>>design to build on multiprobe interface.
>>>
>>>
>>>      
>>>
>>Yes, you are right. Attached is the return probe patch for Ananth's 
>>multiprobe take3 (I just happened to pick up Ananth patch to carry on 
>>the returnprobe works). I also attached Ananth multiprobe take3 patch 
>>here for your convenience.
>>
>>    
>>
>>>2. I found that unregister_kprobe() is modified , can this 
>>>be avoided?
>>>
>>>
>>>      
>>>
>>No change in the unregister_kprobe in this version since it is now just 
>>a wrapper. I made a very simply modification to  
>>unregister_kprobe_single and unregister_aggr_kprobe to avoid code 
>>duplication.
>>
>>    
>>
>>>3. You can modified kprobe_handler(), which can be part of external 
>>>handler,
>>>can it be moved out?
>>>
>>>
>>>      
>>>
>>Yes, I moved this code out to the kprobe's pre_handler that associated 
>>with the kretprobe.
>>
>>    
>>
>>>4. You have again modified resume_execution(), which can be moved out to 
>>>external handler.
>>>
>>>
>>>      
>>>
>>I moved this code out to the trampoline probe post_handle. No 
>>modification to resume_execution() now.
>>
>>    
>>
>>>Thanks
>>>Prasanna
>>>
>>>
>>>
>>>      
>>>
>
>
>
>
>  
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: new_retprobe3.patch
Type: text/x-patch
Size: 16320 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/systemtap/attachments/20050415/116f81c9/attachment.bin>