link: add maxactive for kretprobe #755

alahaiyo · 2022-08-03T04:00:34Z

In some case we want to missed as little as possible and
it is helpful to have this configuration option.

lmb

Thank you for your PR!

Can you explain your use case for maxactive and why implementing your own sampling in BPF isn't sufficient?

As it stands, the PR is too special cased for me:

doesn't work with pmu probes
only supported for retprobes (?)

Kprobes have a large API, and supporting all of it is not possible. So we need a very compelling reason to add something like your proposed change.

alahaiyo · 2022-08-03T13:04:22Z

Thank you for your PR!

Can you explain your use case for maxactive and why implementing your own sampling in BPF isn't sufficient?

As it stands, the PR is too special cased for me:

doesn't work with pmu probes

only supported for retprobes (?)

Kprobes have a large API, and supporting all of it is not possible. So we need a very compelling reason to add something like your proposed change.

If the traced function can sleep, then when the number of processes exceeds the number of CPUs, the kretprobe will not be triggered. Because the kernel defaults maxactive to NR_CPU. When we trace some kernel network functions (such as inet_accept), when the network connection pressure is very high, there will be a lot of kretprobe not triggered.

When writing kprobe_event through tracefs, you can set maxactive to a maximum of 2048, which is enough. The implementation of pmu probe in the kernel will call create_local_trace_kprobe, in which maxactive is hardcoded to 0.

lmb · 2022-08-04T10:19:28Z

Thanks for the background, that's interesting.

So what is the behaviour of a pmu retprobe in the same scenario? It drops retprobe calls, without being able to work around the limitation? Asked another way, what does it mean that maxactive is 0 in the pmu case?

What problem does maxactive guard against in the first place? Maybe we could just hardcode it to 2048 in the tracefs case?

Cc @mmat11

mmat11 · 2022-08-04T13:03:28Z

It drops retprobe calls, without being able to work around the limitation? Asked another way, what does it mean that maxactive is 0 in the pmu case?

I believe this is the case

What problem does maxactive guard against in the first place?

I found an explanation here iovisor/bcc#1072 (comment)

Maybe we could just hardcode it to 2048 in the tracefs case?

From this commit torvalds/linux@696ced4 it seems the maximum is 4096. I guess there's some overhead (see https://elixir.bootlin.com/linux/v5.19/source/kernel/kprobes.c#L2202) in having a big number, else it could have been hardcoded in the kernel itself (?).

I wonder why this parameter isn't configurable via perf_event_open.
IMO we can support the feature but we should raise an error when it's not possible to do so (kernel < 4.12 or maxactive > 0 on non-return probes) and clearly document that setting this option will force the probe to be installed via tracefs

nashuiliang · 2022-08-05T08:39:20Z

hi, @mmat11

I wonder why this parameter isn't configurable via perf_event_open.

Tow related commits(not merged):

https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/

Enable specifying maxactive for fd based kretprobe. This will be useful
for tracing tools like bcc and bpftrace. [1] discussed the need of this
in bpftrace. Use highest highest 12 bit (bit 52-63) to allow maximal
maxactive of 4095.

[1] https://github.com/iovisor/bpftrace/issues/835

https://lore.kernel.org/bpf/20220714193403.13211-1-9erthalion6@gmail.com/

From: Song Liu <songliubraving@fb.com>

Enable specifying maxactive for fd based kretprobe. This will be useful
for tracing tools like bcc and bpftrace (see for example discussion [1]).
Use highest 4 bit (bit 59-63) to allow specifying maxactive by log2.

The original patch [2] seems to be fallen through the cracks and wasn't
applied. I've merely rebased the work done by Song Liu, verififed it
still works, and modified to allow specifying maxactive by log2 per
suggestion from the discussion thread.

Note that changes in rethook implementation may render maxactive
obsolete.

[1]: https://github.com/iovisor/bpftrace/issues/835
[2]: https://lore.kernel.org/all/20191007223111.1142454-1-songliubraving@fb.com/

It seems to be discussing a more elegant way to implement.

IMO we can support the feature but we should raise an error when it's not possible to do so (kernel < 4.12 or maxactive > 0 on non-return probes) and clearly document that setting this option will force the probe to be installed via tracefs

Agree, I also expect this feature.

alahaiyo · 2022-08-08T08:09:54Z

Thank you for your reply！

