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 <y...@fb.com>
