Re: [RFC PATCH 06/13] bpf tools: Introduce ubpf_vm to program instance union

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

Em Wed, Apr 20, 2016 at 06:01:46PM +0000, Wang Nan escreveu:
> Add 'struct ubpf_vm *' into prog_instance union. Introduce if_engine()
> macro to merge common code.
> 
> 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 | 76 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 68 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3755846..3a969fd 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -141,6 +141,9 @@ int libbpf_strerror(int err, char *buf, size_t size)
>  
>  union prog_instance {
>  	int fd;
> +#ifdef HAVE_UBPF_SUPPORT
> +	struct ubpf_vm *vm;
> +#endif
>  };
>  
>  /*
> @@ -176,6 +179,56 @@ struct bpf_program {
>  	bpf_program_clear_priv_t clear_priv;
>  };
>  
> +#ifdef HAVE_UBPF_SUPPORT
> +# define __if_engine(p, d, k, u)	\
> +	do {				\
> +		switch (p->engine) {	\
> +		default:		\
> +		case ENGINE_UNKNOWN: {	\
> +			d;		\
> +		}			\
> +		case ENGINE_KBPF: {	\
> +			k; break;	\
> +		}			\
> +		case ENGINE_UBPF: {	\
> +			u; break;	\
> +		}			\
> +		}			\
> +	} while(0)

These macro tricks are getting unecessarily overly complicated :-\

> +
> +/*
> + * ubpf_destroy() doesn't accept NULL input. This wrapper makes
> + * it similar to zclose.
> + */
> +# define __ubpf_destroy(vm) do {	\
> +	if (vm)				\
> +		ubpf_destroy(vm);	\
> +	(vm) = NULL;			\
> +} while(0)
> +
> +#else
> +# define __if_engine(p, d, k, u)	\
> +	do { k; } while(0)
> +#endif

Why use just one letter parameters, give them proper names

> +
> +#define instan_fd(i) instances.array[i].fd
> +#define instan_vm(i) instances.array[i].vm
> +
> +#define if_engine(p, k, u) __if_engine(p, do { } while(0), k, u)
> +#define set_instance(p, i, k, u)		\
> +	if_engine(p,				\
> +		  p->instan_fd(i) = k,	\
> +		  p->instan_vm(i) = u)

If you had used a void pointer for instances->entries you wouldn't have
to play such tricks, right?

> +
> +static inline void init_instance_array(struct bpf_program *prog)
> +{
> +	size_t size = sizeof(prog->instances.array[0]) * prog->instances.nr;
> +
> +	if_engine(prog,
> +		  memset(prog->instances.array, -1, size),
> +		  memset(prog->instances.array, 0, size));
> +}



> +
>  struct bpf_map {
>  	int fd;
>  	char *name;
> @@ -239,7 +292,9 @@ 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.array[i].fd);
> +			if_engine(prog,
> +				  zclose(prog->instan_fd(i)),
> +				  __ubpf_destroy(prog->instan_vm(i)));

So if we have more types of instances this will become a
switch_engine()?

>  	} else if (prog->instances.nr != -1) {
>  		pr_warning("Internal error: instances.nr is %d\n",
>  			   prog->instances.nr);
> @@ -966,7 +1021,7 @@ bpf_program__load(struct bpf_program *prog,
>  			return -ENOMEM;
>  		}
>  		prog->instances.nr = 1;
> -		prog->instances.array[0].fd = -1;
> +		set_instance(prog, 0, -1, NULL);

And here we would go on adding more and more values? Why not have some

struct bpf_engine {
	void (*init)(struct bpf_program *prog);
	void (*fini)(struct bpf_program *prog);
}

One for the kernel "engine", the other for the userspace one?

>  	}
>  
>  	if (!prog->preprocessor) {
> @@ -977,7 +1032,7 @@ bpf_program__load(struct bpf_program *prog,
>  		err = load_program(prog->insns, prog->insns_cnt,
>  				   license, kern_version, &fd);
>  		if (!err)
> -			prog->instances.array[0].fd = fd;
> +			prog->instan_fd(0) = fd;
>  		goto out;
>  	}
>  
> @@ -997,7 +1052,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.array[i].fd = -1;
> +			prog->instan_fd(i) = -1;
>  			if (result.pfd)
>  				*result.pfd = -1;
>  			continue;
> @@ -1015,7 +1070,7 @@ bpf_program__load(struct bpf_program *prog,
>  
>  		if (result.pfd)
>  			*result.pfd = fd;
> -		prog->instances.array[i].fd = fd;
> +		prog->instan_fd(i) = fd;
>  	}
>  out:
>  	if (err)
> @@ -1301,12 +1356,11 @@ int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
>  		return -ENOMEM;
>  	}
>  
> -	/* fill all fd with -1 */
> -	memset(array, -1, sizeof(array[0]) * nr_instances);
> -
>  	prog->instances.nr = nr_instances;
>  	prog->instances.array = array;
>  	prog->preprocessor = prep;
> +
> +	init_instance_array(prog);
>  	return 0;
>  }
>  
> @@ -1314,6 +1368,12 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n)
>  {
>  	int fd;
>  
> +	if (prog->engine != ENGINE_KBPF) {
> +		pr_warning("Can't get fd from program %s: engine not KBPF or not loaded\n",
> +			   prog->section_name);
> +		return -EINVAL;
> +	}
> +
>  	if (n >= prog->instances.nr || n < 0) {
>  		pr_warning("Can't get the %dth fd from program %s: only %d instances\n",
>  			   n, prog->section_name, prog->instances.nr);
> -- 
> 1.8.3.4