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?
Thanks again,
Daniel
> Signed-off-by: Martin KaFai Lau <ka...@fb.com>
> Acked-by: Alexei Starovoitov <a...@fb.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(-)
