Date:   Wed, 18 Apr 2018 10:46:46 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Daniel Borkmann <daniel@...earbox.net>
CC:     <netdev@...r.kernel.org>, Alexei Starovoitov <ast@...com>,
        <kernel-team@...com>, <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH bpf-next v4 07/10] bpf: btf: Add pretty print support to
 the basic arraymap

On Wed, Apr 18, 2018 at 05:20:11PM +0200, Daniel Borkmann wrote:
> Hi Martin,
> 
> first of all great work on the set! One issue that puzzled me
> while digesting it further below.
> 
> On 04/17/2018 10:42 PM, Martin KaFai Lau wrote:
> > This patch adds pretty print support to the basic arraymap.
> > Support for other bpf maps can be added later.
> > 
> > This patch adds new attrs to the BPF_MAP_CREATE command to allow
> > specifying the btf_fd, btf_key_id and btf_value_id.  The
> > BPF_MAP_CREATE can then associate the btf to the map if
> > the creating map supports BTF.
> 
> Feels like this patch is doing two things at once, meaning i)
> attaching btf object to map object through bpf syscall at map
> creation time, and ...
> 
> > A BTF supported map needs to implement two new map ops,
> > map_seq_show_elem() and map_check_btf().  This patch has
> > implemented these new map ops for the basic arraymap.
> > 
> > It also adds file_operations to the pinned map
> > such that the pinned map can be opened and read.
> 
> ... ii) pretty print map dump via bpf fs for array map.
> 
> > Here is a sample output when reading a pinned arraymap
> > with the following map's value:
> > 
> > struct map_value {
> > 	int count_a;
> > 	int count_b;
> > };
> > 
> > cat /sys/fs/bpf/pinned_array_map:
> > 
> > 0: {1,2}
> > 1: {3,4}
> > 2: {5,6}
> > ...
> 
> Rather than adding this to the bpf fs itself, why not add full BTF
> support for the main debugging and introspection tool we have and
> ship with the kernel for BPF, namely bpftool? You can already dump
> the whole map's key/value pairs via the following command from a
> pinned file:
> 
>   bpftool map dump pinned /sys/fs/bpf/pinned_array_map
> 
> And given we already export the BTF info in your earlier patch through
> the BPF_OBJ_GET_INFO_BY_FD, this would fit perfectly for bpftool
> integration instead where the pretty-print which is done through the
> extra cb map_seq_show_elem (which only does a map lookup and print
> anyway) in this patch can simply all be done in user space w/o any
> additional kernel complexity.
> 
> Aside that this would be very valuable there it would also nicely
> demonstrate usage of it, but more importantly we could avoid implementing
> such pretty-print callback in the kernel for every other map type and
> then having two locations where a user now needs to go for debugging
> (bpftool being one, and cat of pinned file the other; this split seems
> confusing from a user perspective, imho, but also single key lookup +
> pretty-print cannot be realized with the latter whereas it's trivial
> with bpftool).
> 
> The same could be done for bpftool map lookup, updates, deletions, etc
> where the key resp. key/value pair can be specified through a struct
> like initializer from cmdline. (But dump/lookup should be good enough
> starting point initially.) Thoughts?
Right, I have plan on the kernel side to allow that in bpftool.
There are a couple steps to be done first to allow getting the
BTF from a map-fd.

My idea is to have both (bpf fs, and userspace tool like bpftool).
If the user pinned it, it is more intuitive for cat/bash like
user to be able to cat it directly.  This 'cat bpffs/pathto/map'
would eventually be available to all map types with or without
map_lookup_elem support, as long as the map-type makes sense
for pretty print.

Going forward, bpf tool can do a more demanding jobs you mentioned.

> 
> Thanks again,
> Daniel
> 
> > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > Acked-by: Alexei Starovoitov <ast@...com>
> > ---
> >  include/linux/bpf.h      |  20 ++++++-
> >  include/uapi/linux/bpf.h |   3 +
> >  kernel/bpf/arraymap.c    |  50 ++++++++++++++++
> >  kernel/bpf/inode.c       | 146 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  kernel/bpf/syscall.c     |  32 ++++++++++-
> >  5 files changed, 244 insertions(+), 7 deletions(-)
> 

Powered by blists - more mailing lists