diff mbox series

[v3,1/8] Documentation: probes: Add a new ret_ip callback parameter

Message ID 169181860742.505132.14215380532909911090.stgit@devnote2 (mailing list archive)
State Superseded
Headers show
Series bpf: fprobe: rethook: Use ftrace_regs instead of pt_regs | expand

Commit Message

Masami Hiramatsu (Google) Aug. 12, 2023, 5:36 a.m. UTC
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add a new ret_ip callback parameter description.

Fixes: cb16330d1274 ("fprobe: Pass return address to the handlers")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Documentation/trace/fprobe.rst |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Florent Revest Aug. 17, 2023, 8:57 a.m. UTC | #1
On Sat, Aug 12, 2023 at 7:36 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Add a new ret_ip callback parameter description.
>
> Fixes: cb16330d1274 ("fprobe: Pass return address to the handlers")
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Documentation/trace/fprobe.rst |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/trace/fprobe.rst b/Documentation/trace/fprobe.rst
> index 40dd2fbce861..a6d682478147 100644
> --- a/Documentation/trace/fprobe.rst
> +++ b/Documentation/trace/fprobe.rst
> @@ -91,9 +91,9 @@ The prototype of the entry/exit callback function are as follows:
>
>  .. code-block:: c
>
> - int entry_callback(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs, void *entry_data);
> + int entry_callback(struct fprobe *fp, unsigned long entry_ip, unsigned long ret_ip, struct pt_regs *regs, void *entry_data);
>
> - void exit_callback(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs, void *entry_data);
> + void exit_callback(struct fprobe *fp, unsigned long entry_ip, unsigned long ret_ip, struct pt_regs *regs, void *entry_data);
>
>  Note that the @entry_ip is saved at function entry and passed to exit handler.
>  If the entry callback function returns !0, the corresponding exit callback will be cancelled.
> @@ -108,6 +108,10 @@ If the entry callback function returns !0, the corresponding exit callback will
>          Note that this may not be the actual entry address of the function but
>          the address where the ftrace is instrumented.
>
> +@ret_ip
> +        This is the return address of the traced function. This can be used
> +        at both entry and exit.

Maybe that's just the lack of coffee but I had to think twice to
understand what this paragraph meant :) On my first pass I thought
this meant "the address of the return instruction", which made little
sense since there can of course be multiple "ret"s in a function. I
like the name in the fprobe code "parent_ip" because I find it conveys
better that this is an address in the caller of the traced function.
I'm also fine with this "ret_ip" but I propose we modify the paragraph
a little bit to something like:

This is the address that the traced function will return to, somewhere
in its caller. This can be used at both entry and exit.
Masami Hiramatsu (Google) Aug. 18, 2023, 10:59 a.m. UTC | #2
On Thu, 17 Aug 2023 10:57:18 +0200
Florent Revest <revest@chromium.org> wrote:

> On Sat, Aug 12, 2023 at 7:36 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Add a new ret_ip callback parameter description.
> >
> > Fixes: cb16330d1274 ("fprobe: Pass return address to the handlers")
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  Documentation/trace/fprobe.rst |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/trace/fprobe.rst b/Documentation/trace/fprobe.rst
> > index 40dd2fbce861..a6d682478147 100644
> > --- a/Documentation/trace/fprobe.rst
> > +++ b/Documentation/trace/fprobe.rst
> > @@ -91,9 +91,9 @@ The prototype of the entry/exit callback function are as follows:
> >
> >  .. code-block:: c
> >
> > - int entry_callback(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs, void *entry_data);
> > + int entry_callback(struct fprobe *fp, unsigned long entry_ip, unsigned long ret_ip, struct pt_regs *regs, void *entry_data);
> >
> > - void exit_callback(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs, void *entry_data);
> > + void exit_callback(struct fprobe *fp, unsigned long entry_ip, unsigned long ret_ip, struct pt_regs *regs, void *entry_data);
> >
> >  Note that the @entry_ip is saved at function entry and passed to exit handler.
> >  If the entry callback function returns !0, the corresponding exit callback will be cancelled.
> > @@ -108,6 +108,10 @@ If the entry callback function returns !0, the corresponding exit callback will
> >          Note that this may not be the actual entry address of the function but
> >          the address where the ftrace is instrumented.
> >
> > +@ret_ip
> > +        This is the return address of the traced function. This can be used
> > +        at both entry and exit.
> 
> Maybe that's just the lack of coffee but I had to think twice to
> understand what this paragraph meant :) On my first pass I thought
> this meant "the address of the return instruction", which made little
> sense since there can of course be multiple "ret"s in a function. I
> like the name in the fprobe code "parent_ip" because I find it conveys
> better that this is an address in the caller of the traced function.
> I'm also fine with this "ret_ip" but I propose we modify the paragraph
> a little bit to something like:
> 
> This is the address that the traced function will return to, somewhere
> in its caller. This can be used at both entry and exit.

Thanks, that makes it more clear. I'll update it.
diff mbox series

Patch

diff --git a/Documentation/trace/fprobe.rst b/Documentation/trace/fprobe.rst
index 40dd2fbce861..a6d682478147 100644
--- a/Documentation/trace/fprobe.rst
+++ b/Documentation/trace/fprobe.rst
@@ -91,9 +91,9 @@  The prototype of the entry/exit callback function are as follows:
 
 .. code-block:: c
 
- int entry_callback(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs, void *entry_data);
+ int entry_callback(struct fprobe *fp, unsigned long entry_ip, unsigned long ret_ip, struct pt_regs *regs, void *entry_data);
 
- void exit_callback(struct fprobe *fp, unsigned long entry_ip, struct pt_regs *regs, void *entry_data);
+ void exit_callback(struct fprobe *fp, unsigned long entry_ip, unsigned long ret_ip, struct pt_regs *regs, void *entry_data);
 
 Note that the @entry_ip is saved at function entry and passed to exit handler.
 If the entry callback function returns !0, the corresponding exit callback will be cancelled.
@@ -108,6 +108,10 @@  If the entry callback function returns !0, the corresponding exit callback will
         Note that this may not be the actual entry address of the function but
         the address where the ftrace is instrumented.
 
+@ret_ip
+        This is the return address of the traced function. This can be used
+        at both entry and exit.
+
 @regs
         This is the `pt_regs` data structure at the entry and exit. Note that
         the instruction pointer of @regs may be different from the @entry_ip