RE: [PATCH bpf-next 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link

[John Fastabend <john.fastabend@gmail.com>]

Daniel T. Lee wrote:
> The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
> than the previous method using ioctl.
> 
> bpf_program__attach_perf_event manages the enable of perf_event and
> attach of BPF programs to it, so there's no neeed to do this
> directly with ioctl.
> 
> In addition, bpf_link provides consistency in the use of API because it
> allows disable (detach, destroy) for multiple events to be treated as
> one bpf_link__destroy.
> 
> This commit refactors samples that attach the bpf program to perf_event
> by using libbbpf instead of ioctl. Also the bpf_load in the samples were
> removed and migrated to use libbbpf API.
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---

[...]

>  
>  int main(int argc, char **argv)
>  {
> +	int prog_fd, *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
> +	struct bpf_program *prog;
> +	struct bpf_object *obj;
> +	struct bpf_link **link;
>  	char filename[256];
> -	int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
>  
>  	/* process arguments */
>  	while ((opt = getopt(argc, argv, "F:h")) != -1) {
> @@ -165,36 +170,47 @@ int main(int argc, char **argv)
>  	/* create perf FDs for each CPU */
>  	nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
>  	pmu_fd = malloc(nr_cpus * sizeof(int));
> -	if (pmu_fd == NULL) {
> -		fprintf(stderr, "ERROR: malloc of pmu_fd\n");
> +	link = malloc(nr_cpus * sizeof(struct bpf_link *));
> +	if (pmu_fd == NULL || link == NULL) {
> +		fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
>  		return 1;
>  	}
>  
>  	/* load BPF program */
>  	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> -	if (load_bpf_file(filename)) {
> +	if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd)) {
>  		fprintf(stderr, "ERROR: loading BPF program (errno %d):\n",
>  			errno);
> -		if (strcmp(bpf_log_buf, "") == 0)
> -			fprintf(stderr, "Try: ulimit -l unlimited\n");
> -		else
> -			fprintf(stderr, "%s", bpf_log_buf);
>  		return 1;
>  	}
> +
> +	prog = bpf_program__next(NULL, obj);
> +	if (!prog) {
> +		printf("finding a prog in obj file failed\n");
> +		return 1;
> +	}
> +
> +	map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map");
> +	if (map_fd < 0) {
> +		printf("finding a ip_map map in obj file failed\n");
> +		return 1;
> +	}
> +
>  	signal(SIGINT, int_exit);
>  	signal(SIGTERM, int_exit);
>  
>  	/* do sampling */
>  	printf("Sampling at %d Hertz for %d seconds. Ctrl-C also ends.\n",
>  	       freq, secs);
> -	if (sampling_start(pmu_fd, freq) != 0)
> +	if (sampling_start(pmu_fd, freq, prog, link) != 0)
>  		return 1;
>  	sleep(secs);
> -	sampling_end(pmu_fd);
> +	sampling_end(link);
>  	free(pmu_fd);
> +	free(link);

Not really a problem with this patch but on error we don't free() memory but
then on normal exit there is a free() its a bit inconsistent. How about adding
free on errors as well?

>  
>  	/* output sample counts */
> -	print_ip_map(map_fd[0]);
> +	print_ip_map(map_fd);
>  
>  	return 0;
>  }

[...]
  
>  static void print_ksym(__u64 addr)
> @@ -137,6 +136,7 @@ static inline int generate_load(void)
>  static void test_perf_event_all_cpu(struct perf_event_attr *attr)
>  {
>  	int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
> +	struct bpf_link **link = malloc(nr_cpus * sizeof(struct bpf_link *));

need to check if its null? Its not going to be very friendly to segfault
later. Or maybe I'm missing the check.

>  	int *pmu_fd = malloc(nr_cpus * sizeof(int));
>  	int i, error = 0;
>  
> @@ -151,8 +151,12 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
>  			error = 1;
>  			goto all_cpu_err;
>  		}
> -		assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
> -		assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE) == 0);
> +		link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
> +		if (link[i] < 0) {
> +			printf("bpf_program__attach_perf_event failed\n");
> +			error = 1;
> +			goto all_cpu_err;
> +		}
>  	}
>  
>  	if (generate_load() < 0) {
> @@ -161,11 +165,11 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
>  	}
>  	print_stacks();
>  all_cpu_err:
> -	for (i--; i >= 0; i--) {
> -		ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
> -		close(pmu_fd[i]);
> -	}
> +	for (i--; i >= 0; i--)
> +		bpf_link__destroy(link[i]);
> +
>  	free(pmu_fd);
> +	free(link);
>  	if (error)
>  		int_exit(0);
>  }

Thanks,
John