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)".
