From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [PATCH net 00/13] cls_u32 cleanups and fixes. Date: Sun, 9 Sep 2018 15:15:38 +0100 Message-ID: <20180909141538.GG19965@ZenIV.linux.org.uk> References: <20180909013132.3222-1-viro@ZenIV.linux.org.uk> <5f835727-3f86-1532-889f-d11ae17e16d4@mojatatu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, Cong Wang , Jiri Pirko To: Jamal Hadi Salim Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:50726 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727080AbeIITFa (ORCPT ); Sun, 9 Sep 2018 15:05:30 -0400 Content-Disposition: inline In-Reply-To: <5f835727-3f86-1532-889f-d11ae17e16d4@mojatatu.com> Sender: netdev-owner@vger.kernel.org List-ID: 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?