Message-ID: <20181214172642.GA26720@kernel.org>
Date:   Fri, 14 Dec 2018 14:26:42 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH bpf-next 0/8] tools: bpftool: add probes for system and
 device

Em Fri, Dec 14, 2018 at 02:56:06PM +0000, Quentin Monnet escreveu:
> 2018-12-14 11:00 UTC-0300 ~ Arnaldo Carvalho de Melo <acme@...nel.org>
> > One other thing I noticed is that this has lots of goodies that are not
> > bpftool specific, like the json writer, the procfs reading routines,
> > etc.

> > perf has these and some were even moved to tools/lib/api/, things like
> > finding the procfs, debugfs, sysfs mount points, routines to mount then
> > automatically when the user can do it, etc.

> > Have you considered using them? If so, what prevented you from doing it?
> > Licensing?
 
> Some of it is not necessarily relevant. For JSON, for example, it seems
> that related parts in perf provide JSON parsing. bpftool does not parse
> any JSON, it only needs to print JSON, and I didn't find that in perf.

Right, perf reads, bpftool writes, so we have a complete JSON library,
having it in one place would expose it for further reuse.

For instance, I want to output in JSON the information in a perf.data
file:

[root@...co ~]# perf report --header-only
# ========
# captured on    : Fri Dec 14 14:00:33 2018
# header version : 1
# data offset    : 488
# data size      : 22784
# feat offset    : 23272
# hostname : quaco
# os release : 4.20.0-rc5
# perf version : 4.20.rc3.g4ef9231
# arch : x86_64
# nrcpus online : 8
# nrcpus avail : 8
# cpudesc : Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
# cpuid : GenuineIntel,6,142,10
# total memory : 24555788 kB
# cmdline : /home/acme/bin/perf record -e /home/acme/git/perf/tools/perf/examples/bpf/augmented_raw_syscalls.c sleep 0.00001 
# event : name = raw_syscalls:sys_exit, , id = { 3708326, 3708327, 3708328, 3708329, 3708330, 3708331, 3708332, 3708333 }, type = 2, size = 112, >
# event : name = raw_syscalls:sys_enter, , id = { 3708334, 3708335, 3708336, 3708337, 3708338, 3708339, 3708340, 3708341 }, type = 2, size = 112,>
# CPU_TOPOLOGY info available, use -I to display
# NUMA_TOPOLOGY info available, use -I to display
# pmu mappings: intel_pt = 8, software = 1, power = 11, uprobe = 7, uncore_imc = 12, cpu = 4, cstate_core = 18, uncore_cbox_2 = 15, breakpoint = >
# CACHE info available, use -I to display
# time of first sample : 0.000000
# time of last sample : 0.000000
# sample duration :      0.000 ms
# MEM_TOPOLOGY info available, use -I to display
# missing features: BRANCH_STACK GROUP_DESC AUXTRACE STAT CLOCKID 
# ========
#
[root@...co ~]#

For consumption by scripts.

> This being said making both JSON parsing and printing available under
> e.g. tools/lib/api (providing licensing details allow it) would make
> sense to me, so that it can be shared if e.g. bpftool had to parse JSON
> one day (or if other tools were added).

Right, I'd suggest tools/lib/json/ instead.
 
> As for BPF, most of the low-level object management directly comes from
> libbpf. I see perf uses it as well, but it doesn't really look like the
> code of the two tools could be merged easily now.

Right, perf was its first user, so since day one the idea was for
everything bpf related to go to tools/lib/bpf/ for reuse by other tools,
but what I was alluding to was some more stuff that comes before having
a BPF object for loading, and involves calling an external compiler, and
that involves finding the kbuild environment, etc.

That file name "llvm-utils.c" is kinda misleading, as it doesn't have
only thing exclusively related to llvm, for instance:

int llvm__get_nr_cpus(void)
{
        static int nr_cpus_avail = 0;
        char serr[STRERR_BUFSIZE];

        if (nr_cpus_avail > 0)
                return nr_cpus_avail;

        nr_cpus_avail = sysconf(_SC_NPROCESSORS_CONF);
        if (nr_cpus_avail <= 0) {
                pr_err(
"WARNING:\tunable to get available CPUs in this system: %s\n"
"        \tUse 128 instead.\n", str_error_r(errno, serr, sizeof(serr)));
                nr_cpus_avail = 128;
        }
        return nr_cpus_avail;
}

void llvm__get_kbuild_opts(char **kbuild_dir, char **kbuild_include_opts)

That calls things like:

static int detect_kbuild_dir(char **kbuild_dir)

int
fetch_kernel_version(unsigned int *puint, char *str,
                     size_t str_size)

That handles artifacts such as:

        if (access("/proc/version_signature", R_OK) == 0)
                if (!fetch_ubuntu_kernel_version(puint))
                        int_ver_ready = true;

Introduced in:

commit d18acd15c6dfb669f0463afa31ac5343594d2fe2
Author: Wang Nan <wangnan0@...wei.com>
Date:   Tue Nov 15 04:05:44 2016 +0000

    perf tools: Fix kernel version error in ubuntu

    On ubuntu the internal kernel version code is different from what can
    be retrived from uname:
 
> Regarding procfs, debugfs, sysfs, it's true that there are things that
> we could maybe reuse. Here I don't have any argument - mostly I didn't
> notice these functions were available here when working on bpftool (I
> mostly looked at iproute2 instead).

Ok
 
> > Sharing these non-tool specific routines in tools/lib/ is a good thing
> > and we should work out details on what prevents that from happening.
> > 
> > Another thing that came to mind is that the bpf loaded in perf has
> > routines for figuring out the kbuild directory, kernel version, etc.
> > 
> > Please take a look at tools/perf/util/llvm-utils.c and
> > tools/perf/util/bpf-loader.c. If we could reuse what is there, working
> > out licensing details with Wang, etc, that would be awesome.
> 
> At the moment bpftool does not use LLVM stuff. My opinion is that for
> loading programs, improvements that could benefit to multiple projects
> should probably go to libbpf. For things such as retrieving the kernel
> version, we can maybe do something.

See above about the "llvm" routines, which are there because we need it
to call llvm but are not llvm specific and maybe interesting for other
purposes.
 
> Overall I agree it would be nice to share more code between the two,
> providing licensing allows it. I'm not sure there's much to share about
> BPF itself, but JSON or other FS-related things could maybe be reused.
> I'll try to look at it in more details when I have some time, and keep
> it in mind for future bpftool work. Thanks!

Ok!

- Arnaldo

Powered by blists - more mailing lists