On 2020-05-02 10:28 p.m., Cong Wang wrote:
On Sat, May 2, 2020 at 2:19 AM Jamal Hadi Salim <j...@mojatatu.com> wrote:
On 2020-05-02 4:48 a.m., Jamal Hadi Salim wrote:
[..]
Note:
tc filter show dev dummy0 root
should not show that filter. OTOH,
tc filter show dev dummy0 parent ffff:
should.
Hmm, but we use TC_H_MAJ(tcm->tcm_parent) to do the
lookup, 'root' (ffff:ffff) has the same MAJ with ingress
(ffff:0000).
I have some long analysis and theory below.
And qdisc_lookup() started to search for ingress since 2008,
commit 8123b421e8ed944671d7241323ed3198cccb4041.
So it is likely too late to change this behavior even if it is not
what we prefer.
My gut feeling is that whatever broke (likely during block addition
maybe even earlier during clsact addition) is in the code
path for adding filter. Dumping may have bugs but i would
point a finger to filter addition first.
More below.... (sorry long email).
Here's what i tried after applying your patch:
----
# $TC qd add dev $DEV ingress
# $TC qd add dev $DEV root prio
# $TC qd ls dev $DEV
qdisc noqueue 0: dev lo root refcnt 2
qdisc prio 8008: dev enp0s1 root refcnt 2 bands 3 priomap 1 2 2 2 1 2 0
0 1 1 1 1 1 1 1 1
qdisc ingress ffff: dev enp0s1 parent ffff:fff1 ----------------
-----
egress i.e root is at 8008:
ingress is at ffff:fff1
If say:
---
# $TC filter add dev $DEV root protocol arp prio 10 basic action pass
----
i am instructing the kernel to "go and find root (which is 8008:)
and install the filter there".
IOW, I could install that filter alternatively as:
----
# $TC filter add dev $DEV parent 8008: protocol arp prio 11 basic action
pass
---
Basically these two filters are equivalent and should end in the
same qdisc.
To test, I added those two filters (the prio is useful to visualize
in the dump).
Lets see the dump:
-------
# $TC filter show dev $DEV root
filter protocol arp pref 10 basic chain 0
filter protocol arp pref 10 basic chain 0 handle 0x1
action order 1: gact action pass
random type none pass val 0
index 1 ref 1 bind 1
---------
I was hoping i'd see both filters, but alas there's only
one.
Lets try a different dump explicitly specifying root qdisc id:
---------
# $TC filter show dev $DEV parent 8008:
filter protocol arp pref 11 basic chain 0
filter protocol arp pref 11 basic chain 0 handle 0x1
action order 1: gact action pass
random type none pass val 0
index 2 ref 1 bind 1
---------
Again i was hoping to see both filters.
This sounds like the two filters are anchored
at two different qdiscs instead of the same one
(i.e root). Hence my suspicion...
Lets add a filter at ingress:
-----
$TC filter add dev $DEV parent ffff:fff1 protocol arp basic action pass
-------
Ok lets dump this from ingress:
-----
# $TC filter show dev $DEV parent ffff:fff1
filter protocol arp pref 10 basic chain 0
filter protocol arp pref 10 basic chain 0 handle 0x1
action order 1: gact action pass
random type none pass val 0
index 1 ref 1 bind 1
filter protocol arp pref 49152 basic chain 0
filter protocol arp pref 49152 basic chain 0 handle 0x1
action order 1: gact action pass
random type none pass val 0
index 3 ref 1 bind 1
-------------
Same result if i said "root".
I was only expecting to see the one with pref 49152 in
the above output.
It _feels_ like those two filters(ingress and egress) are
installed in the same struct.
Ok, last dump without specifying a parent, which should
pick root qdisc per code:
------
# $TC filter show dev $DEV
filter parent 8008: protocol arp pref 11 basic chain 0
filter parent 8008: protocol arp pref 11 basic chain 0 handle 0x1
action order 1: gact action pass
random type none pass val 0
index 2 ref 1 bind 1
----
Semantically this should have dumped all 3 filters and
not just pick root. But that issue has been there
for a decade like you said. So it is reasonable to specify
parent ffff:fff1 for dumping just ingress side.
Also reasonable not to see ingress when dumping root.
If parent is not specified, only egress will be shown, as
we just assign q = dev->qdisc.
Which is the root egress qdisc.
That is the "$TC filter show dev $DEV" scenario.
See my comment above.
I agree, 'root' should mean the root qdisc on egress, matching
ingress with 'root' doesn't make much sense to me either.
But I am afraid it is too late to change ,if this behavior has been
there for 12+ years.
Although i cant pinpoint exactly when - this used to work (I dont think
its 12+ years but I could be wrong). These semantics are really broken.
Do you have time to look at the theory that things break at install?
If you dont have time i could try to debug it by Tuesday.
cheers,
jamal
