Message-ID: <930f9848-5037-df52-5867-ff6fba3eecfe@fb.com>
Date:   Wed, 12 Sep 2018 21:57:29 -0700
From:   Yonghong Song <yhs@...com>
To:     Daniel Borkmann <daniel@...earbox.net>, <ast@...com>,
        <netdev@...r.kernel.org>
CC:     <kernel-team@...com>
Subject: Re: [PATCH bpf-next 4/4] tools/bpf: bpftool: add net support



On 9/12/18 3:29 PM, Daniel Borkmann wrote:
> On 09/06/2018 01:58 AM, Yonghong Song wrote:
>> Add "bpftool net" support. Networking devices are enumerated
>> to dump device index/name associated with xdp progs.
>>
>> For each networking device, tc classes and qdiscs are enumerated
>> in order to check their bpf filters.
>> In addition, root handle and clsact ingress/egress are also checked for
>> bpf filters.  Not all filter information is printed out. Only ifindex,
>> kind, filter name, prog_id and tag are printed out, which are good
>> enough to show attachment information. If the filter action
>> is a bpf action, its bpf program id, bpf name and tag will be
>> printed out as well.
>>
>> For example,
>>    $ ./bpftool net
>>    xdp [
>>    ifindex 2 devname eth0 prog_id 198
>>    ]
> 
> Could we make the output more terse? E.g. the 'ifindex' and 'devname' is basically
> zero information but will take lots of space. 'eth0 (2)' would for example make it
> shorter. Also info is missing whether the attached prog is driver/hw/generic XDP. :(

Right 'eth0 (2)' is a good idea. Similarly to other bpftool plain 
output, agree we should have concise printout.
For the above xdp output, I guess I tested on an old kernel and will 
test on newer kernel to ensure it works properly.

> 
>>    tc_filters [
>>    ifindex 2 kind qdisc_htb name prefix_matcher.o:[cls_prefix_matcher_htb]
>>              prog_id 111727 tag d08fe3b4319bc2fd act []
>>    ifindex 2 kind qdisc_clsact_ingress name fbflow_icmp
>>              prog_id 130246 tag 3f265c7f26db62c9 act []
>>    ifindex 2 kind qdisc_clsact_egress name prefix_matcher.o:[cls_prefix_matcher_clsact]
>>              prog_id 111726 tag 99a197826974c876
>>    ifindex 2 kind qdisc_clsact_egress name cls_fg_dscp
>>              prog_id 108619 tag dc4630674fd72dcc act []
>>    ifindex 2 kind qdisc_clsact_egress name fbflow_egress
>>              prog_id 130245 tag 72d2d830d6888d2c
>>    ]
> 
> Similar comment here. Do we need the tag here? I think it's not really needed, e.g.
> the output of bpftool perf [0] doesn't provide it either and therefore makes the list
> as nice one-liners, so overview is much nicer there. Is there a reason that tc progs
> do not show dev name as opposed to xdp progs? Can we shorten everything to make it
> a one-liner like in bpftool perf?

Yes, we should remove 'tag'. Users can use prog_id to find tag.
There is no IFNAME attribute in the tc filter return message.
But I already got them in previous iplink message, so I can
print out here.

> Should we have a small indicator here if the tc prog was offloaded?

Not sure about this one. As you suggested in the next email, we
can have a message like if users want more information they
can use more specific tools 'ip link ...', 'tc filter ...' etc.

> 
> Does the dump work with tc shared blocks?

I does not.I did not have experiences with tc shared block and that is 
why I did not add support for it.

> 
> Should we also dump networking related cgroup BPF progs here under bpftool net?

probably not, but in additional to the above suggestions about using 
different tools, we can add a message to suggest check `bpftool cgroup 
tree` for cgroup related net programs.

> 
> Thanks,
> Daniel
> 
>    [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b04df400c30235fa347313c9e2a0695549bd2c8e
> 

Powered by blists - more mailing lists