Re: [PATCH bpf-next] tools/bpf: bpftool: improve output format for bpftool net

[Yonghong Song <yhs@fb.com>]

On 9/17/18 3:19 AM, Daniel Borkmann wrote:
> On 09/14/2018 11:49 PM, Yonghong Song wrote:
>> This is a followup patch for Commit f6f3bac08ff9
>> ("tools/bpf: bpftool: add net support").
>> Some improvements are made for the bpftool net output.
>> Specially, plain output is more concise such that
>> per attachment should nicely fit in one line.
>> Compared to previous output, the prog tag is removed
>> since it can be easily obtained with program id.
>> Similar to xdp attachments, the device name is added
>> to tc_filters attachments.
>>
>> The bpf program attached through shared block
>> mechanism is supported as well.
>>    $ ip link add dev v1 type veth peer name v2
>>    $ tc qdisc add dev v1 ingress_block 10 egress_block 20 clsact
>>    $ tc qdisc add dev v2 ingress_block 10 egress_block 20 clsact
>>    $ tc filter add block 10 protocol ip prio 25 bpf obj bpf_shared.o sec ingress flowid 1:1
>>    $ tc filter add block 20 protocol ip prio 30 bpf obj bpf_cyclic.o sec classifier flowid 1:1
>>    $ bpftool net
>>    xdp [
>>    ]
>>    tc_filters [
>>     v2(7) qdisc_clsact_ingress bpf_shared.o:[ingress] id 23
>>     v2(7) qdisc_clsact_egress bpf_cyclic.o:[classifier] id 24
>>     v1(8) qdisc_clsact_ingress bpf_shared.o:[ingress] id 23
>>     v1(8) qdisc_clsact_egress bpf_cyclic.o:[classifier] id 24
> 
> Just one minor note for this one here, do we even need the "qdisc_" prefix? Couldn't it just simply
> be "clsact/ingress", "clsact/egress", "htb" etc?

Will do.

> 
>>    ]
>>
>> The documentation and "bpftool net help" are updated
>> to make it clear that current implementation only
>> supports xdp and tc attachments. For programs
>> attached to cgroups, "bpftool cgroup" can be used
>> to dump attachments. For other programs e.g.
>> sk_{filter,skb,msg,reuseport} and lwt/seg6,
>> iproute2 tools should be used.
>>
>> The new output:
>>    $ bpftool net
>>    xdp [
>>     eth0(2) id/drv 198
> 
> Could we change the "id/{drv,offload,generic} xyz" into e.g. "eth0(2) {driver,offload,generic} id 198",
> meaning, the "id xyz" being a child of either "driver", "offload" or "generic". Reason would be two-fold:
> i) we can keep the "id xyz" notion consistent as used under "tc_filters", and ii) it allows to put further
> information aside from just "id" member under "driver", "offload" or "generic" in the future.

Will do.

> 
>>    ]
>>    tc_filters [
> 
> Nit: can we use just "tc" for the above? Main use case would be clsact with one of its two hooks anyway,
> and the term "filter" is sort of tc historic; while being correct bpf progs would do much more than just
> filtering, and context is pretty clear anyway from qdisc that we subsequently dump.

Make sense.

Will address all these comments and submit a revision soon. Thanks!

> 
>>     eth0(2) qdisc_clsact_ingress fbflow_icmp id 335 act [{icmp_action id 336}]
>>     eth0(2) qdisc_clsact_egress fbflow_egress id 334
>>    ]
>>    $ bpftool -jp net
>>    [{
>>          "xdp": [{
>>                  "devname": "eth0",
>>                  "ifindex": 2,
>>                  "id/drv": 198
>>              }
>>          ],
>>          "tc_filters": [{
>>                  "devname": "eth0",
>>                  "ifindex": 2,
>>                  "kind": "qdisc_clsact_ingress",
>>                  "name": "fbflow_icmp",
>>                  "id": 335,
>>                  "act": [{
>>                          "name": "icmp_action",
>>                          "id": 336
>>                      }
>>                  ]
>>              },{
>>                  "devname": "eth0",
>>                  "ifindex": 2,
>>                  "kind": "qdisc_clsact_egress",
>>                  "name": "fbflow_egress",
>>                  "id": 334
>>              }
>>          ]
>>      }
>>    ]
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>