diff mbox series

[v2,2/3] compiler: inline does not imply notrace

Message ID 20230525210040.3637-3-namit@vmware.com
State New
Headers
Series kprobes: notrace enhancements |

Commit Message

Nadav Amit May 25, 2023, 9 p.m. UTC 
  From: Nadav Amit <namit@vmware.com>

Functions that are marked as "inline" are currently also not tracable.
This limits tracing functionality for many functions for no reason.
Apparently, this has been done for two reasons.

First, as described in commit 5963e317b1e9d2a ("ftrace/x86: Do not
change stacks in DEBUG when calling lockdep"), it was intended to
prevent some functions that cannot be traced from being traced as these
functions were marked as inline (among others).

Yet, this change has been done a decade ago, and according to Steven
Rostedt, ftrace should have improved and hopefully resolved nested
tracing issues by now. Arguably, if functions that should be traced -
for instance since they are used during tracing - still exist, they
should be marked as notrace explicitly.

The second reason, which Steven raised, is that attaching "notrace" to
"inline" prevented tracing differences between different configs, which
caused various problem. This consideration is not very strong, and tying
"inline" and "notrace" does not seem very beneficial. The "inline"
keyword is just a hint, and many functions are currently not tracable
due to this reason.

Disconnect "inline" from "notrace".

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 include/linux/compiler_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steven Rostedt May 26, 2023, 2:28 a.m. UTC | #1 
On Thu, 25 May 2023 14:00:39 -0700
Nadav Amit <nadav.amit@gmail.com> wrote:

> From: Nadav Amit <namit@vmware.com>
> 
> Functions that are marked as "inline" are currently also not tracable.
> This limits tracing functionality for many functions for no reason.
> Apparently, this has been done for two reasons.
> 
> First, as described in commit 5963e317b1e9d2a ("ftrace/x86: Do not
> change stacks in DEBUG when calling lockdep"), it was intended to
> prevent some functions that cannot be traced from being traced as these
> functions were marked as inline (among others).
> 
> Yet, this change has been done a decade ago, and according to Steven
> Rostedt, ftrace should have improved and hopefully resolved nested
> tracing issues by now. Arguably, if functions that should be traced -
> for instance since they are used during tracing - still exist, they
> should be marked as notrace explicitly.
> 
> The second reason, which Steven raised, is that attaching "notrace" to
> "inline" prevented tracing differences between different configs, which
> caused various problem. This consideration is not very strong, and tying
> "inline" and "notrace" does not seem very beneficial. The "inline"
> keyword is just a hint, and many functions are currently not tracable
> due to this reason.
> 
> Disconnect "inline" from "notrace".

FYI, I have a patch queued (still needs to go through testing) that
already does this ;-)

https://lore.kernel.org/all/20230502164102.1a51cdb4@gandalf.local.home/

-- Steve
Nadav Amit May 26, 2023, 5:17 a.m. UTC | #2 
> On May 25, 2023, at 7:28 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 25 May 2023 14:00:39 -0700
> Nadav Amit <nadav.amit@gmail.com> wrote:
> 
>> From: Nadav Amit <namit@vmware.com>
>> 
>> Functions that are marked as "inline" are currently also not tracable.
>> This limits tracing functionality for many functions for no reason.
>> Apparently, this has been done for two reasons.
>> 
>> First, as described in commit 5963e317b1e9d2a ("ftrace/x86: Do not
>> change stacks in DEBUG when calling lockdep"), it was intended to
>> prevent some functions that cannot be traced from being traced as these
>> functions were marked as inline (among others).
>> 
>> Yet, this change has been done a decade ago, and according to Steven
>> Rostedt, ftrace should have improved and hopefully resolved nested
>> tracing issues by now. Arguably, if functions that should be traced -
>> for instance since they are used during tracing - still exist, they
>> should be marked as notrace explicitly.
>> 
>> The second reason, which Steven raised, is that attaching "notrace" to
>> "inline" prevented tracing differences between different configs, which
>> caused various problem. This consideration is not very strong, and tying
>> "inline" and "notrace" does not seem very beneficial. The "inline"
>> keyword is just a hint, and many functions are currently not tracable
>> due to this reason.
>> 
>> Disconnect "inline" from "notrace".
> 
> FYI, I have a patch queued (still needs to go through testing) that
> already does this ;-)
> 
> https://lore.kernel.org/all/20230502164102.1a51cdb4@gandalf.local.home/

Ugh. If you cc’d me, I wouldn’t bother you during your vacation. :)

I think you may like the first patch in my series to precede this patch
though as some of the function I marked as “notrace" are currently “inline”.

Let me know how you want to proceed, so I would know how to break this
series.
Steven Rostedt May 26, 2023, 5:35 a.m. UTC | #3 
On Thu, 25 May 2023 22:17:33 -0700
Nadav Amit <nadav.amit@gmail.com> wrote:


> > FYI, I have a patch queued (still needs to go through testing) that
> > already does this ;-)
> > 
> > https://lore.kernel.org/all/20230502164102.1a51cdb4@gandalf.local.home/  
> 
> Ugh. If you cc’d me, I wouldn’t bother you during your vacation. :)

I'm currently passed the vacation part and now in Taiwan for work.

> 
> I think you may like the first patch in my series to precede this patch
> though as some of the function I marked as “notrace" are currently “inline”.
> 
> Let me know how you want to proceed, so I would know how to break this
> series.

Currently there's a nasty bug in v6.4-rc3 I'm fighting where I can't
proceed on anything until it's resolved. But I could also just pull
your first and third patch too. I'll let you know when I'm finished
debugging.

-- Steve
Steven Rostedt May 26, 2023, 5:40 a.m. UTC | #4 
On Thu, 25 May 2023 22:17:33 -0700
Nadav Amit <nadav.amit@gmail.com> wrote:

> Ugh. If you cc’d me, I wouldn’t bother you during your vacation. :)

Oh, and if you are interested in tracing patches, just subscribe to
linux-trace-kernel@vger.kernel.org.

-- Steve
diff mbox series

Patch

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 547ea1ff806e..bab3e25bbe3f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -184,7 +184,7 @@  struct ftrace_likely_data {
  * of extern inline functions at link time.
  * A lot of inline functions can cause havoc with function tracing.
  */
-#define inline inline __gnu_inline __inline_maybe_unused notrace
+#define inline inline __gnu_inline __inline_maybe_unused
 
 /*
  * gcc provides both __inline__ and __inline as alternate spellings of