jprobe question

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

jprobe question

Zhang, Yanmin
Mostly, jprobe handler has parameters. If the parameters are changed in
the jprobe handler, should the original function use the changed values?


For example, function store_online calls cpu_down. If we register a
jprobe at cpu_down, see below function.


int cpu_down_handler(unsigned int cpu)
{
        cpu += 1;
        return cpu;
}

Assume store_online calls cpu_down with parameter cpu=2, when
cpu_down_handler exits back to original function cpu_down, should
cpu_down use the new parameter value 3(orig_cpu +1) instead of the 2
(the original value)?

My answer is no. Because c compiler might change the parameter values
even though we don't change them in c codes sometimes.

What's your idea?

Yanmin

Reply | Threaded
Open this post in threaded view
|

Re: jprobe question

Ananth N Mavinakayanahalli-2
On Tue, Nov 29, 2005 at 11:54:44AM +0800, Zhang, Yanmin wrote:

> Mostly, jprobe handler has parameters. If the parameters are changed in
> the jprobe handler, should the original function use the changed values?
>
>
> For example, function store_online calls cpu_down. If we register a
> jprobe at cpu_down, see below function.
>
>
> int cpu_down_handler(unsigned int cpu)
> {
> cpu += 1;
> return cpu;
> }
>
> Assume store_online calls cpu_down with parameter cpu=2, when
> cpu_down_handler exits back to original function cpu_down, should
> cpu_down use the new parameter value 3(orig_cpu +1) instead of the 2
> (the original value)?

It shouldn't and it won't

> My answer is no. Because c compiler might change the parameter values
> even though we don't change them in c codes sometimes.
>
> What's your idea?

If you look closely at the jprobes code, care is taken to reset the
stack/pt_regs to the original values after returning from the jprobe
handler. Changes to the parameters in the handlers should not affect
the original parameter values.

Ananth
Reply | Threaded
Open this post in threaded view
|

Re: jprobe question

Keshavamurthy, Anil S
In reply to this post by Zhang, Yanmin
On Mon, Nov 28, 2005 at 07:53:12PM -0800, Zhang, Yanmin wrote:
>
>    Mostly,  jprobe  handler has parameters. If the parameters are changed
>    in  the  jprobe  handler, should the original function use the changed
>    values?

 Good question.

>    My  answer is no. Because c compiler might change the parameter values
>    even though we don't change them in c codes sometimes.
>
>    What's your idea?
>

You are correct, gcc assumes that the callee owns the argument space and
could overwrite it. If you see the code in function setjmp_pre_handler()
(for i386 & x86_64), we are saving this area and restoring it back before
passing the control back to the probed(original) function.

For Ia64, I was under the assumption that this might not be the case, but
you proved it wrong. So for Ia64 also we need to implement the similar logic of
saving and restoring the register stack space.

Will open a bugzilla entry for this to track this bug.

Patch welcome.

Thanks,
-Anil Keshavamurthy
Reply | Threaded
Open this post in threaded view
|

RE: jprobe question

Zhang, Yanmin
In reply to this post by Zhang, Yanmin
>>-----Original Message-----

>>From: Keshavamurthy Anil S [mailto:[hidden email]]
>>Sent: 2005年11月30日 2:54
>>To: Zhang, Yanmin
>>Cc: [hidden email]; Keshavamurthy, Anil S; Mao, Bibo
>>Subject: Re: jprobe question
>>
>>On Mon, Nov 28, 2005 at 07:53:12PM -0800, Zhang, Yanmin wrote:
>>>
>>>    Mostly,  jprobe  handler has parameters. If the parameters are changed
>>>    in  the  jprobe  handler, should the original function use the changed
>>>    values?
>>
>> Good question.
>>
>>>    My  answer is no. Because c compiler might change the parameter values
>>>    even though we don't change them in c codes sometimes.
>>>
>>>    What's your idea?
>>>
>>
>>You are correct, gcc assumes that the callee owns the argument space and
>>could overwrite it. If you see the code in function setjmp_pre_handler()
>>(for i386 & x86_64), we are saving this area and restoring it back before
>>passing the control back to the probed(original) function.
>>
>>For Ia64, I was under the assumption that this might not be the case, but
>>you proved it wrong. So for Ia64 also we need to implement the similar logic
>>of
>>saving and restoring the register stack space.
>>
>>Will open a bugzilla entry for this to track this bug.
>>
>>Patch welcome.
Here are the patches. In function non_syscall, I save the ar.bsp (after instruction cover) to the scratch area below pt_regs, then calls preserve_scratch_area. preserve_scratch_area will reserve 1 new 16-byte area for next call to ia64_bad_break. Later on, kprobe handler uses pt_regs->cr_ifs and the saved ar.bsp to get the real ar.bsp and parameter number. This approach also considers user space probe. kprobe_save_bsp patch has no any impact on critical fault patch.
The jprobe patch is to save the function parameters when the jprobe is hit and restore them after break.