It drops retprobe calls, without being able to work around the limitation? Asked another way, what does it mean that maxactive is 0 in the pmu case?

I believe this is the case

Agree

What problem does maxactive guard against in the first place?

I found an explanation here iovisor/bcc#1072 (comment)

Maybe we could just hardcode it to 2048 in the tracefs case?

From this commit torvalds/linux@696ced4 it seems the maximum is 4096.

Oh my mistake, the maximum is 4096.

I guess there's some overhead (see https://elixir.bootlin.com/linux/v5.19/source/kernel/kprobes.c#L2202) in having a big number, else it could have been hardcoded in the kernel itself (?).

I wonder why this parameter isn't configurable via perf_event_open.

As @nashuiliang said, the feature is under discussion now.

IMO we can support the feature but we should raise an error when it's not possible to do so (kernel < 4.12 or maxactive > 0 on non-return probes) and clearly document that setting this option will force the probe to be installed via tracefs

Yes, I agree. I will add this and commit again.

lmb · 2022-08-08T09:01:45Z

@alahaiyo I'm not sure yet what the best approach is here, so there is a risk that your work might be wasted. I'll read through all the info here and try to reach an opinion today or tomorrow.

alahaiyo · 2022-08-08T11:50:04Z

@alahaiyo I'm not sure yet what the best approach is here, so there is a risk that your work might be wasted. I'll read through all the info here and try to reach an opinion today or tomorrow.

I'm really looking forward to your opinion.

lmb · 2022-08-09T08:51:43Z

From the kernel side we have two separate APIs to create a kretprobe:

via tracefs: supports setting maxactive and missed events are reported via
/sys/kernel/debug/tracing/kprobe_profile.
via perf_event_open: supports neither maxactive nor missed events. Peter Zijlstra wants to avoid adding maxactive to p_e_o if possible. A patch to expose missed events has gotten stuck, but the maintainer seems open to it.

On the user space side, maxactive is exposed in bcc and gobpf. bpftrace hardcodes maxactive to 0 and would like to print the number of missed events instead.

Let's consider the implications of the maxactive API as proposed in the PR:

On pre 4.12 kernels the user will receive an error from the kernel, since the feature is not supported. So users have to avoid setting maxactive on old kernels. This requires checking kernel version or similar, or feature probing from the library to avoid setting maxactive.
On >= 4.12 kernels, we will force use of tracefs if maxactive!=0 even if pmu is available. tracefs behaves poorly when the tracer crashes.
If maxactive becomes available on pmu after all, we have to do feature detection to
fall back to tracefs for kernels that have pmu but don't have the maxactive API.
If maxactive never becomes available on pmu we are forever locked into tracefs for kretprobes, even if regular pmu kprobes don't require maxactive anymore.
It's completely unclear what users should set maxactive to, since it needs to account for all callers of a function on the whole system. So being able to set maxactive still doesn't guarantee that all retprobes fire.

All in all, maxactive seems like a bad API. I think bpftrace has it right with exposing the number of missed events instead. If we manage to resurrect the fd-based missed events patch we could provide a somewhat unified API across tracefs and pmu.

@alahaiyo in your PR description you say that you want to "miss as little as possible". Would it be acceptable to you if you instead had a way to see how many misses you had, and scaling your result accordingly?

If not, what value would you set maxactive to? How do you figure out whether the kretprobe needs maxactive in the first place? How do you plan on dealing with kernels < 4.12?

If just misses is not enough, we could add a higher level option like FunctionMaySleep bool or similar instead of Maxactive int. Behind the scenes the lib would try to figure out the best strategy, which for now would be setting maxactive=4096 and forcing tracefs. This is better than maxactive since it avoids exposing the low level implementation detail and gives us flexibility in the future. The downside is that it requires a lot more feature probing and magic in the library itself.

lmb · 2022-08-10T14:36:03Z

@alahaiyo do you notice the missed events on specific machines, or is your goal to reduce misses outright? If there is a problem with a specific machine, is CONFIG_PREEMPT enabled? What is the value of /sys/devices/system/cpu/possible?

lmb · 2022-08-11T09:39:16Z

From iovisor/bcc#2224 (comment):

For kernels without maxactive supports in debugfs, the error resolution is not straightforward. The kernel will still install a probe, but under a different name.

The issue is that < 4.12 kernels will treat the r50:... written to tracefs as the name of the probe. This will need to be handled if we end up adding maxactive after all.

