PPing loader improvements #31
Conversation
|
Looks mostly good, just a few minor comments:
|
Ah, good to know. Will check it out. And yes, in the current state (as of these commits) I think it was mainly to load time I was concerned about as the verifier took a noticeable amount of time verifying the programs. In general I guess it's also a bit unnecessary to load programs that aren't used, even if it doesn't cause much of an issue.
If I remember it correctly, it was a bit of an arbitrarily picked value that was high enough that it should not put any limit on what the user could effectively do but low enough that it wouldn't cause an overflow. Using 0 to disable cleanup seems like a good idea, right now I think passing 0 for the cleanup period simply ends up running the cleanup in a busy loop. |
simosund
force-pushed
the
pping_loader_improvements
branch
from
a5be394 to
e88aae4
Compare
|
Hi @tohojo, I have updated the commit that introduced tc/ingress as an alternative to XDP to make use of the I have also updated the commit which adds bounds to the float arguments to have an upper limit of one week (instead of 1 billion ms or s) and disable the periodical cleanup in case the value 0 is passed. So feel free to take another look and let me know if there's anything else that can be improved. |
simosund
force-pushed
the
pping_loader_improvements
branch
from
e88aae4 to
27b07f4
Compare
pping/pping.c
Outdated
| if (snprintf(prog_path, sizeof(prog_path), "%s/%s", pin_dir, section) < 0) | ||
| return -EINVAL; | ||
| prog_fd = bpf_program__fd( | ||
| bpf_object__find_program_by_title(obj, sec)); |
There was a problem hiding this comment.
The bpf_object__find_program_by_title() API is being deprecated entirely, so we should probably switch this over to bpf_object__find_program_by_name() instead. See https://lore.kernel.org/r/20211214035931.1148209-1-kuifeng@fb.com
There was a problem hiding this comment.
Ok, wasn't aware of that, but then it makes sense to fix that. I have however gone on vacation now so might take a little while until I get around to it.
There was a problem hiding this comment.
Hi @tohojo, just wanted to notify you that I've now changed all instances of bpf_object__find_program_by_title() to bpf_object__find_program_by_name(). As I already had several instances of bpf_object__find_program_by_title() before the commits in this PR I decided to simply add this change as a separate commit.
pping/pping_kern.c
Outdated
| @@ -405,7 +405,7 @@ static void pping_ingress(void *ctx, struct parsing_context *pctx) | |||
| // Programs | |||
|
|
|||
| // Egress path using TC-BPF | |||
| SEC(SEC_EGRESS_TC) | |||
| SEC("classifier/egress") | |||
There was a problem hiding this comment.
These should just be SEC("tc") as that's the New And Improved(tm) way of doing this; see: https://lore.kernel.org/bpf/270e27b1-e5be-5b1c-b343-51bd644d0747@iogearbox.net/
(and yes, you can have two programs with the same section name, that's the point of moving to function name-based lookup :) )
There was a problem hiding this comment.
Hmm, as of right now though changing to SEC("tc") causes the program to fail loading with error:
libbpf: prog 'pping_tc_egress': missing BPF prog type, check ELF section name 'tc'
i.e. libbpf is unable to automatically infer the correct program type from the section name. Not sure if that's something that has changed in a newer version of libbpf (i.e. if you updating the libbpf submodule would fix it).
I could of course explicitly set the program type of each program in the loader, but think it's rather convenient that libbpf automatically infers it. So do you think relying on libbpf to infer the program type is a bad idea and that I should change it to SEC("tc") and then explicitly set the program type in the loader (in which case I guess I should also do that with the XDP program to be consistent), or should I leave it be for now?
|
Ah yes, you're right, we should update the libbpf version. Just pushed
that change, so try rebasing and see if that helps. Be warned that you
may get some other deprecation errors as well,
bpf_object__get_program_by_title() was not the only function that was deprecated...
|
Add cleanup code to load_attach_bpfprogs function, so it should always unpin tc program, and additionally detach the tc and xdp programs in case of any failure. Also unpin tc program directly after attaching it (rather than on program shutdown), so that multiple instances of pping can be run simultaneously (on different interfaces). Finally, rename some of the functions for attaching/detaching tc programs to be more consistent with the xdp ones. Note: Still need to keep a copy of most of the cleanup code in main as well, as the tc and xdp programs also need to be detached on program shutdown or if later functions fail. Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
libbpf v0.4 added an API for attaching/detaching TC-BPF programs. So use the new API to attach the tc program instead of calling on an external script (which uses the tc command line utility). Avoid removing the clsact qdisc on program shutdown or error, as there's currently no convenient way to ensure the qdisc isn't used by other programs as well. This means pping will not completely clean up after itself, but this is a safer alternative than always destroying the qdsic as done by the external script, which may pull the rug out underneath other programs using the qdisc. Finally, remove the pin_dir member from the configuration as pping no longer pins any programs or maps, and remove deleted tc loading scripts from README. Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
For some machines, XDP may not be suitable due to ex. lack of XDP support in NIC drivers or another program already being attached to the XDP hook on the desired interface. Therefore, add an option to use the tc-ingress hook instead of XDP to attach the pping ingress BPF program on. In practice, this adds an additional BPF program to the object file (a TC ingress program). To avoid loading an unnecessary BPF program, also explicitly disable autoloading for the ingress program not selected. Also, change the tc programs to return TC_ACT_OK instead of BPF_OK. While both should be compatible, the TC_ACT_* return codes seem to be more commonly used for TC-BPF programs. Concerns with this commit: - The error messages for XDP attach failure has gotten slightly less descriptive. I plan to improve the code for attaching and detaching XDP programs in a separate commit, and will then address that. Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
Make several changes to functions related to attaching and detaching the BPF programs: - Check the BPF program id when detaching programs to ensure that the correct programs are removed. - When attaching tc-programs, keep track of if the clsact qdisc was created or existed previously. Attempt to delete the qdisc if it was created and attaching failed. If the --force argument was given, also attempt to delete qdisc on shutdown in case it did not previously exist. - Rely on XDP flags to replace existing XDP program if --force is used rather than explicitly detaching any XDP program first. - Print out hints for why pping might have failed attaching the XDP program. Also, use libbpf_strerror instead of strerror to better display libbpf-specific error codes, and for more reliable error handling in general (don't need to ensure the error codes are positive). Finally, change return codes of tc programs to TC_ACT_UNSPEC from TC_ACT_OK to allow other TC-BPF programs to be used on the same interface as pping. Concerns with this commit: - When attaching a tc program libbpf will emit a warning if the clsact qdisc already exists on the interface. The fact that the clsact already exists is not an issue, and is handled in tc_attach by checking for EEXIST, so the warning could be a bit misleading/confusing for the user. - The tc_attach and xdp_attach functions attempt to return the u32 prog_id in an int. In case the programs are assigned a very high id (> 2^31) this may cause it to be interpreted as an error instead. Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
The rate-limit and cleanup-interval arguments were only verified to be positive. Add a check for an upper bound to avoid user being able to pass values that result in an internal overflow. The limits for both rate-limit and cleanup-interval have been set to one week which should be more then enough for any reasonable user. Additionally, disable the period cleanup entirely if the value 0 is passed to cleanup-interval. Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
The libbpf API has deprecated a number of functions used by the pping loader. While a couple of functions have simply been renamed, bpf_object__find_program_by_title has been completely deprecated in favor of bpf_object__find_program_by_name. Therefore, change so that BPF programs are found based on the C function names rather than section names. Also remove defines of section names as they are no longer used, and change the section names in pping_kern.c to use "tc" instead of "classifier/ingress" and "classifier/egress". Finally replace the flags json_format and json_ppviz in pping_config with a single enum for the different output formats. This makes the logic for which output format to use clearer compared to relying on multiple (supposedly) mutually exclusive flags (and implicitly assuming standard format if neither flag was set). One potential concern with this commit is that it introduces some "magical strings". In case the function names in pping_kern.c are changed it will require multiple changes in pping.c. Signed-off-by: Simon Sundberg <simon.sundberg@kau.se>
simosund
force-pushed
the
pping_loader_improvements
branch
from
2e8da76 to
cfdf224
Compare
|
I've now rebased on the latest master (with the updated libbpf submodule) and think I've fixed all instances where some deprecated libbpf API was used (at least I no longer get any deprecation warnings when compiling). |
Several improvements and fixes to the loading process of the BPF programs, mainly focused on the loading of TC programs. Based on commits from PR#23.