On Sun, Sep 09, 2018 at 08:58:50AM -0400, Jamal Hadi Salim wrote: > > Since you have the momentum here: i noticed something > unusual while i was trying to craft a test that would > vet some of your changes. This has nothing to do with > your changes, same happens on my stock debian laptop > with kernel: > 4.17.0-0.bpo.3-amd64 #1 SMP Debian 4.17.17-1~bpo9+1 (2018-08-27) > > Looking at git - possibly introduced around the time u32 > lockless was being introduced and maybe even earlier > than that.
It's always been that way, actually - before that point the old knode simply got reused, which excluded any chance of changing n->sel. > Unfortunately i dont have time to dig > further. > > To reproduce what i am referring to, here's a setup: > > $tc filter add dev eth0 parent ffff: protocol ip prio 102 u32 \ > classid 1:2 match ip src 192.168.8.0/8 > $tc filter replace dev eth0 parent ffff: protocol ip prio 102 \ > handle 800:0:800 u32 classid 1:2 match ip src 1.1.0.0/24 > > u32_change() code path should have allowed changing of the > keynode. Umm... Interesting - TCA_U32_SEL is not the only thing that gets ignored there; TCA_U32_MARK gets the same treatment. And then there's a lovely question what to do with n->pf - it's an array of n->sel.nkeys counters, and apparently we want (at least in common cases) to avoid resetting those. *If* we declare that ->nkeys mismatch means failure, it's all relatively easy to implement. Alternatively, we could declare that selector change means resetting the stats. Preferences?
