On Aug 7, 2024, at 3:08 AM, Masami Hiramatsu [off-list ref] wrote:

On Wed, 7 Aug 2024 00:19:20 +0000
Song Liu [off-list ref] wrote:

quoted

quoted

On Aug 6, 2024, at 5:01 PM, Masami Hiramatsu [off-list ref] wrote:

On Tue, 6 Aug 2024 20:12:55 +0000
Song Liu [off-list ref] wrote:

quoted

quoted

On Aug 6, 2024, at 1:01 PM, Steven Rostedt [off-list ref] wrote:

On Tue, 6 Aug 2024 16:00:49 -0400
Steven Rostedt [off-list ref] wrote:

quoted

quoted

quoted

quoted

+ if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr)
+ addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk));
+    

So you do the lookup twice if this is enabled?

Why not just use "kallsyms_lookup_name_without_suffix()" the entire time,
and it should work just the same as "kallsyms_lookup_name()" if it's not
needed?    

We still want to give priority to full match. For example, we have:

[root@~]# grep c_next /proc/kallsyms
ffffffff81419dc0 t c_next.llvm.7567888411731313343
ffffffff81680600 t c_next
ffffffff81854380 t c_next.llvm.14337844803752139461

If the goal is to explicitly trace c_next.llvm.7567888411731313343, the
user can provide the full name. If we always match _without_suffix, all
of the 3 will match to the first one. 

Does this make sense?  

Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked
like you did the command twice.

But that said, does this only have to be for llvm? Or should we do this for
even gcc? As I believe gcc can give strange symbols too.

I think most of the issue comes with LTO, as LTO promotes local static
functions to global functions. IIUC, we don't have GCC built, LTO enabled
kernel yet.

In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", 
and ".isra.0.cold". We didn't do anything about these before this set. So I 
think we are OK not handling them now. We sure can enable it for GCC built
kernel in the future.

Hmm, I think it should be handled as it is. This means it should do as
livepatch does. Since I expected user will check kallsyms if gets error,
we should keep this as it is. (if a symbol has suffix, it should accept
symbol with suffix, or user will get confused because they can not find
which symbol is kprobed.)

Sorry about the conclusion (so I NAK this), but this is a good discussion.

Do you mean we do not want patch 3/3, but would like to keep 1/3 and part 
of 2/3 (remove the _without_suffix APIs)? If this is the case, we are 
undoing the change by Sami in [1], and thus may break some tracing tools.

What tracing tools may be broke and why?

For this suffix problem, I would like to add another patch to allow probing on
suffixed symbols. (It seems suffixed symbols are not available at this point)

The problem is that the suffixed symbols maybe a "part" of the original function,
thus user has to carefully use it.

It appears there are multiple APIs that may need change. For example, on gcc
built kernel, /sys/kernel/tracing/available_filter_functions does not show 
the suffix: 

  [root@(none)]# grep cmos_irq_enable /proc/kallsyms
  ffffffff81db5470 t __pfx_cmos_irq_enable.constprop.0
  ffffffff81db5480 t cmos_irq_enable.constprop.0
  ffffffff822dec6e t cmos_irq_enable.constprop.0.cold

  [root@(none)]# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions
  cmos_irq_enable

perf-probe uses _text+<offset> for such cases:

  [root@(none)]# cat /sys/kernel/tracing/kprobe_events
  p:probe/cmos_irq_enable _text+14374016 

I am not sure which APIs do we need to change here. 

Thanks,
Song