libbpf-tools/profile: Add support for PID-namespacing #5152

ekyooo · 2024-11-21T04:01:39Z

Support PID namespace translation as memleak.py does.

eiffel-fl

I have few comments, but it is looking really good!

eiffel-fl · 2024-11-22T12:44:55Z

libbpf-tools/profile.c

@@ -595,6 +610,8 @@ int main(int argc, char **argv)
 				env.perf_max_stack_depth * sizeof(unsigned long));
 	bpf_map__set_max_entries(obj->maps.stackmap, env.stack_storage_size);

+	set_pidns(obj);


You can catch the returned value and show a warning message in case PID namespace does not exist?

fixed. thank you.

eiffel-fl · 2024-11-22T13:48:05Z

libbpf-tools/profile.c

+	if (kernel_at_least(5, 7))
+		return -EPERM;


The problem with fixed version check is that they will not work on kernel where a specific patch could have been backported.
Is there a way to rather check for the existence of a given bpf helper?

In bcc/tools, we have a few cases (including profile) checking kernel version for bpf_get_ns_current_pid_tgid() availability. But for libbpf-tools, I think we can do better.
See trace_helpers.c. There are a few probe_*() functions which tries to detect whether a particular feature (prog_type, map_type) is supported or not. I think we can have another probe function to check whether bpf_get_ns_current_pid_tgid() is supported or not.

Thank you for good feedback.
I have applied the opinion.

@eiffel-fl
Could you please check if it's what you want?

Thank you.

yonghong-song · 2024-11-23T18:10:56Z

libbpf-tools/profile.c

@@ -595,6 +631,10 @@ int main(int argc, char **argv)
 				env.perf_max_stack_depth * sizeof(unsigned long));
 	bpf_map__set_max_entries(obj->maps.stackmap, env.stack_storage_size);

+	err = set_pidns(obj);
+	if (err)


Let us do 'err && env.verbose' here. We want to print out the below information under verbose mode.

it's modified. thank you.

yonghong-song · 2024-11-23T18:35:32Z

libbpf-tools/profile.c

+	if (kernel_at_least(5, 7))
+		return -EPERM;


In bcc/tools, we have a few cases (including profile) checking kernel version for bpf_get_ns_current_pid_tgid() availability. But for libbpf-tools, I think we can do better.
See trace_helpers.c. There are a few probe_*() functions which tries to detect whether a particular feature (prog_type, map_type) is supported or not. I think we can have another probe function to check whether bpf_get_ns_current_pid_tgid() is supported or not.

eiffel-fl

Hi,

Some other comments, it is looking great though!
I will test it later.

Best regards.

eiffel-fl · 2024-12-06T07:51:35Z

libbpf-tools/trace_helpers.c

+		{ .code = BPF_JMP | BPF_EXIT },
+	};
+
+	insn_cnt = sizeof(insns) / sizeof(struct bpf_insn);


Suggested change

insn_cnt = sizeof(insns) / sizeof(struct bpf_insn);

insn_cnt = sizeof(insns) / sizeof(insns[0]);

This way, you do not have problem if one day we need to change the type.

it's modified. Thank you.

eiffel-fl · 2024-12-06T07:52:43Z

libbpf-tools/profile.bpf.c

+	struct bpf_pidns_info ns = {};
+
+	if (use_pidns && !bpf_get_ns_current_pid_tgid(pidns_dev, pidns_ino, &ns,
+						      sizeof(struct bpf_pidns_info))) {


Use ns for sizeof instead of the type name?

It's modified. Thank you.

eiffel-fl · 2024-12-06T07:54:14Z

libbpf-tools/trace_helpers.c

@@ -1248,6 +1248,28 @@ bool probe_ringbuf()
 	return true;
 }

+bool probe_bpf_ns_current_pid_tgid(void)


I would first add a commit introducing this helper, then using it in another commit in profile.c.
What do you think?

It's done. thank you.

Add PID translation for nested PID namespace environments, such as container.