alahaiyo · 2022-08-16T02:49:24Z

@alahaiyo do you notice the missed events on specific machines, or is your goal to reduce misses outright? If there is a problem with a specific machine, is CONFIG_PREEMPT enabled? What is the value of /sys/devices/system/cpu/possible?

It's not on specific machines. CONFIG_PREEMPT is not set and the value of /sys/devices/system/cpu/possible is 0-95.

alahaiyo · 2022-08-16T03:03:04Z

From iovisor/bcc#2224 (comment):

For kernels without maxactive supports in debugfs, the error resolution is not straightforward. The kernel will still install a probe, but under a different name.

The issue is that < 4.12 kernels will treat the r50:... written to tracefs as the name of the probe. This will need to be handled if we end up adding maxactive after all.

Yes, that is a good reference.

alahaiyo · 2022-08-16T12:45:23Z

On pre 4.12 kernels the user will receive an error from the kernel, since the feature is not supported. So users have to avoid setting maxactive on old kernels. This requires checking kernel version or similar, or feature probing from the library to avoid setting maxactive.

We can handle with that.

On >= 4.12 kernels, we will force use of tracefs if maxactive!=0 even if pmu is available. tracefs behaves poorly when the tracer crashes.

In the interface of link.Kprobe, users can ignore maxactive, so the default is 0 and pmu will be used. If the user does not want to missed events, then they can set maxactive by themselves, especially when the misses is unbearable.

If maxactive becomes available on pmu after all, we have to do feature detection to
fall back to tracefs for kernels that have pmu but don't have the maxactive API.
If maxactive never becomes available on pmu we are forever locked into tracefs for kretprobes, even if regular pmu kprobes don't require maxactive anymore.
It's completely unclear what users should set maxactive to, since it needs to account for all callers of a function on the whole system. So being able to set maxactive still doesn't guarantee that all retprobes fire.

Even setting maxactive to 4096 does not guarantee that events will not be missed, and if so we have no other way. But I think the user needs to be given the ability to adjust the possibility of event missed.

If not, what value would you set maxactive to? How do you figure out whether the kretprobe needs maxactive in the first place? How do you plan on dealing with kernels < 4.12?

In fact, I don't know how to set maxactive. In our case it was only found that there were kretprobe events missing and this was not acceptable. The solution to this problem is to increase maxactive to a large enough value.

we could add a higher level option like FunctionMaySleep bool or similar instead of Maxactive int

But I still think exposing maxactive is better than making the user think about whether the function might sleep. Maxactive is considered as an attribute of kretprobe, which can make it easier for users to find out whether the function of trace will have the problem of loss of kretprobe events.

lmb

Sorry for the long delay, wrapping my head around this issue has been tricky. I was hoping to find a better solution that exposing maxactive, but I'm drawing a blank.

I've left some comments for what I'd like you to change, mostly documentation and making sure we're refusing maxactive in as many cases as possible. Please also add tests that cover the maxactive API.

link/kprobe.go

alahaiyo · 2022-11-02T10:21:53Z

I'm glad that this PR can continue to push forward. I have made changes to the code based on the comments. @lmb

link/kprobe.go

lmb · 2022-12-09T14:33:29Z

@alahaiyo I think the issue I pointed out in #755 (comment) is not addressed yet?

This commit exposes kretprobe's maxactive parameter to the user. You can modify the limit on the number of concurrent events with maxactive. This feature is currently supported from kernel version 4.12. Using maxactive on kernels lower than version 4.12 will result in the creation of events with names that do not match expectations. Signed-off-by: daikunhai <daikunhai@didiglobal.com>

alahaiyo · 2022-12-13T02:55:36Z

I found that it already has a query for the event ID after the event is created. So I just need to determine if I need to recreate it. My new commit is pushed. @lmb

lmb

I like how you integrated the check for unsupported maxactive! In the interests of getting this merged soon I'll push a couple of commits that contain changes I would've asked you to make. There is only one functional change: I'm dropping the fallback to disable maxactive.

Please take a look at my commits and let me know if you are OK with the modifications. Please also update the tests as mentioned.

lmb · 2022-12-14T12:55:11Z

link/kprobe.go