jprobe_protect_out_reg_ia64.patch (3K) Download Attachment
kprobe_save_bsp_to_scratch_area_2.6.14_mm1.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: jprobe question

Keshavamurthy, Anil S
In reply to this post by Zhang, Yanmin
>Here are the patches. In function non_syscall, I save the
>ar.bsp (after instruction cover) to the scratch area below
>pt_regs, then calls preserve_scratch_area.
>preserve_scratch_area will reserve 1 new 16-byte area for next
>call to ia64_bad_break. Later on, kprobe handler uses
>pt_regs->cr_ifs and the saved ar.bsp to get the real ar.bsp
>and parameter number. This approach also considers user space
>probe. kprobe_save_bsp patch has no any impact on critical fault patch.
>The jprobe patch is to save the function parameters when the
>jprobe is hit and restore them after break.

Yanmin,
        I am not sure whether using the scratch area below the pt_regs
to save ar.bsp would be the right thing to do and more over it does look
like a hack.
Clean solution would be to have a field in pt_regs to save ar.bsp, until
we
have it, here is what you can do for now.

In the setjmp_pre_handler() get the ar.bsp by unwinding the stack
and then calling unw_get_bsp() to get the ar.bsp. Save this ar.bsp in
the kcb structure and later
in longjmp_break_handler() you can use this ar.bsp without unwinding the
stack.

Let me know your thoughts and with you next version of the patch please
cc IA64 mailing list too.


Cheers,
Anil Keshavamurthy








Reply | Threaded
Open this post in threaded view
|

RE: jprobe question

Zhang, Yanmin
In reply to this post by Zhang, Yanmin
>>-----Original Message-----
>>From: Keshavamurthy, Anil S
>>Sent: 2005年12月1日 4:56
>>To: Zhang, Yanmin
>>Cc: '[hidden email]'; Mao, Bibo
>>Subject: RE: jprobe question
>>
>>>Here are the patches. In function non_syscall, I save the
>>>ar.bsp (after instruction cover) to the scratch area below
>>>pt_regs, then calls preserve_scratch_area.
>>>preserve_scratch_area will reserve 1 new 16-byte area for next
>>>call to ia64_bad_break. Later on, kprobe handler uses
>>>pt_regs->cr_ifs and the saved ar.bsp to get the real ar.bsp
>>>and parameter number. This approach also considers user space
>>>probe. kprobe_save_bsp patch has no any impact on critical fault patch.
>>>The jprobe patch is to save the function parameters when the
>>>jprobe is hit and restore them after break.
>>
>>Yanmin,
>> I am not sure whether using the scratch area below the pt_regs
>>to save ar.bsp would be the right thing to do and more over it does look like
>>a hack.
>>Clean solution would be to have a field in pt_regs to save ar.bsp,
[YM] Yes. But some bigwigs might be unhappy to add new members into pt_regs.

 until we
>>have it, here is what you can do for now.
>>
>>In the setjmp_pre_handler() get the ar.bsp by unwinding the stack
>>and then calling unw_get_bsp() to get the ar.bsp. Save this ar.bsp in the kcb
>>structure and later
>>in longjmp_break_handler() you can use this ar.bsp without unwinding the stack.
[YM] unwind costs too much time because it will create a switch_stack and unwind frame on the top of the stack. Anyway, I will work out a new patch using unwind.

Reply | Threaded
Open this post in threaded view
|

RE: jprobe question

Keshavamurthy, Anil S
In reply to this post by Zhang, Yanmin
>[YM] unwind costs too much time because it will create a
>switch_stack and unwind frame on the top of the stack. Anyway,
>I will work out a new patch using unwind.

Yes, I understand the unwind cost, but at the
same time hate quick and dirty way to store bsp
in the pt_reg reserved area. For now don't
worry about the unwind cost. Let's
push saving bsp patch separately later and at
that time we can optimize jprobes.

Also more comment before you cook up the new patch,
some people on ia64 mailing list don't like adding
asm volatile ("....") instruction in the C file.
If possible avoid this asm in the C file.

Post your new patch onto Ia64 mailing list and Cc'ing systemtap.

-Anil