Re: [PATCH bpf-next v3 1/7] samples: bpf: refactor hbm program with libbpf

On Tue, Nov 24, 2020 at 1:03 AM Daniel T. Lee [off-list ref] wrote:

This commit refactors the existing cgroup programs with libbpf
bpf loader. Since bpf_program__attach doesn't support cgroup program
attachment, this explicitly attaches cgroup bpf program with
bpf_program__attach_cgroup(bpf_prog, cg1).

Also, to change attach_type of bpf program, this uses libbpf's
bpf_program__set_expected_attach_type helper to switch EGRESS to
INGRESS. To keep bpf program attached to the cgroup hierarchy even
after the exit, this commit uses the BPF_LINK_PINNING to pin the link
attachment even after it is closed.

Besides, this program was broken due to the typo of BPF MAP definition.
But this commit solves the problem by fixing this from 'queue_stats' map
struct hvm_queue_stats -> hbm_queue_stats.

Fixes: 36b5d471135c ("selftests/bpf: samples/bpf: Split off legacy stuff from bpf_helpers.h")
Signed-off-by: Daniel T. Lee <redacted>
---
Changes in v2:
 - restore read_trace_pipe2
 - remove unnecessary return code and cgroup fd compare
 - add static at global variable and remove unused variable
 - change cgroup path with unified controller (/unified/)
 - add link pinning to prevent cleaning up on process exit

Changes in v3:
 - cleanup bpf_link, bpf_object and cgroup fd both on success and error
 - remove link NULL cleanup since __destroy() can handle
 - fix cgroup test on cgroup fd cleanup

 samples/bpf/.gitignore     |   3 +
 samples/bpf/Makefile       |   2 +-
 samples/bpf/do_hbm_test.sh |  32 +++++------
 samples/bpf/hbm.c          | 111 ++++++++++++++++++++-----------------
 samples/bpf/hbm_kern.h     |   2 +-
 5 files changed, 78 insertions(+), 72 deletions(-)

Thanks for the nice clean up! I've applied the series to bpf-next. If
Martin finds any other problems, those can be fixed in a follow up
patch(es). But see also below about find_program_by_title().

-       if (ret) {
-               printf("ERROR: bpf_prog_load_xattr failed for: %s\n", prog);
-               printf("  Output from verifier:\n%s\n------\n", bpf_log_buf);
-               ret = -1;
-       } else {
-               ret = map_fd;
+       bpf_prog = bpf_object__find_program_by_title(obj, "cgroup_skb/egress");

It would be good to avoid using find_program_by_title(), as it's going
to get deprecated and eventually removed. Looking up by section name
("title") is ambiguous now that libbpf supports many BPF programs per
same section. There is find_program_by_name() which looks program by
its C function name. I suggest using it.

+       if (!bpf_prog) {
+               printf("ERROR: finding a prog in obj file failed\n");
+               goto err;
+       }
+