Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] freplace removal #476

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[RFC] freplace removal #476

wants to merge 10 commits into from

Conversation

atenart
Copy link
Contributor

@atenart atenart commented Jan 21, 2025

We discussed removing the use of freplace and instead statically compile hooks into the probes. This is based on top of #464 and the last patch only is about $topic. The goal is only to show what this change could look like.

Notes:

  • The Hook object containing the hooks' instructions was repurposed to be an enum about which hooks to enable.
  • The ovs collector wasn't converted.
  • The USDT part wasn't converted. I'm thinking this part could actually be done a bit differently as there is no point of having common hooks shared between collectors. Instead a probe could be build per-collector.

Overall this requires a bit more maintenance but it doesn't look that bad.

Thoughts?

This is more logical and allows adding collectors to retis/src/collect
in later patches.

Signed-off-by: Antoine Tenart <[email protected]>
This is only moving patches, not changing anything besides the import
mod paths. Following patches will further remove the module concept.

Signed-off-by: Antoine Tenart <[email protected]>
Instead directly use collectors. In details this removes the ModuleId,
Module trait, Modules abstraction and a lot of the dynamic steps that
used to happen when using modules.

Following-up patches are needed to complete to make things completely
clean but those are not strictly about removing the module concept.

Signed-off-by: Antoine Tenart <[email protected]>
This also unties the SectionId <> collector cli arguments relationship.

Signed-off-by: Antoine Tenart <[email protected]>
There is no remaining user after collectors stopped from using it.

Signed-off-by: Antoine Tenart <[email protected]>
Using a more logical order, grouping related parameters and trying to
display then in a logical order (of importance).

Signed-off-by: Antoine Tenart <[email protected]>
Convert hooks to be statically built in the probes.

Signed-off-by: Antoine Tenart <[email protected]>
@atenart
Copy link
Contributor Author

atenart commented Jan 23, 2025

I added an initial (incomplete, eg. retval in fexit) support for fentry/fexit to show what it'll likely look like.

The main advantage is fexit probes have access the to function's input parameters, which helps not requiring the extra dance we have in the kretprobe implementation. Other than that kprobe is superior to fentry for us as it supports "loading once attaching to many", and (I have local patches for that) support for attaching multiple probes at once (which speed up things greatly). I saw some activity to improve fentry in that regard upstream but that isn't ready yet and I'm not sure what will be the extend of the improvements.

As a summary, with those changes, the preferred probe types in Retis would be:

  • raw_tracepoint for key probes that must be stable over time, unless
  • they are marked as noinline in which case kprobe is superior
  • fexit for working on the returned values
  • kprobe for all other, including user defined, probes

@amorenoz
Copy link
Contributor

amorenoz commented Feb 5, 2025

Just for reference, we've discussed this and there is a potential problem with this approach:

Originally, filter code was freplaced. This is the clean and elegant approach of injecting the filter code. However, in order to support older kernels (RHEL 8.4) that did not support pointer arguments, we moved to the current approach that intercepts the program registration and modifies the program on the fly.

LIBBPF_API int libbpf_register_prog_handler (const char *sec, enum bpf_prog_type prog_type, enum bpf_attach_type exp_attach_type, const struct libbpf_prog_handler_opts *opts)

This API was introduced because perf needed it and has received some pushback. Perf no longer needs it and there is a chance it gets deprecated, in which case we would be left with:

  • Manually modify the programs in rust (before libbpf).
  • Going back to freplace and not support native filtering fentry/fexit. Fentry could be replaced by kprobes and fexits could use the trick we do for kretprobes then what's the point?

@atenart
Copy link
Contributor Author

atenart commented Feb 6, 2025

Le'ts also note (as per my understanding) having fexit support is desired for multiple reasons (easy access to fct arguments, filtering, etc).

u8 nft;
} __binding hooks = {};

static __always_inline int call_hooks(struct retis_context *ctx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should be fine if this turns from __always_inline to __noinline keeping it static.
The advantage would be not sharing the stack with the main prog (512 bytes are not that much).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, and probably better, we could avoid inlining in DEFINE_HOOK().

@@ -23,6 +24,8 @@ impl EventFmt for KernelEvent {
"raw_tracepoint" => "tp",
"kprobe" => "k",
"kretprobe" => "kr",
"fentry" => "f",
"fexit" => "fe",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe fx for this one?
fe is common between fentry and fexit :)

@vlrpl
Copy link
Contributor

vlrpl commented Mar 20, 2025

just a general comment here.
I was thinking, assuming we want to explore different approaches, that this is where libbpf linker could help (Linker in libbpf-rs).
An idea could be having our main programs normally calling __weak functions and relink the ones we intend to include.
Basically, this simply moves relinking. The only difference would be precedence (but nothing prevents us from enforcing it if needed).
Ideally filtering would use this as well, but ELF generation is required and we don't have it (but even in this case, nothing prevents us from implementing it, just needs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants