| Message ID | 20201119150617.92010-1-danieltimlee@gmail.com |
|---|---|
| Headers | show |
| Series | bpf: remove bpf_load loader completely | expand |
On Thu, Nov 19, 2020 at 03:06:11PM +0000, Daniel T. Lee wrote: [ ... ] > static int run_bpf_prog(char *prog, int cg_id) > { > - int map_fd; > - int rc = 0; > + struct hbm_queue_stats qstats = {0}; > + char cg_dir[100], cg_pin_path[100]; > + struct bpf_link *link = NULL; > 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"); > @@ -190,16 +183,25 @@ 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; Again, this is not needed. bpf_link__destroy() can handle both NULL and error pointer. Please take a look at the bpf_link__destroy() in libbpf.c > + goto err; > + } > + > + sprintf(cg_pin_path, "/sys/fs/bpf/hbm%d", cg_id); > + rc = bpf_link__pin(link, cg_pin_path); > + if (rc < 0) { > + printf("ERROR: bpf_link__pin failed: %d\n", rc); > goto err; > } > > @@ -213,7 +215,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; > @@ -242,7 +244,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; > @@ -289,14 +291,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; > > @@ -398,10 +400,10 @@ static int run_bpf_prog(char *prog, int cg_id) > err: > rc = 1; > > - if (cg1) > - close(cg1); > + bpf_link__destroy(link); > + close(cg1); > cleanup_cgroup_environment(); > - > + bpf_object__close(obj); The bpf_* cleanup condition still looks wrong. I can understand why it does not want to cleanup_cgroup_environment() on the success case because the sh script may want to run test under this cgroup. However, the bpf_link__destroy(), bpf_object__close(), and even close(cg1) should be done in both success and error cases. The cg1 test still looks wrong also. The cg1 should be init to -1 and then test for "if (cg1 == -1)".
On Fri, Nov 20, 2020 at 06:34:05PM -0800, Martin KaFai Lau wrote: > On Thu, Nov 19, 2020 at 03:06:11PM +0000, Daniel T. Lee wrote: > [ ... ] > > > static int run_bpf_prog(char *prog, int cg_id) > > { > > - int map_fd; > > - int rc = 0; > > + struct hbm_queue_stats qstats = {0}; > > + char cg_dir[100], cg_pin_path[100]; > > + struct bpf_link *link = NULL; > > 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"); > > @@ -190,16 +183,25 @@ 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; > Again, this is not needed. bpf_link__destroy() can > handle both NULL and error pointer. Please take a look > at the bpf_link__destroy() in libbpf.c > > > + goto err; > > + } > > + > > + sprintf(cg_pin_path, "/sys/fs/bpf/hbm%d", cg_id); > > + rc = bpf_link__pin(link, cg_pin_path); > > + if (rc < 0) { > > + printf("ERROR: bpf_link__pin failed: %d\n", rc); > > goto err; > > } > > > > @@ -213,7 +215,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; > > @@ -242,7 +244,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; > > @@ -289,14 +291,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; > > > > @@ -398,10 +400,10 @@ static int run_bpf_prog(char *prog, int cg_id) > > err: > > rc = 1; > > > > - if (cg1) > > - close(cg1); > > + bpf_link__destroy(link); > > + close(cg1); > > cleanup_cgroup_environment(); > > - > > + bpf_object__close(obj); > The bpf_* cleanup condition still looks wrong. > > I can understand why it does not want to cleanup_cgroup_environment() > on the success case because the sh script may want to run test under this > cgroup. > > However, the bpf_link__destroy(), bpf_object__close(), and > even close(cg1) should be done in both success and error > cases. > > The cg1 test still looks wrong also. The cg1 should > be init to -1 and then test for "if (cg1 == -1)". oops. I meant cg1 != -1 .
Sorry for the late reply. On Sat, Nov 21, 2020 at 11:34 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Thu, Nov 19, 2020 at 03:06:11PM +0000, Daniel T. Lee wrote: > [ ... ] > > > static int run_bpf_prog(char *prog, int cg_id) > > [ ... ] > > 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; > Again, this is not needed. bpf_link__destroy() can > handle both NULL and error pointer. Please take a look > at the bpf_link__destroy() in libbpf.c > > > + goto err; > > + } > > [ ... ] > > @@ -398,10 +400,10 @@ static int run_bpf_prog(char *prog, int cg_id) > > err: > > rc = 1; > > > > - if (cg1) > > - close(cg1); > > + bpf_link__destroy(link); > > + close(cg1); > > cleanup_cgroup_environment(); > > - > > + bpf_object__close(obj); > The bpf_* cleanup condition still looks wrong. > > I can understand why it does not want to cleanup_cgroup_environment() > on the success case because the sh script may want to run test under this > cgroup. > > However, the bpf_link__destroy(), bpf_object__close(), and > even close(cg1) should be done in both success and error > cases. > > The cg1 test still looks wrong also. The cg1 should > be init to -1 and then test for "if (cg1 == -1)". Thanks for pointing this out. I'll remove NULL initialize and fix this on the next patch. -- Best, Daniel T. Lee