| Message ID | 20201117145644.1166255-3-danieltimlee@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: remove bpf_load loader completely | expand |
On Tue, Nov 17, 2020 at 02:56:37PM +0000, Daniel T. Lee wrote: [ ... ] > diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c > index b9f9f771dd81..008bc635ad9b 100644 > --- a/samples/bpf/hbm.c > +++ b/samples/bpf/hbm.c > @@ -46,7 +46,6 @@ > #include <bpf/bpf.h> > #include <getopt.h> > > -#include "bpf_load.h" > #include "bpf_rlimit.h" > #include "trace_helpers.h" > #include "cgroup_helpers.h" > @@ -68,9 +67,10 @@ bool edt_flag; > static void Usage(void); > static void do_error(char *msg, bool errno_flag); > > +struct bpf_program *bpf_prog; > struct bpf_object *obj; > -int bpfprog_fd; > int cgroup_storage_fd; > +int queue_stats_fd; > > static void do_error(char *msg, bool errno_flag) > { > @@ -83,56 +83,54 @@ static void do_error(char *msg, bool errno_flag) > > static int prog_load(char *prog) > { > - struct bpf_prog_load_attr prog_load_attr = { > - .prog_type = BPF_PROG_TYPE_CGROUP_SKB, > - .file = prog, > - .expected_attach_type = BPF_CGROUP_INET_EGRESS, > - }; > - int map_fd; > - struct bpf_map *map; > + int rc = 1; > > - int ret = 0; > + obj = bpf_object__open_file(prog, NULL); > + if (libbpf_get_error(obj)) { > + printf("ERROR: opening BPF object file failed\n"); > + return rc; > + } > > - if (access(prog, O_RDONLY) < 0) { > - printf("Error accessing file %s: %s\n", prog, strerror(errno)); > - return 1; > + /* load BPF program */ > + if (bpf_object__load(obj)) { > + printf("ERROR: loading BPF object file failed\n"); > + goto cleanup; > } > - if (bpf_prog_load_xattr(&prog_load_attr, &obj, &bpfprog_fd)) > - ret = 1; > - if (!ret) { > - map = bpf_object__find_map_by_name(obj, "queue_stats"); > - map_fd = bpf_map__fd(map); > - if (map_fd < 0) { > - printf("Map not found: %s\n", strerror(map_fd)); > - ret = 1; > - } > + > + bpf_prog = bpf_object__find_program_by_title(obj, "cgroup_skb/egress"); > + if (!bpf_prog) { > + printf("ERROR: finding a prog in obj file failed\n"); > + goto cleanup; > } > > - 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; > + queue_stats_fd = bpf_object__find_map_fd_by_name(obj, "queue_stats"); > + if (queue_stats_fd < 0) { > + printf("ERROR: finding a map in obj file failed\n"); > + goto cleanup; > } > > - return ret; > + rc = 0; Just return 0. > + > +cleanup: > + if (rc != 0) so this test can be avoided. > + bpf_object__close(obj); > + > + return rc; > } > > static int run_bpf_prog(char *prog, int cg_id) > { > - int map_fd; > - int rc = 0; > + struct hbm_queue_stats qstats = {0}; > + struct bpf_link *link = NULL; > + char cg_dir[100]; > int key = 0; > int cg1 = 0; > - int type = BPF_CGROUP_INET_EGRESS; > - char cg_dir[100]; > - struct hbm_queue_stats qstats = {0}; > + int rc = 0; > > sprintf(cg_dir, "/hbm%d", cg_id); > - map_fd = prog_load(prog); > - if (map_fd == -1) > - return 1; > + rc = prog_load(prog); > + if (rc != 0) > + return rc; > > if (setup_cgroup_environment()) { > printf("ERROR: setting cgroup environment\n"); > @@ -152,16 +150,18 @@ static int run_bpf_prog(char *prog, int cg_id) > qstats.stats = stats_flag ? 1 : 0; > qstats.loopback = loopback_flag ? 1 : 0; > qstats.no_cn = no_cn_flag ? 1 : 0; > - if (bpf_map_update_elem(map_fd, &key, &qstats, BPF_ANY)) { > + if (bpf_map_update_elem(queue_stats_fd, &key, &qstats, BPF_ANY)) { > printf("ERROR: Could not update map element\n"); > goto err; > } > > if (!outFlag) > - type = BPF_CGROUP_INET_INGRESS; > - if (bpf_prog_attach(bpfprog_fd, cg1, type, 0)) { > - printf("ERROR: bpf_prog_attach fails!\n"); > - log_err("Attaching prog"); > + bpf_program__set_expected_attach_type(bpf_prog, BPF_CGROUP_INET_INGRESS); > + > + link = bpf_program__attach_cgroup(bpf_prog, cg1); There is a difference here. I think the bpf_prog will be detached when link is gone (e.g. process exit) I am not sure it is what hbm is expected considering cg is not clean-up on the success case. > + if (libbpf_get_error(link)) { > + fprintf(stderr, "ERROR: bpf_program__attach_cgroup failed\n"); > + link = NULL; not needed. bpf_link__destroy() can handle err ptr. > goto err; > } > > @@ -175,7 +175,7 @@ static int run_bpf_prog(char *prog, int cg_id) > #define DELTA_RATE_CHECK 10000 /* in us */ > #define RATE_THRESHOLD 9500000000 /* 9.5 Gbps */ > > - bpf_map_lookup_elem(map_fd, &key, &qstats); > + bpf_map_lookup_elem(queue_stats_fd, &key, &qstats); > if (gettimeofday(&t0, NULL) < 0) > do_error("gettimeofday failed", true); > t_last = t0; > @@ -204,7 +204,7 @@ static int run_bpf_prog(char *prog, int cg_id) > fclose(fin); > printf(" new_eth_tx_bytes:%llu\n", > new_eth_tx_bytes); > - bpf_map_lookup_elem(map_fd, &key, &qstats); > + bpf_map_lookup_elem(queue_stats_fd, &key, &qstats); > new_cg_tx_bytes = qstats.bytes_total; > delta_bytes = new_eth_tx_bytes - last_eth_tx_bytes; > last_eth_tx_bytes = new_eth_tx_bytes; > @@ -251,14 +251,14 @@ static int run_bpf_prog(char *prog, int cg_id) > rate = minRate; > qstats.rate = rate; > } > - if (bpf_map_update_elem(map_fd, &key, &qstats, BPF_ANY)) > + if (bpf_map_update_elem(queue_stats_fd, &key, &qstats, BPF_ANY)) > do_error("update map element fails", false); > } > } else { > sleep(dur); > } > // Get stats! > - if (stats_flag && bpf_map_lookup_elem(map_fd, &key, &qstats)) { > + if (stats_flag && bpf_map_lookup_elem(queue_stats_fd, &key, &qstats)) { > char fname[100]; > FILE *fout; > > @@ -367,10 +367,12 @@ static int run_bpf_prog(char *prog, int cg_id) > err: > rc = 1; > > + bpf_link__destroy(link); > + > if (cg1) This test looks wrong since cg1 is a fd. > close(cg1); > cleanup_cgroup_environment(); > - > + bpf_object__close(obj); > return rc; > } >
On Wed, Nov 18, 2020 at 11:10 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Nov 17, 2020 at 02:56:37PM +0000, Daniel T. Lee wrote: > [ ... ] > > > + > > +cleanup: > > + if (rc != 0) > so this test can be avoided. > Thanks for pointing me out! I will follow this approach. > > + bpf_object__close(obj); > > + > > + return rc; > > } > > > > [...] > > if (!outFlag) > > - type = BPF_CGROUP_INET_INGRESS; > > - if (bpf_prog_attach(bpfprog_fd, cg1, type, 0)) { > > - printf("ERROR: bpf_prog_attach fails!\n"); > > - log_err("Attaching prog"); > > + bpf_program__set_expected_attach_type(bpf_prog, BPF_CGROUP_INET_INGRESS); > > + > > + link = bpf_program__attach_cgroup(bpf_prog, cg1); > There is a difference here. > I think the bpf_prog will be detached when link is gone (e.g. process exit) > I am not sure it is what hbm is expected considering > cg is not clean-up on the success case. > I think you're right. As I did in the third patch, I will use the link__pin approach to prevent the link from being cleaned up when the process exit. > > + if (libbpf_get_error(link)) { > > + fprintf(stderr, "ERROR: bpf_program__attach_cgroup failed\n"); > > + link = NULL; > not needed. bpf_link__destroy() can handle err ptr. > Thank you for the detailed advice, but in order to make it more clear that link is no longer used, how about keeping this approach? > > goto err; > > } > > [...] > > + > > if (cg1) > This test looks wrong since cg1 is a fd. > I'll remove unnecessary fd compare.
diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore index b2f29bc8dc43..0b9548ea8477 100644 --- a/samples/bpf/.gitignore +++ b/samples/bpf/.gitignore @@ -52,3 +52,6 @@ xdp_tx_iptunnel xdpsock xsk_fwd testfile.img +hbm_out.log +iperf.* +*.out diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile index 3e83cd5ca1c2..01449d767122 100644 --- a/samples/bpf/Makefile +++ b/samples/bpf/Makefile @@ -110,7 +110,7 @@ xdp_fwd-objs := xdp_fwd_user.o task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS) xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) -hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) +hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) # Tell kbuild to always build the programs always-y := $(tprogs-y) diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c index b9f9f771dd81..008bc635ad9b 100644 --- a/samples/bpf/hbm.c +++ b/samples/bpf/hbm.c @@ -46,7 +46,6 @@ #include <bpf/bpf.h> #include <getopt.h> -#include "bpf_load.h" #include "bpf_rlimit.h" #include "trace_helpers.h" #include "cgroup_helpers.h" @@ -68,9 +67,10 @@ bool edt_flag; static void Usage(void); static void do_error(char *msg, bool errno_flag); +struct bpf_program *bpf_prog; struct bpf_object *obj; -int bpfprog_fd; int cgroup_storage_fd; +int queue_stats_fd; static void do_error(char *msg, bool errno_flag) { @@ -83,56 +83,54 @@ static void do_error(char *msg, bool errno_flag) static int prog_load(char *prog) { - struct bpf_prog_load_attr prog_load_attr = { - .prog_type = BPF_PROG_TYPE_CGROUP_SKB, - .file = prog, - .expected_attach_type = BPF_CGROUP_INET_EGRESS, - }; - int map_fd; - struct bpf_map *map; + int rc = 1; - int ret = 0; + obj = bpf_object__open_file(prog, NULL); + if (libbpf_get_error(obj)) { + printf("ERROR: opening BPF object file failed\n"); + return rc; + } - if (access(prog, O_RDONLY) < 0) { - printf("Error accessing file %s: %s\n", prog, strerror(errno)); - return 1; + /* load BPF program */ + if (bpf_object__load(obj)) { + printf("ERROR: loading BPF object file failed\n"); + goto cleanup; } - if (bpf_prog_load_xattr(&prog_load_attr, &obj, &bpfprog_fd)) - ret = 1; - if (!ret) { - map = bpf_object__find_map_by_name(obj, "queue_stats"); - map_fd = bpf_map__fd(map); - if (map_fd < 0) { - printf("Map not found: %s\n", strerror(map_fd)); - ret = 1; - } + + bpf_prog = bpf_object__find_program_by_title(obj, "cgroup_skb/egress"); + if (!bpf_prog) { + printf("ERROR: finding a prog in obj file failed\n"); + goto cleanup; } - 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; + queue_stats_fd = bpf_object__find_map_fd_by_name(obj, "queue_stats"); + if (queue_stats_fd < 0) { + printf("ERROR: finding a map in obj file failed\n"); + goto cleanup; } - return ret; + rc = 0; + +cleanup: + if (rc != 0) + bpf_object__close(obj); + + return rc; } static int run_bpf_prog(char *prog, int cg_id) { - int map_fd; - int rc = 0; + struct hbm_queue_stats qstats = {0}; + struct bpf_link *link = NULL; + char cg_dir[100]; int key = 0; int cg1 = 0; - int type = BPF_CGROUP_INET_EGRESS; - char cg_dir[100]; - struct hbm_queue_stats qstats = {0}; + int rc = 0; sprintf(cg_dir, "/hbm%d", cg_id); - map_fd = prog_load(prog); - if (map_fd == -1) - return 1; + rc = prog_load(prog); + if (rc != 0) + return rc; if (setup_cgroup_environment()) { printf("ERROR: setting cgroup environment\n"); @@ -152,16 +150,18 @@ static int run_bpf_prog(char *prog, int cg_id) qstats.stats = stats_flag ? 1 : 0; qstats.loopback = loopback_flag ? 1 : 0; qstats.no_cn = no_cn_flag ? 1 : 0; - if (bpf_map_update_elem(map_fd, &key, &qstats, BPF_ANY)) { + if (bpf_map_update_elem(queue_stats_fd, &key, &qstats, BPF_ANY)) { printf("ERROR: Could not update map element\n"); goto err; } if (!outFlag) - type = BPF_CGROUP_INET_INGRESS; - if (bpf_prog_attach(bpfprog_fd, cg1, type, 0)) { - printf("ERROR: bpf_prog_attach fails!\n"); - log_err("Attaching prog"); + bpf_program__set_expected_attach_type(bpf_prog, BPF_CGROUP_INET_INGRESS); + + link = bpf_program__attach_cgroup(bpf_prog, cg1); + if (libbpf_get_error(link)) { + fprintf(stderr, "ERROR: bpf_program__attach_cgroup failed\n"); + link = NULL; goto err; } @@ -175,7 +175,7 @@ static int run_bpf_prog(char *prog, int cg_id) #define DELTA_RATE_CHECK 10000 /* in us */ #define RATE_THRESHOLD 9500000000 /* 9.5 Gbps */ - bpf_map_lookup_elem(map_fd, &key, &qstats); + bpf_map_lookup_elem(queue_stats_fd, &key, &qstats); if (gettimeofday(&t0, NULL) < 0) do_error("gettimeofday failed", true); t_last = t0; @@ -204,7 +204,7 @@ static int run_bpf_prog(char *prog, int cg_id) fclose(fin); printf(" new_eth_tx_bytes:%llu\n", new_eth_tx_bytes); - bpf_map_lookup_elem(map_fd, &key, &qstats); + bpf_map_lookup_elem(queue_stats_fd, &key, &qstats); new_cg_tx_bytes = qstats.bytes_total; delta_bytes = new_eth_tx_bytes - last_eth_tx_bytes; last_eth_tx_bytes = new_eth_tx_bytes; @@ -251,14 +251,14 @@ static int run_bpf_prog(char *prog, int cg_id) rate = minRate; qstats.rate = rate; } - if (bpf_map_update_elem(map_fd, &key, &qstats, BPF_ANY)) + if (bpf_map_update_elem(queue_stats_fd, &key, &qstats, BPF_ANY)) do_error("update map element fails", false); } } else { sleep(dur); } // Get stats! - if (stats_flag && bpf_map_lookup_elem(map_fd, &key, &qstats)) { + if (stats_flag && bpf_map_lookup_elem(queue_stats_fd, &key, &qstats)) { char fname[100]; FILE *fout; @@ -367,10 +367,12 @@ static int run_bpf_prog(char *prog, int cg_id) err: rc = 1; + bpf_link__destroy(link); + if (cg1) close(cg1); cleanup_cgroup_environment(); - + bpf_object__close(obj); return rc; } diff --git a/samples/bpf/hbm_kern.h b/samples/bpf/hbm_kern.h index e00f26f6afba..722b3fadb467 100644 --- a/samples/bpf/hbm_kern.h +++ b/samples/bpf/hbm_kern.h @@ -69,7 +69,7 @@ struct { __uint(type, BPF_MAP_TYPE_ARRAY); __uint(max_entries, 1); __type(key, u32); - __type(value, struct hvm_queue_stats); + __type(value, struct hbm_queue_stats); } queue_stats SEC(".maps"); struct hbm_pkt_info {
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. 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 <danieltimlee@gmail.com> --- samples/bpf/.gitignore | 3 ++ samples/bpf/Makefile | 2 +- samples/bpf/hbm.c | 96 +++++++++++++++++++++--------------------- samples/bpf/hbm_kern.h | 2 +- 4 files changed, 54 insertions(+), 49 deletions(-)