Re: [PATCH bpf-next v4 07/10] bpf: btf: Add pretty print support to the basic arraymap

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Wed, 18 Apr 2018 17:20:11 +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?

I felt the same way but I'm obviously biased so I didn't say anything ;)

I'm concerned about exposing the map data in this arbitrary format.
This will be a kernel ABI.  And the format even though it looks nice
and concise for an array may not be as good for more complex maps where
key is not an int.  Will key for hashes be specified as fields in {}?
Why the discrepancy between array and hash then (array's key isn't)?
This print type is also not any standard notation AFAIU.  In bpftool
user can just ask to render JSON, including actual field names.  Or we
can add other formats if we feel like or change the default one, it's
not kernel ABI.  I'd hate to see people deploying regexps to parse the
output of pinned map dump when there is modern tooling available...

How useful is it really to provide a dump on a pinned file?  I guess
this is mostly for beginners?  Is this really more handy than bpftool?