Each Example does not need it's own vmlinux_505.h file by mimullin-bbry · Pull Request #144 · libbpf/libbpf-rs

mimullin-bbry · 2021-11-09T01:17:42Z

ISSUE: #143

Move vmlinux_505.h from each example into it's own directory at ./examples/vmlinux_header/vmlinux_505.h

Softlink each example's vmlinux.h to
examples/vmlinux_header/vmlinux_505.h

This will reduce the amount of disk space needed

Create a new file examples/vmlinux_header/vmlinux_514.h
This file was generated via bpftool on an Arch Linux machine running
Kernel 5.14.16
bpftool btf dump file /sys/kernel/btf/vmlinux format c > examples/vmlinux_header/vmlinux_514.h
Softlink this file to examples/runqslower/src/bpf/vmlinux_new.h

This new header file is required because in change
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/include/linux/sched.h?h=v5.14.17&id=2f064a59a11ff9bc22e52e9678bc601404c7cb34
the task_struct::state field was changed to __state

Fixup runqslower.bpf.c to use proper state vs __state depending on
kernel version

./examples/vmlinux_header/ Softlink each example's vmlinux.h to examples/vmlinux_header/vmlinux_505.h This will reduce the amount of disk space needed Create a new file examples/vmlinux_header/vmlinux_514.h This file was generated via bpftool on an Arch Linux machine running Kernel 5.14.16 bpftool btf dump file /sys/kernel/btf/vmlinux format c > examples/vmlinux_header/vmlinux_514.h Softlink this file to examples/runqslower/src/bpf/vmlinux_new.h This new header file is required because in change https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/include/linux/sched.h?h=v5.14.17&id=2f064a59a11ff9bc22e52e9678bc601404c7cb34 the task_struct::state field was changed to __state Fixup runqslower.bpf.c to use proper state vs __state depending on kernel version

DevasiaThomas · 2021-11-10T16:59:25Z

@mimullin-bbry Is there a runtime approach to deal with state vs __state in runqslower.bpf.c? Else one would have to compile on 2 kernels? not really CO-RE?

DevasiaThomas · 2021-11-10T17:10:47Z

@mimullin-bbry Can we do what @anakryiko did in his blogpost, the struct flavors part, for this one so that its co-re? If not, why?

mimullin-bbry · 2021-11-10T17:35:23Z

@DevasiaThomas I don't think so (eager to be schooled on this issue). What I'm reading in @anakryiko 's post is to solve runtime kernel decision making, but this issue is a compile time issue where task_struct::__state does not exist pre 5.14 and task_struct::state does not exist post 5.14.

Thus to compile runqslower using vmlinux_505.h the verifier will barf upon loading into a 5.14 kernel saying something along the lines of "symbol task_struct::state does not exist."

Since example code is intended to be run on the machine that built it, users trying to understand how to "do bpf" will have a pretty poor time with runqslower on Fedora 35, arch or any distro using a 5.14 kernel and beyond. (note: arch has another issue where their uapi is not up to date, but thats not a problem I can fix).

EDIT: Actually, after a re-read of the blogpost, I think you are correct that something better can be done. Let me give this some cycles.

DevasiaThomas · 2021-11-10T17:47:22Z

EDIT: Actually, after a re-read of the blogpost, I think you are correct that something better can be done. Let me give this some cycles.

@mimullin-bbry Yeah I'm hoping you can use a 5.14 vmlinux.h but define the older struct(for state) within the example's code as __pre_5_1_4 and do what @anakryiko did and compile it on 5.14 but also be able to run on pre 5.14. If that happens, everybody wins 🎉

…d-co-re/#dealing-with-kernel-version-and-configuration-differences

mimullin-bbry · 2021-11-10T18:31:57Z

I've updated to use the techniques in https://nakryiko.com/posts/bpf-portability-and-co-re/#dealing-with-kernel-version-and-configuration-differences

I have not however, tested on a pre 5.13 kernel. (will update this comment when that is complete).

Pushing pre-testing for potential feedback w.r.t. copying the 505 version of struct task_struct, and whether that is a direction the project wants to take.

EDIT:

Tested against Arch linux running Kernel:

Linux jup 5.14.16-arch1-1 #1 SMP PREEMPT Tue, 02 Nov 2021 22:22:59 +0000 x86_64 GNU/Linux

and Alma Linux running kernel:

Linux localhost.localdomain 4.18.0-305.25.1.el8_4.x86_64 #1 SMP Tue Nov 2 10:34:25 EDT 2021 x86_64 x86_64 x86_64 GNU/Linux

Both machines compile and run capable and runqslower successful.

Binaries were then copied from Alma to Arch and ran successful
Binaries compiled on Arch would not run on Alma due to glibc versioning issues (this is expected).

DevasiaThomas · 2021-11-10T20:27:17Z

Thats good news @mimullin-bbry . How did you deal with the compile time ifdefs for the task struct?

anakryiko · 2021-11-10T23:24:41Z

See https://github.com/iovisor/bcc/pull/3659/files for how it was done for runqslower tool, for example.

You might also find https://nakryiko.com/posts/bpf-core-reference-guide/#handling-incompatible-field-and-type-changes useful.

Either way, I'd rather have only one vmlinux.h version to reduce the size of the repo. So we can update to the latest vmlinux.h, but we should get rid of the old one and update examples accordingly.

anakryiko

let's put vmlinux_514.h under examples/ and create a symlink vmlinux.h -> vmlinux_514.h right there.

Also, I was expecting that search path to vmlinux.h would need to be changed somewhere either in build script or somewhere else. Don't see it here. What am I missing?

anakryiko · 2021-11-10T23:28:23Z


