Re: [RFC PATCH 04/13] bpf tools: Replace fd array to union array

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Apr 20, 2016 at 06:01:44PM +0000, Wang Nan escreveu:
> Following commits will add new types for instance of a bpf program.
> For kernel bpf program, a program instance is fd. For ubpf program,
> the instance of a program is a 'struct ubpf_vm'. This patch promote
> original 'fds' array to 'union prog_instance' array, prepare for
> further extending.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Li Zefan <lizefan@huawei.com>
> ---
>  tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 55b739e..dd5a107 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -138,6 +138,10 @@ int libbpf_strerror(int err, char *buf, size_t size)
>  # define LIBBPF_ELF_C_READ_MMAP ELF_C_READ
>  #endif
>  
> +union prog_instance {
> +	int fd;
> +};
> +
>  /*
>   * bpf_prog should be a better name but it has been used in
>   * linux/filter.h.
> @@ -157,7 +161,7 @@ struct bpf_program {
>  
>  	struct {
>  		int nr;
> -		int *fds;
> +		union prog_instance *array;
>  	} instances;

Why not just do:

	struct {
		int nr;
		union {
			int *fds;
		};
	} instances;

And then do no further changes and when going to the ubpf (haven't
looked at the actual patch, proposing this based on your above
description), just transform it into:

	struct {
		int nr;
		union {
			int		*fds;
			struct ubpf_vm	*vms;
		};
	} instances;

Or plain use a void pointer and use:

	struct {
		int  nr;
		void *entries;
	} instances;

	if (prog->instances.nr > 0) {
		int *fds = prog->instances.entries;

		for (i = 0; i < prog->instances.nr; i++)
			zclose(fd[i]);
	}

>  	bpf_program_prep_t preprocessor;
>  
> @@ -229,14 +233,14 @@ static void bpf_program__unload(struct bpf_program *prog)
>  	 */
>  	if (prog->instances.nr > 0) {
>  		for (i = 0; i < prog->instances.nr; i++)
> -			zclose(prog->instances.fds[i]);
> +			zclose(prog->instances.array[i].fd);
>  	} else if (prog->instances.nr != -1) {
>  		pr_warning("Internal error: instances.nr is %d\n",
>  			   prog->instances.nr);
>  	}
>  
>  	prog->instances.nr = -1;
> -	zfree(&prog->instances.fds);
> +	zfree(&prog->instances.array);
>  }
>  
>  static void bpf_program__exit(struct bpf_program *prog)
> @@ -287,7 +291,7 @@ bpf_program__init(void *data, size_t size, char *name, int idx,
>  	memcpy(prog->insns, data,
>  	       prog->insns_cnt * sizeof(struct bpf_insn));
>  	prog->idx = idx;
> -	prog->instances.fds = NULL;
> +	prog->instances.array = NULL;
>  	prog->instances.nr = -1;
>  
>  	return 0;
> @@ -937,20 +941,20 @@ bpf_program__load(struct bpf_program *prog,
>  {
>  	int err = 0, fd, i;
>  
> -	if (prog->instances.nr < 0 || !prog->instances.fds) {
> +	if (prog->instances.nr < 0 || !prog->instances.array) {
>  		if (prog->preprocessor) {
>  			pr_warning("Internal error: can't load program '%s'\n",
>  				   prog->section_name);
>  			return -LIBBPF_ERRNO__INTERNAL;
>  		}
>  
> -		prog->instances.fds = malloc(sizeof(int));
> -		if (!prog->instances.fds) {
> -			pr_warning("Not enough memory for BPF fds\n");
> +		prog->instances.array = malloc(sizeof(union prog_instance));
> +		if (!prog->instances.array) {
> +			pr_warning("Not enough memory for BPF instance\n");
>  			return -ENOMEM;
>  		}
>  		prog->instances.nr = 1;
> -		prog->instances.fds[0] = -1;
> +		prog->instances.array[0].fd = -1;
>  	}
>  
>  	if (!prog->preprocessor) {
> @@ -961,7 +965,7 @@ bpf_program__load(struct bpf_program *prog,
>  		err = load_program(prog->insns, prog->insns_cnt,
>  				   license, kern_version, &fd);
>  		if (!err)
> -			prog->instances.fds[0] = fd;
> +			prog->instances.array[0].fd = fd;
>  		goto out;
>  	}
>  
> @@ -981,7 +985,7 @@ bpf_program__load(struct bpf_program *prog,
>  		if (!result.new_insn_ptr || !result.new_insn_cnt) {
>  			pr_debug("Skip loading the %dth instance of program '%s'\n",
>  				 i, prog->section_name);
> -			prog->instances.fds[i] = -1;
> +			prog->instances.array[i].fd = -1;
>  			if (result.pfd)
>  				*result.pfd = -1;
>  			continue;
> @@ -999,7 +1003,7 @@ bpf_program__load(struct bpf_program *prog,
>  
>  		if (result.pfd)
>  			*result.pfd = fd;
> -		prog->instances.fds[i] = fd;
> +		prog->instances.array[i].fd = fd;
>  	}
>  out:
>  	if (err)
> @@ -1269,27 +1273,27 @@ int bpf_program__fd(struct bpf_program *prog)
>  int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
>  			  bpf_program_prep_t prep)
>  {
> -	int *instances_fds;
> +	union prog_instance *array;
>  
>  	if (nr_instances <= 0 || !prep)
>  		return -EINVAL;
>  
> -	if (prog->instances.nr > 0 || prog->instances.fds) {
> +	if (prog->instances.nr > 0 || prog->instances.array) {
>  		pr_warning("Can't set pre-processor after loading\n");
>  		return -EINVAL;
>  	}
>  
> -	instances_fds = malloc(sizeof(int) * nr_instances);
> -	if (!instances_fds) {
> -		pr_warning("alloc memory failed for fds\n");
> +	array = malloc(sizeof(array[0]) * nr_instances);
> +	if (!array) {
> +		pr_warning("alloc memory failed for instances\n");
>  		return -ENOMEM;
>  	}
>  
>  	/* fill all fd with -1 */
> -	memset(instances_fds, -1, sizeof(int) * nr_instances);
> +	memset(array, -1, sizeof(array[0]) * nr_instances);
>  
>  	prog->instances.nr = nr_instances;
> -	prog->instances.fds = instances_fds;
> +	prog->instances.array = array;
>  	prog->preprocessor = prep;
>  	return 0;
>  }
> @@ -1304,7 +1308,7 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n)
>  		return -EINVAL;
>  	}
>  
> -	fd = prog->instances.fds[n];
> +	fd = prog->instances.array[n].fd;
>  	if (fd < 0) {
>  		pr_warning("%dth instance of program '%s' is invalid\n",
>  			   n, prog->section_name);
> -- 
> 1.8.3.4