Re: [RFC PATCH 07/13] bpf tools: Load ubpf program

[Arnaldo Carvalho de Melo]

Em Wed, Apr 20, 2016 at 06:01:47PM +0000, Wang Nan escreveu:
> In bpf_program__load(), load ubpf program according to its engine type.
> 
> API is improvemented to hold 'struct ubpf_vm *'.
> 
> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> Cc: Brendan Gregg <brendan.d.gregg@xxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Li Zefan <lizefan@xxxxxxxxxx>
> ---
>  tools/lib/bpf/libbpf.c | 66 +++++++++++++++++++++++++++++++++++++++-----------
>  tools/lib/bpf/libbpf.h | 11 +++++++--
>  2 files changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3a969fd..e4a1e77 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -81,6 +81,7 @@ static const char *libbpf_strerror_table[NR_ERRNO] = {
>  	[ERRCODE_OFFSET(PROG2BIG)]	= "Program too big",
>  	[ERRCODE_OFFSET(KVER)]		= "Incorrect kernel version",
>  	[ERRCODE_OFFSET(NOUBPF)]	= "UBPF support is not compiled",
> +	[ERRCODE_OFFSET(LOADUBPF)]	= "Failed to load user space BPF program",
>  };
>  
>  int libbpf_strerror(int err, char *buf, size_t size)
> @@ -949,6 +950,31 @@ static int bpf_object__collect_reloc(struct bpf_object *obj)
>  	return 0;
>  }
>  
> +#ifdef HAVE_UBPF_SUPPORT
> +static int
> +load_ubpf_program(struct bpf_insn *insns, int insns_cnt,
> +		  struct ubpf_vm **pvm)
> +{
> +	struct ubpf_vm *vm = ubpf_create();
> +	char *message;
> +	int err;
> +
> +	if (!vm) {
> +		pr_warning("Failed to create ubpf vm\n");
> +		return -LIBBPF_ERRNO__LOADUBPF;
> +	}
> +
> +	err = ubpf_load(vm, insns, insns_cnt * sizeof(insns[0]), &message);
> +	if (err < 0) {
> +		pr_warning("Failed to load ubpf program: %s\n", message);
> +		return -LIBBPF_ERRNO__LOADUBPF;
> +	}
> +
> +	*pvm = vm;
> +	return 0;
> +}
> +#endif
> +
>  static int
>  load_program(struct bpf_insn *insns, int insns_cnt,
>  	     char *license, u32 kern_version, int *pfd)
> @@ -1002,11 +1028,12 @@ bpf_program__load(struct bpf_program *prog,
>  		  char *license, u32 kern_version)
>  {
>  	int err = 0, fd, i;
> +#ifdef HAVE_UBPF_SUPPORT
> +	struct ubpf_vm *vm;
> +#endif
>  
>  	if (prog->engine == ENGINE_UNKNOWN)
>  		prog->engine = ENGINE_KBPF;
> -	if (prog->engine != ENGINE_KBPF)
> -		return -EINVAL;
>  
>  	if (prog->instances.nr < 0 || !prog->instances.array) {
>  		if (prog->preprocessor) {
> @@ -1029,10 +1056,15 @@ bpf_program__load(struct bpf_program *prog,
>  			pr_warning("Program '%s' is inconsistent: nr(%d) != 1\n",
>  				   prog->section_name, prog->instances.nr);
>  		}
> -		err = load_program(prog->insns, prog->insns_cnt,
> -				   license, kern_version, &fd);
> +
> +		if_engine(prog,
> +			  (err = load_program(prog->insns, prog->insns_cnt,
> +					      license, kern_version, &fd)),
> +			  (err = load_ubpf_program(prog->insns, prog->insns_cnt,
> +						   &vm)));


Same thing here, with that struct bpf_engine fops you would just do:

		err = engine->load(prog, arg);

> +
>  		if (!err)
> -			prog->instan_fd(0) = fd;
> +			set_instance(prog, 0, fd, vm);
>  		goto out;
>  	}
>  
> @@ -1052,15 +1084,21 @@ 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->instan_fd(i) = -1;
> -			if (result.pfd)
> -				*result.pfd = -1;
> +
> +			if_engine(prog, prog->instan_fd(i) = -1,
> +					prog->instan_vm(i) = NULL);
> +			if (result.ptr)
> +				if_engine(prog, *result.pfd = -1, *result.pvm = NULL);
>  			continue;
>  		}
>  
> -		err = load_program(result.new_insn_ptr,
> -				   result.new_insn_cnt,
> -				   license, kern_version, &fd);
> +		if_engine(prog,
> +			  (err = load_program(result.new_insn_ptr,
> +					      result.new_insn_cnt,
> +					      license, kern_version, &fd)),
> +			  (err = load_ubpf_program(result.new_insn_ptr,
> +						   result.new_insn_cnt,
> +						   &vm)));
>  
>  		if (err) {
>  			pr_warning("Loading the %dth instance of program '%s' failed\n",
> @@ -1068,9 +1106,9 @@ bpf_program__load(struct bpf_program *prog,
>  			goto out;
>  		}
>  
> -		if (result.pfd)
> -			*result.pfd = fd;
> -		prog->instan_fd(i) = fd;
> +		if (result.ptr)
> +			if_engine(prog, *result.pfd = fd, *result.pvm = vm);
> +		set_instance(prog, i, fd, vm);
>  	}
>  out:
>  	if (err)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index f6965ce..8e69c6f 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -27,6 +27,7 @@ enum libbpf_errno {
>  	LIBBPF_ERRNO__PROG2BIG,	/* Program too big */
>  	LIBBPF_ERRNO__KVER,	/* Incorrect kernel version */
>  	LIBBPF_ERRNO__NOUBPF,	/* UBPF support is not compiled */
> +	LIBBPF_ERRNO__LOADUBPF,	/* Failed to load user space BPF program */
>  	__LIBBPF_ERRNO__END,
>  };
>  
> @@ -123,6 +124,8 @@ struct bpf_insn;
>   * bpf_program__nth_fd(prog, 0).
>   */
>  
> +struct ubpf_vm;
> +
>  struct bpf_prog_prep_result {
>  	/*
>  	 * If not NULL, load new instruction array.
> @@ -131,8 +134,12 @@ struct bpf_prog_prep_result {
>  	struct bpf_insn *new_insn_ptr;
>  	int new_insn_cnt;
>  
> -	/* If not NULL, result fd is set to it */
> -	int *pfd;
> +	/* If not NULL, result is set to it */
> +	union {
> +		void *ptr;
> +		int *pfd;
> +		struct ubpf_vm **pvm;
> +	};
>  };

see? Here you use anonymous unions, but after all I wrote, I think that
you really need a fops struct, like we have so many in the kernel
sources, see struct inode_operations, file_operations, etc, and then
that bpf_program->instance.entries needs just to be a void pointer.

- Arnaldo