…d_tgid Add a utility function to check if bpf_get_ns_current_pid_tgid is available in the target kernel.

…he kernel Support PID namespace mapping only in kernels where bpf_get_ns_current_pid_tgid is available.

eiffel-fl

LGTM! Thank you!

eiffel-fl · 2024-12-13T08:23:36Z

libbpf-tools/profile.bpf.c

 	u64 *valp;
 	static const u64 zero;
 	struct key_t key = {};
+	u64 id;


Nit: id scope can be reduced to else block.

Yes, I prefer that style, but as far as I know, bcc/libbpf-tools follows the 'declaration first' rule.
Thank you for good opinion.

ekyooo · 2024-12-13T08:39:45Z

LGTM! Thank you!

Thank you.
I wonder if the test went well in your environment as well.

eiffel-fl · 2024-12-13T17:20:34Z

LGTM! Thank you!

Thank you. I wonder if the test went well in your environment as well.

I am not really familiar with the CI, but I was able to compile and run the tool locally without problems.

ekyooo requested review from drzaeus77, goldshtn, yonghong-song, 4ast, brendangregg and davemarchevsky as code owners November 21, 2024 04:01

ekyooo mentioned this pull request Nov 21, 2024

libbpf-tools: add profile #3782

Merged

ekyooo force-pushed the profile_pidns branch from 4f415e1 to b0a1119 Compare November 21, 2024 09:28

eiffel-fl reviewed Nov 22, 2024

View reviewed changes

yonghong-song reviewed Nov 23, 2024

View reviewed changes

ekyooo force-pushed the profile_pidns branch 2 times, most recently from f9be357 to 405c48f Compare December 3, 2024 11:22

eiffel-fl reviewed Dec 6, 2024

View reviewed changes

ekyooo added 3 commits December 13, 2024 14:48

libbpf-tools/profile: Add support for PID-namespacing

719f4b2

Add PID translation for nested PID namespace environments, such as container.

libbpf-tools/trace_helpers: Add helper to probe bpf_get_ns_current_pi…

257b04c

…d_tgid Add a utility function to check if bpf_get_ns_current_pid_tgid is available in the target kernel.

libbpf_tools/profile: Support PID namespace mapping if available in t…

1b40174

…he kernel Support PID namespace mapping only in kernels where bpf_get_ns_current_pid_tgid is available.

ekyooo force-pushed the profile_pidns branch from 405c48f to 1b40174 Compare December 13, 2024 05:56

eiffel-fl approved these changes Dec 13, 2024

View reviewed changes

yonghong-song merged commit 04dbccd into iovisor:master Dec 18, 2024
8 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libbpf-tools/profile: Add support for PID-namespacing #5152

libbpf-tools/profile: Add support for PID-namespacing #5152

ekyooo commented Nov 21, 2024

eiffel-fl left a comment

eiffel-fl Nov 22, 2024

ekyooo Dec 3, 2024

eiffel-fl Nov 22, 2024

yonghong-song Nov 23, 2024

ekyooo Dec 3, 2024

yonghong-song Nov 23, 2024

ekyooo Dec 3, 2024

yonghong-song Nov 23, 2024

eiffel-fl left a comment

eiffel-fl Dec 6, 2024

ekyooo Dec 13, 2024

eiffel-fl Dec 6, 2024

ekyooo Dec 13, 2024

eiffel-fl Dec 6, 2024

ekyooo Dec 13, 2024

eiffel-fl left a comment

eiffel-fl Dec 13, 2024

ekyooo Dec 13, 2024

ekyooo commented Dec 13, 2024

eiffel-fl commented Dec 13, 2024

	insn_cnt = sizeof(insns) / sizeof(struct bpf_insn);
	insn_cnt = sizeof(insns) / sizeof(insns[0]);

libbpf-tools/profile: Add support for PID-namespacing #5152

libbpf-tools/profile: Add support for PID-namespacing #5152

Conversation

ekyooo commented Nov 21, 2024

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekyooo commented Dec 13, 2024

eiffel-fl commented Dec 13, 2024