Re: [PATCH v11 00/39] perf tools: filtering events using eBPF programs - part1

[Arnaldo Carvalho de Melo]

Em Wed, Jul 08, 2015 at 11:57:13PM +0800, pi3orama escreveu:
> åèæç iPhone
> > å 2015å7æ8æïäå10:03ïArnaldo Carvalho de Melo <acme@xxxxxxxxxx> åéï
> > And those wrappers struck me as exaggerated, see one of them:

> > int bpf_program__get_fd(struct bpf_program *prog, int *pfd)
> > {            
> >    if (!pfd)
> >        return -EINVAL;
> > 
> >    *pfd = prog->fd;
> >    return 0;
> > }

> > What can go wrong with accessing a struct member? The only think I
> > thought about was: hey, the struct pointer needs to be checked against
> > NULL, but no, in this case what you thought could go wrong was for the
> > library user to pass a NULL pointer as the return place (pfd).
> > 
> 
> OK, I will change. You won't see it in next version.
 
> However, for the specific function you mentioned, please see patch
> 39/39 in this patchset. It shows that, accessing a struct member can
> go wrong in a way unpredictable when start working on this patch.

So, I haven't got there yet, i.e. I need to understand a patch by what
is in the parts of the code it changes or adds or by what is explained
in its changeset.
 
> When I start working on this patch I thought this function may go
> wrong in only two obvious situations: 1: caller feed a NULL pointer;
> 2: try to get file descriptor before loading that program.

> However, when we start working on BPF prologue generator we realize
> that we must provide a way to enable one program be loaded multiple
> times with different prologues.

> Patch 39/39 is the winner among many ideas which provides the desire
> function while not impact old code (new code for you) too much, which
> we discussed and tries for weeks.

I understand that this was discussed somewhere, but we need to have the
relevant context for a specific patch in its changelog comments or in
comments added in that specific patch, otherwise we will have to look at
multiple messages discussing it, filter out dead ends, etc.

> In that patch we allow caller attach a pre-processor to a program, and
> require them call a new function bpf_program__get_nth_fd() to get the
> nth descriptor.

> The semantic of old bpf_program__get_fd() becomes hard to define so we
> simply disallow them to call it if preprocessing is required. This is
> how a new source of error raise during development.

Ok, but then that other type of error can be returned as a negative
value:

int bpf_program__get_fd(struct bpf_program *prog)
{
	ret = -EINVAL;

	if (prog == NULL)
		goto out;

	ret = -EBADFD;
	if (not yet opened)
		goto out;

	ret = -ESOMETHINGELSE
	if (something else)
		goto out;

	/* More logic */
		
	err = prog->fd;
out:
	return ret;
}

int some_other_func(struct bpf_program *prog)
{
	int fd = bpf_program__get_fd(prog);

	if (fd < 0)
		goto out_err;

	return 0;
out_err:
	printf("Something went wrong: %s\n", strerror(-fd));
	return fd;
}

Looking at the users, i.e. going to the future to figure out the current
patch we are talking about...

Oops:

[PATCH v11 35/39] perf tools: Add bpf_fd field to evsel and config it

+                               err = bpf_program__get_fd(prog, &fd);
+
+                               if (err || fd < 0) {
+                                       pr_err("bpf: failed to get file descriptor\n");
+                                       return -EINVAL;
+                               }

Here you use _both_ the error return, and doesn't look if it is less
than zero, and if it is zero, i.e. the operation went well, that is not
enough, you have to anyway look at the value of the fd?

And 39/39:

It just checks prog->preprocessed and if not set, returns as well
-EINVAL. I.e. its callers will have to continue checking for errors
_and_ for the fd value returned, it returned zero, I can say that this
is confusing :-\

Reading 39/39, I think you could have just:

	struct bpf_program {
		struct {
			int nr;
			int *fds;
		} instance;
	};

Then _always_ use that prog->instance_fd[], thing, it starts with 1
entry, i.e. what you call now prog->fd is really prog->instance_fd[0],
this way you don't have to have two functions (get_fd and get_nth_fd) to
access the fds associated with a bpf_program, just one, that receives an
index.

Or you could make:

static int bpf_program__get_fd(struct bpf_program *prog)
{
	return bpf_program__get_nth_fd(prog, 0);
}

But now I don't even know if this all makes sense, this is the last
patch in the series, I can't see a user of this preprocessor, to see
if this couldn't be done in other way :-\

I'll continue looking at the patches a bit more...

- Arnaldo
 
