On Sun, May 3, 2020 at 5:48 AM Jamal Hadi Salim <j...@mojatatu.com> wrote: > > On 2020-05-03 8:02 a.m., Jamal Hadi Salim wrote: > > 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". > > Ok, I went backwards and looked at many kernel sources. > It is true we install the filters in two different locations > i.e just specifying TC_H_ROOT does not equate to picking > the egress qdisc with that flag. > And has been broken for way too long - so we have to live > with it. > I wish we had more tdc tests and earlier. > > Advise to users is not to use semantics like "root" or ingress > but rather explicitly specify the parent.
Yeah, I was confused too when my colleagues reported this to me. > > So ignore what i said above. I will ACK your patch. Thanks for review!
