netdev - Re: [PATCH v2 bpf-next 4/4] tools/bpftool: recognize bpf_prog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzYZ1ML9tncsXVWo1bFSrj1L6vQQKvqHgHANVehGCeBpVA@mail.gmail.com>
Date:   Mon, 25 Feb 2019 11:24:25 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net,
        edumazet@...gle.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 4/4] tools/bpftool: recognize bpf_prog_info
 runtime and runcnt

On Mon, Feb 25, 2019 at 3:17 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 02/23/2019 06:44 PM, Alexei Starovoitov wrote:
> > $ bpftool p s
> > 1: kprobe  tag a56587d488d216c9  gpl runtime 79786 runcnt 8
> >       loaded_at 2019-02-22T12:22:51-0800  uid 0
> >       xlated 352B  not jited  memlock 4096B
> >
> > $ bpftool --json --pretty p s
> > [{
> >         "id": 1,
> >         "type": "kprobe",
> >         "tag": "a56587d488d216c9",
> >         "gpl_compatible": true,
> >         "run_time_ns": 79786,
> >         "run_cnt": 8,
> >         "loaded_at": 1550866971,
> >         "uid": 0,
> >         "bytes_xlated": 352,
> >         "jited": false,
> >         "bytes_memlock": 4096
> >     }
> > ]
> >
> > Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> > ---
> >  tools/bpf/bpftool/Documentation/bpftool-prog.rst | 4 +++-
> >  tools/bpf/bpftool/prog.c                         | 7 +++++++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index 12bc1e2d4b46..d8b46a8f17e0 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -171,7 +171,7 @@ EXAMPLES
> >
> >  ::
> >
> > -    10: xdp  name some_prog  tag 005a3d2123620c8b  gpl
> > +    10: xdp  name some_prog  tag 005a3d2123620c8b  gpl runtime 81632 runcnt 10
> >              loaded_at 2017-09-29T20:11:00+0000  uid 0
> >              xlated 528B  jited 370B  memlock 4096B  map_ids 10
> >
> > @@ -184,6 +184,8 @@ EXAMPLES
> >              "type": "xdp",
> >              "tag": "005a3d2123620c8b",
> >              "gpl_compatible": true,
> > +            "run_time_ns": 81632,
> > +            "run_cnt": 10,
> >              "loaded_at": 1506715860,
> >              "uid": 0,
> >              "bytes_xlated": 528,
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index db978c8d76a8..cdb6bd424340 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -214,6 +214,10 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
> >                    info->tag[4], info->tag[5], info->tag[6], info->tag[7]);
> >
> >       jsonw_bool_field(json_wtr, "gpl_compatible", info->gpl_compatible);
> > +     if (info->runtime) {
> > +             jsonw_uint_field(json_wtr, "run_time_ns", info->runtime);
> > +             jsonw_uint_field(json_wtr, "run_cnt", info->runcnt);
>
> Didn't Andrii mean to generally name these fields run_time_ns and run_cnt, so also
> uapi aside from just changing bpftool output (why keeping these names not the same)?

Yeah, I think consistency is important. This also applies to patch #1
(bpf: enable program stats), which exposes same stats through procfs.
Ideally all those names would be consistent (for obvious reasons). As
for _ns, I realize it's a bit longer and more verbose, but having time
units as part of a name is really great for users and doesn't require
extra documentation (and extra man lookups).


>
> > +     }
> >
> >       print_dev_json(info->ifindex, info->netns_dev, info->netns_ino);
> >
> > @@ -277,6 +281,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
> >       fprint_hex(stdout, info->tag, BPF_TAG_SIZE, "");
> >       print_dev_plain(info->ifindex, info->netns_dev, info->netns_ino);
> >       printf("%s", info->gpl_compatible ? "  gpl" : "");
> > +     if (info->runtime)
> > +             printf(" runtime %lld runcnt %lld",
> > +                    info->runtime, info->runcnt);
> >       printf("\n");
> >
> >       if (info->load_time) {
> >
>