-	pe := fmt.Sprintf("-:%s/%s", group, sanitizeSymbol(symbol))
+	var pe string
+	if lowKernel {
+		pe = fmt.Sprintf("-:kprobes/r_%s_0", symbol)


Can you explain how the transform from r4096:ebpf_XYZ/sym into kprobes/r_sym_0 works? I'd expect that the 4096 is interpreted as a part of the event name and so would show up in the mangled event name.

Why is this not sanitizing the symbol?

In the old kernel, if the second character is not a:(https://elixir.bootlin.com/linux/v4.10/source/kernel/trace/trace_kprobe.c#L638), the event will not be created temporarily. Then here(https://elixir.bootlin.com/linux/v4.10/source/kernel/trace/trace_kprobe.c#L713), the kernel will create an event named r_symbol_offset

Why is this not sanitizing the symbol?

So I think we can't sanitizing the symbol when kernel would use full symbol name to create event.
There is an example：
root@hecs-352404:~# echo "r1024:ebpf_abcdefg/_ib_umem_release __ib_umem_release" >> /sys/kernel/debug/tracing/kprobe_events
root@hecs-352404:~# cat /sys/kernel/debug/tracing/kprobe_events
r:kprobes/r___ib_umem_release_0 __ib_umem_release

I updated the comment to reflect this.

link/kprobe_test.go

link/kprobe.go

alahaiyo · 2022-12-15T12:47:37Z

I like how you integrated the check for unsupported maxactive! In the interests of getting this merged soon I'll push a couple of commits that contain changes I would've asked you to make. There is only one functional change: I'm dropping the fallback to disable maxactive.

Please take a look at my commits and let me know if you are OK with the modifications. Please also update the tests as mentioned.

I total OK with that. And I push my commits which deal with tests and a little bugfix.

Move the code that deals with various kernel quirks such as not getting ErrExist and maxactive not being supported into the function used to create a probe event.

We modified logic in createTraceFSProbeEvent, so that in any kernel can not create a event when it already exists.

When the kernel creates strange event names because it does not support maxactive, you need to use the full symbol name to remove it.

lmb · 2022-12-15T13:20:38Z

link/kprobe.go

+// args.retprobeMaxActive is used on non kprobe types. Returns ErrNotSupported if
+// the kernel is too old to support kretprobe maxactive.
+func createTraceFSProbeEvent(typ probeType, args probeArgs) (uint64, error) {
+	// Before attempting to create a trace event through tracefs,


@mmat11 does my refactor here make sense? I think it's nicer if createTraceFSProbeEvent encapsulates all the ugly bits of the tracefs interface.

lmb · 2022-12-15T13:22:52Z

deal with tests and a little bugfix.

A good catch. I've opted to return os.ErrExist from create... instead, that way we don't have to change the tests. I also included the symbol offset in the even name when removing, since that may be != 0.

lmb

@alahaiyo thanks for pushing this through, sorry it took so long!

lmb reviewed Aug 3, 2022

View reviewed changes

alahaiyo force-pushed the kretprobe_add_maxactive branch from fd4973a to 49a907d Compare August 3, 2022 10:10

alahaiyo closed this Aug 3, 2022

alahaiyo deleted the kretprobe_add_maxactive branch August 3, 2022 11:49

alahaiyo restored the kretprobe_add_maxactive branch August 3, 2022 11:50

alahaiyo reopened this Aug 3, 2022

lmb requested changes Aug 29, 2022

View reviewed changes

alahaiyo force-pushed the kretprobe_add_maxactive branch 2 times, most recently from a2a73bf to 118ae4a Compare November 2, 2022 10:01

alahaiyo requested a review from lmb November 2, 2022 10:10

lmb requested changes Dec 5, 2022

View reviewed changes

link/kprobe.go Outdated Show resolved Hide resolved

link/kprobe.go Outdated Show resolved Hide resolved

link/kprobe.go Outdated Show resolved Hide resolved

link/kprobe.go Show resolved Hide resolved

link/kprobe.go Outdated Show resolved Hide resolved

alahaiyo force-pushed the kretprobe_add_maxactive branch 2 times, most recently from 2642713 to a253f5f Compare December 12, 2022 12:39

alahaiyo force-pushed the kretprobe_add_maxactive branch from a253f5f to 4dd0416 Compare December 13, 2022 02:17

lmb reviewed Dec 14, 2022

View reviewed changes

alahaiyo force-pushed the kretprobe_add_maxactive branch 3 times, most recently from 6b48b81 to 9d835bb Compare December 15, 2022 11:36

lmb and others added 7 commits December 15, 2022 13:16

link: split closeTraceFSPerfEvent

b6c3c69

link: don't disable maxactive automatically

c7f3ea9

link: make createTraceFSProbeEvent deal with kernel quirks

81daeb7

Move the code that deals with various kernel quirks such as not getting ErrExist and maxactive not being supported into the function used to create a probe event.

link: let TestKretprobeMaxActive avalibe in old kernel

c365400

We modified logic in createTraceFSProbeEvent, so that in any kernel can not create a event when it already exists.

link: deal with the kernel symbol with multiple consecutive underscores

f6c979f

When the kernel creates strange event names because it does not support maxactive, you need to use the full symbol name to remove it.

link: account for symbol offset for probe event removal < 4.12

cfd3730

link: make MaxActive tests a bit nicer

f7c4707

lmb force-pushed the kretprobe_add_maxactive branch from d8bbf99 to 5041050 Compare December 15, 2022 13:16

link: return os.ErrExist from createTraceFSProbeEvent on old kernels

40c2c7a

lmb force-pushed the kretprobe_add_maxactive branch from 5041050 to 40c2c7a Compare December 15, 2022 13:19

lmb reviewed Dec 15, 2022

View reviewed changes

lmb approved these changes Dec 15, 2022

View reviewed changes

lmb merged commit 1e326d1 into cilium:master Dec 15, 2022

alahaiyo deleted the kretprobe_add_maxactive branch December 16, 2022 08:41

grcevski mentioned this pull request Jan 26, 2024

After 1 minute beyla doesn't generate webserver traces grafana/beyla#573

Closed

shkarface mentioned this pull request May 27, 2024

Add support for tracefs siderolabs/pkgs#963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

link: add maxactive for kretprobe #755

link: add maxactive for kretprobe #755

alahaiyo commented Aug 3, 2022

lmb left a comment

alahaiyo commented Aug 3, 2022

lmb commented Aug 4, 2022

mmat11 commented Aug 4, 2022

nashuiliang commented Aug 5, 2022

alahaiyo commented Aug 8, 2022

lmb commented Aug 8, 2022

alahaiyo commented Aug 8, 2022

lmb commented Aug 9, 2022

lmb commented Aug 10, 2022

lmb commented Aug 11, 2022

alahaiyo commented Aug 16, 2022

alahaiyo commented Aug 16, 2022

alahaiyo commented Aug 16, 2022

lmb left a comment

alahaiyo commented Nov 2, 2022

lmb commented Dec 9, 2022

alahaiyo commented Dec 13, 2022

lmb left a comment

lmb Dec 14, 2022

alahaiyo Dec 15, 2022

alahaiyo Dec 15, 2022

lmb Dec 15, 2022

alahaiyo commented Dec 15, 2022

lmb Dec 15, 2022

mmat11 Dec 15, 2022

lmb commented Dec 15, 2022

lmb left a comment

link: add maxactive for kretprobe #755

link: add maxactive for kretprobe #755

Conversation

alahaiyo commented Aug 3, 2022

lmb left a comment

Choose a reason for hiding this comment

alahaiyo commented Aug 3, 2022

lmb commented Aug 4, 2022

mmat11 commented Aug 4, 2022

nashuiliang commented Aug 5, 2022

alahaiyo commented Aug 8, 2022

lmb commented Aug 8, 2022

alahaiyo commented Aug 8, 2022

lmb commented Aug 9, 2022

lmb commented Aug 10, 2022

lmb commented Aug 11, 2022

alahaiyo commented Aug 16, 2022

alahaiyo commented Aug 16, 2022

alahaiyo commented Aug 16, 2022

lmb left a comment

Choose a reason for hiding this comment

alahaiyo commented Nov 2, 2022

lmb commented Dec 9, 2022

alahaiyo commented Dec 13, 2022

lmb left a comment

Choose a reason for hiding this comment

lmb Dec 14, 2022

Choose a reason for hiding this comment

alahaiyo Dec 15, 2022

Choose a reason for hiding this comment

alahaiyo Dec 15, 2022

Choose a reason for hiding this comment

lmb Dec 15, 2022

Choose a reason for hiding this comment

alahaiyo commented Dec 15, 2022

lmb Dec 15, 2022

Choose a reason for hiding this comment

mmat11 Dec 15, 2022

Choose a reason for hiding this comment

lmb commented Dec 15, 2022

lmb left a comment

Choose a reason for hiding this comment