+static inline long get_task_state(struct task_struct *ts)
+{
+	if (LINUX_KERNEL_VERSION < KERNEL_VERSION(5, 14, 0)) {


see https://github.com/iovisor/bcc/pull/3659/files, there are better ways than using LINUX_KERNEL_VERSION check. Due to backporting, kernel version isn't always representative. CO-RE provides bpf_core_field_exists() check, we should use that here.

anakryiko · 2021-11-10T23:29:30Z

+	struct thread_info thread_info;
+	volatile long int state;
+	void *stack;
+	refcount_t usage;
+	unsigned int flags;
+	unsigned int ptrace;
+	struct llist_node wake_entry;
+	int on_cpu;
+	unsigned int cpu;


you don't need all the fields of task_struct, only few necessary fields can be defined. It's also better to leave such definitions close to BPF source code that's using this type. See https://nakryiko.com/posts/bpf-core-reference-guide/#handling-incompatible-field-and-type-changes for some more examples and details.

Also, I was expecting that search path to vmlinux.h would need to be changed somewhere either in build script or somewhere else. Don't see it here. What am I missing?

Not sure what you mean by this. I've moved vmlinux_514.h into the examples directory, and softlinked a vmlinux.h to it. I then softlink each examples vmlinux.h to the example/vmlinux.h file. The build scripts do not need to be aware of the vmlinux.h file.

# pwd && ls -l /code/single-vmlinux/examples/runqslower/src/bpf total 8 -rw-r--r-- 1 mm mm 2677 Nov 10 20:46 runqslower.bpf.c -rw-r--r-- 1 mm mm 232 Nov 8 19:00 runqslower.h lrwxrwxrwx 1 mm mm 18 Nov 10 19:15 vmlinux.h -> ../../../vmlinux.h

# pwd && ls -l /code/single-vmlinux/examples total 2448 drwxr-xr-x 3 mm mm 4096 Nov 8 19:00 capable drwxr-xr-x 3 mm mm 4096 Nov 8 19:00 query drwxr-xr-x 3 mm mm 4096 Nov 8 19:00 runqslower -rw-r--r-- 1 mm mm 2492078 Nov 8 19:12 vmlinux_514.h lrwxrwxrwx 1 mm mm 13 Nov 10 19:10 vmlinux.h -> vmlinux_514.h

I've added a double layer of indirection so that it's easy to replace vmlinux_514.h with a newer version sometime down the road. that way updating examples/vmlinux.h to point to say examples/vmlinux_601.h means we do not need to worry about each example's version of vmlinux.h

Q: Would you like vmlinux_514.h renamed to just vmlinux.h (thus vmlinux.h is the 'real' file)? This would conserve git space as a new version of vmlinux.h would just be a delta from the last version, rather than a delete of the old file and a create of a new file? Trade off is that it's harder to understand what kernel version vmlinux.h was created from.

Never mind, I forgot we do vmlinux.h symlinking in each example to avoid needing to specify extra -I to the compiler. Please keep vmlinux_514.h name and leave all the vmlinux.h as symlinks

you don't need all the fields of task_struct, only few necessary fields can be defined.

@anakryiko I didn't know you could do this neither do I know the concept behind this. The closest thing I know is in OOPs where you can assign a child object to variable of type parent and just refer to the fields the parent knows about. Is this only possible in bpf CO-RE code or can you do this in regular C as well? I would love to read about this concept if its possible in regular C.

It's BPF CO-RE thing. Only fields that are accessed are necessary (to let compiler generate CO-RE relocation that will be used by libbpf to find corresponding type and field in the full kernel BTF definition).

move the 'old' task struct into runqslower.bpf.c and slim it down to only give us what we need

mimullin-bbry · 2021-11-11T02:24:03Z

Closing as I had problems squashing and merging. Please see #149

DevasiaThomas mentioned this pull request Nov 10, 2021

All Examples should use the same vmlinux.h file #143

Closed

use techniques found in https://nakryiko.com/posts/bpf-portability-an…

70d25d4

…d-co-re/#dealing-with-kernel-version-and-configuration-differences

fix spaces/tabs

2078760

anakryiko requested changes Nov 10, 2021

View reviewed changes

mimullin-bbry added 3 commits November 10, 2021 20:47

Use bpf_core_field_exists instead of LINUX_KERNEL_VERSION

82afe44

move the 'old' task struct into runqslower.bpf.c and slim it down to only give us what we need

Use bpf_core_field_exists instead of LINUX_KERNEL_VERSION

b8aacd5

move the 'old' task struct into runqslower.bpf.c and slim it down to only give us what we need

Merge branch 'libbpf:master' into single-vmlinux

132f804

mimullin-bbry closed this Nov 11, 2021

mimullin-bbry deleted the single-vmlinux branch December 7, 2021 18:52

Conversation

mimullin-bbry commented Nov 9, 2021

Uh oh!

DevasiaThomas commented Nov 10, 2021

Uh oh!

DevasiaThomas commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimullin-bbry commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DevasiaThomas commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimullin-bbry commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DevasiaThomas commented Nov 10, 2021

Uh oh!

anakryiko commented Nov 10, 2021

Uh oh!

anakryiko left a comment

Choose a reason for hiding this comment

Uh oh!

anakryiko Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

anakryiko Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

mimullin-bbry Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anakryiko Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

DevasiaThomas Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anakryiko Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

mimullin-bbry commented Nov 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DevasiaThomas commented Nov 10, 2021 •

edited

Loading

mimullin-bbry commented Nov 10, 2021 •

edited

Loading

DevasiaThomas commented Nov 10, 2021 •

edited

Loading

mimullin-bbry commented Nov 10, 2021 •

edited

Loading

mimullin-bbry Nov 11, 2021 •

edited

Loading

DevasiaThomas Nov 11, 2021 •

edited

Loading