> Thank you.
> 
> > So, yes, I still think this is way exaggerated, if you insist that the
> > struct must be opaque and thus we need accessors, I think that having:
> > 
> > int bpf_program__fd(struct bpf_program *prog)
> > {
> >    return prog->fd;
> > }
> > 
> > Is way more sane, yes, I would trow away those extra four characters
> > (get_).
> > 
> > Heck, in this case there is not even a possible problem where we would
> > want to return something negative instead of doing what was requested.
> > 
> > If you find any other part in tools/perf/ (or anywhere else) that
> > doesn't follows the convention it states it conforms to, please file a
> > bug or submit a patch, like you did in the case you mentioned (ed30775),
> > it would be a bug and has to be fixed.
> > 
> > - Arnaldo
> > 
> >> Wang Nan (39):
> >>  bpf: Use correct #ifdef controller for trace_call_bpf()
> >>  tracing, perf: Implement BPF programs attached to uprobes
> >>  bpf tools: Introduce 'bpf' library and add bpf feature check
> >>  bpf tools: Allow caller to set printing function
> >>  bpf tools: Open eBPF object file and do basic validation
> >>  bpf tools: Read eBPF object from buffer
> >>  bpf tools: Check endianness and make libbpf fail early
> >>  bpf tools: Iterate over ELF sections to collect information
> >>  bpf tools: Collect version and license from ELF sections
> >>  bpf tools: Collect map definitions from 'maps' section
> >>  bpf tools: Collect symbol table from SHT_SYMTAB section
> >>  bpf tools: Collect eBPF programs from their own sections
> >>  bpf tools: Collect relocation sections from SHT_REL sections
> >>  bpf tools: Record map accessing instructions for each program
> >>  bpf tools: Add bpf.c/h for common bpf operations
> >>  bpf tools: Create eBPF maps defined in an object file
> >>  bpf tools: Relocate eBPF programs
> >>  bpf tools: Introduce bpf_load_program() to bpf.c
> >>  bpf tools: Load eBPF programs in object files into kernel
> >>  bpf tools: Introduce accessors for struct bpf_program
> >>  bpf tools: Introduce accessors for struct bpf_object
> >>  bpf tools: Link all bpf objects onto a list
> >>  perf tools: Introduce llvm config options
> >>  perf tools: Call clang to compile C source to object code
> >>  perf tools: Auto detecting kernel build directory
> >>  perf tools: Auto detecting kernel include options
> >>  perf tests: Add LLVM test for eBPF on-the-fly compiling
> >>  perf tools: Make perf depend on libbpf
> >>  perf record: Enable passing bpf object file to --event
> >>  perf record: Compile scriptlets if pass '.c' to --event
> >>  perf tools: Parse probe points of eBPF programs during preparation
> >>  perf probe: Attach trace_probe_event with perf_probe_event
> >>  perf record: Probe at kprobe points
> >>  perf record: Load all eBPF object into kernel
> >>  perf tools: Add bpf_fd field to evsel and config it
> >>  perf tools: Attach eBPF program to perf event
> >>  perf tools: Suppress probing messages when probing by BPF loading
> >>  perf record: Add clang options for compiling BPF scripts
> >>  bpf tools: Load a program with different instance using preprocessor
> >> 
> >> include/linux/trace_events.h    |    7 +-
> >> kernel/events/core.c            |    4 +-
> >> kernel/trace/Kconfig            |    2 +-
> >> kernel/trace/trace_uprobe.c     |    5 +
> >> tools/build/Makefile.feature    |    6 +-
> >> tools/build/feature/Makefile    |    6 +-
> >> tools/build/feature/test-bpf.c  |   18 +
> >> tools/lib/bpf/.gitignore        |    2 +
> >> tools/lib/bpf/Build             |    1 +
> >> tools/lib/bpf/Makefile          |  195 +++++++
> >> tools/lib/bpf/bpf.c             |   85 +++
> >> tools/lib/bpf/bpf.h             |   23 +
> >> tools/lib/bpf/libbpf.c          | 1184 +++++++++++++++++++++++++++++++++++++++
> >> tools/lib/bpf/libbpf.h          |  107 ++++
> >> tools/perf/MANIFEST             |    3 +
> >> tools/perf/Makefile.perf        |   19 +-
> >> tools/perf/builtin-probe.c      |    4 +-
> >> tools/perf/builtin-record.c     |   43 +-
> >> tools/perf/config/Makefile      |   19 +-
> >> tools/perf/tests/Build          |    1 +
> >> tools/perf/tests/builtin-test.c |    4 +
> >> tools/perf/tests/llvm.c         |   85 +++
> >> tools/perf/tests/make           |    4 +-
> >> tools/perf/tests/tests.h        |    1 +
> >> tools/perf/util/Build           |    2 +
> >> tools/perf/util/bpf-loader.c    |  310 ++++++++++
> >> tools/perf/util/bpf-loader.h    |   46 ++
> >> tools/perf/util/config.c        |    4 +
> >> tools/perf/util/debug.c         |    5 +
> >> tools/perf/util/debug.h         |    1 +
> >> tools/perf/util/evlist.c        |   41 ++
> >> tools/perf/util/evlist.h        |    1 +
> >> tools/perf/util/evsel.c         |   17 +
> >> tools/perf/util/evsel.h         |    1 +
> >> tools/perf/util/llvm-utils.c    |  373 ++++++++++++
> >> tools/perf/util/llvm-utils.h    |   39 ++
> >> tools/perf/util/parse-events.c  |   16 +
> >> tools/perf/util/parse-events.h  |    2 +
> >> tools/perf/util/parse-events.l  |    6 +
> >> tools/perf/util/parse-events.y  |   29 +-
> >> tools/perf/util/probe-event.c   |   82 +--
> >> tools/perf/util/probe-event.h   |    7 +-
> >> 42 files changed, 2759 insertions(+), 51 deletions(-)
> >> create mode 100644 tools/build/feature/test-bpf.c
> >> create mode 100644 tools/lib/bpf/.gitignore
> >> create mode 100644 tools/lib/bpf/Build
> >> create mode 100644 tools/lib/bpf/Makefile
> >> create mode 100644 tools/lib/bpf/bpf.c
> >> create mode 100644 tools/lib/bpf/bpf.h
> >> create mode 100644 tools/lib/bpf/libbpf.c
> >> create mode 100644 tools/lib/bpf/libbpf.h
> >> create mode 100644 tools/perf/tests/llvm.c
> >> create mode 100644 tools/perf/util/bpf-loader.c
> >> create mode 100644 tools/perf/util/bpf-loader.h
> >> create mode 100644 tools/perf/util/llvm-utils.c
> >> create mode 100644 tools/perf/util/llvm-utils.h
> >> 
> >> -- 
> >> 1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/