This is the mail archive of the systemtap@sources.redhat.com mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Fwd: Re: [PATCH] Return probe]


Prasanna S Panchamukhi wrote:

Hi Jim, Hien,

Now this patch looks much better. Please note that this patch requires to call
kprobe_flush_task() at every do_exit()/do_execv() which adds to some amount of
performance overhead.


Yes this does add few instructions to do the cleanup but i am not sure if this is really a performance overhead.
In a data center environment process are long lived hence exit and exec account for a very small portion of the time. However this is a concern in the case of short lived process like in an interactive shell script or compile kind of environment. In the case of exit process is dyeing anyway hence few extra instructions as part of the death i don't think makes any difference. In the case of exec it is probably more relevant to be concerned. I suggest we do a small benchmark by compiling kernel sources with and without return probes and see if the addition of these few lines of code makes any perceivable performance overhead. If we notice any performance problem we can make cleanup inline. Even after code tightening we still feel performance impact is significant, we should investigate the issue further.


Also this method could not use multiple handlers feature in a cleaner way.



Can you give more details of what you mean by cleaner way?

Just a thought if a probe inside the tampoline can be avoided, since we change
the return address on stack to tampoline and now within the tampoline we can call the handlers. Then the return from tampoline can be changed to actual return address. We may need to get the pt_regs structure and avoid preemption during the handler execution .


Do you feel that there are some performance benefits by avoiding int3 in a tampoline?


I will let Jim/Hien answer this one.

Can we make this patch still cleaner by using multiple handlers feature?
Should we proceed wth this patch now?



I thought Hien already made this patch work with latest multiple handlers patch from Ananth.
I think we should proceed with this patch unless you have some strong technical objections and a better approach solve those objections.


Also from discussions followed on the mailing lists that this method does not handle the case where the return address stored on the stack is corrupted. This
can be avoided by reistering a watchpoint at the place where the return address is stored. There are other issue such as watchpoint register depletion
etc, which might add some amount of complexity.




I think we considered the watch point approach but didn't implement because number of watchpoints in most architectures are quiet limited than the number of return probes we need to activate. Coming to the problem of stack overruns and corruptions we might detect the problem of corruption with the watch points but i am not sure how we can really still track return of the function using this approach. When we detect corruption using watch point, i guess all we can do is trying to gracefully get out of the mess, anything else we can do?

Should we proceed with this limitation?

If we suspect a stack corruption can we first use watchpoint probes to solve the corruption problem before using return probes for its purpose.

If this limitation is not acceptable should we prototype the above approach?



We have this limitation with this approach but watch point approach for return probes to me has much severe limitation than this occasional corruption bugs. Should we change our design due to these kinds of occasional corruption problems? Let me be a devils advocate and say what if the kprobes structure itself gets corrupted due to a bad pointer, should we change the kprobes design for such occasional bugs.

Just a thought if we can implement the exit probe feature in much cleaner and simpler way.
One of the alternative methods mentioned in your design document that "inserting a probepoint every time a probed function is entered, and the removal of the probepoint after the function returns" seems to be much simpler.
Also the problem mentioned in your design document about this method that "the instruction at the RA may be executed even if the probed function isn't called"
can be solved by tracking the current task associated with probe.


We can probably track the current task but to me this has two disadvantages one is we will get way many traps then function returns. The second the task of keeping track of which traps are real and which are false makes it difficult to associate exit calls with corresponding entry calls hence prone to bugs.

Ofcourse the problem of return address corruption will exist here also.

How about prototyping this alternative approach just to see how this looks ?


I am not sure we are going to learn lot of insight by prototyping this approach.

Is there a simpler way of implementing the exit probe feature ?


I think if there is an obvious better way someone would have suggested, that makes me feel it is not easy to come up with much more simpler than the current approach. It is possible that we can make the current implementation better by cleaning up the code.

Just to summarize your main concerns i hear about this approach are
1) We are adding little bit of additional cleanup code in the exit and exec paths
2) If there is a stack corruption we have no clue of when the function returns
I tried my best to address these concerns in this reply.


Did i miss any other concerns?

Your suggestions are welcome.

Thanks
Prasanna



bye,
Vara Prasad


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]