From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH net] net: sched: cls: Fix offloading when ingress dev is vxlan Date: Wed, 6 Jun 2018 08:46:15 -0700 Message-ID: <20180606084615.6a7f2780@cakuba.netronome.com> References: <1528185843-18645-1-git-send-email-paulb@mellanox.com> <20180605115747.0e939ac4@cakuba.netronome.com> <20180605.150640.12956507851401975.davem@davemloft.net> <20180605142700.6033a7a0@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: David Miller , Paul Blakey , Jiri Pirko , Cong Wang , Jamal Hadi Salim , Linux Netdev List , Yevgeny Kliteynik , Roi Dayan , Shahar Klein , Mark Bloch , Or Gerlitz To: Or Gerlitz Return-path: Received: from mx4.wp.pl ([212.77.101.11]:47020 "EHLO mx4.wp.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752067AbeFFPqZ (ORCPT ); Wed, 6 Jun 2018 11:46:25 -0400 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Wed, 6 Jun 2018 08:15:27 +0300, Or Gerlitz wrote: > On Wed, Jun 6, 2018 at 12:27 AM, Jakub Kicinski wrote: > > On Tue, 05 Jun 2018 15:06:40 -0400 (EDT), David Miller wrote: > >> From: Jakub Kicinski > >> Date: Tue, 5 Jun 2018 11:57:47 -0700 > >> > >> > Do we still care about correctness and not breaking backward > >> > compatibility? > >> > >> Jakub let me know if you want me to revert this change. > > > > Yes, I think this patch introduces a regression when block is shared > > between offload capable and in-capable device, therefore it should be > > reverted. Shared blocks went through a number of review cycles to > > ensure such cases are handled correctly. > > > > > > Longer story about the actual issue which is never explained in the > > commit message is this: in kernels 4.10 - 4.14 skip_sw flag was > > supported on tunnels in cls_flower only: > > > > static int fl_hw_replace_filter(struct tcf_proto *tp, > > [...] > > if (!tc_can_offload(dev)) { > > if (tcf_exts_get_dev(dev, &f->exts, &f->hw_dev) || > > (f->hw_dev && !tc_can_offload(f->hw_dev))) { > > f->hw_dev = dev; > > return tc_skip_sw(f->flags) ? -EINVAL : 0; > > } > > dev = f->hw_dev; > > cls_flower.egress_dev = true; > > } else { > > f->hw_dev = dev; > > } > > > > > > In 4.15 - 4.17 with addition of shared blocks egdev mechanism was > > promoted to a generic TC thing supported on all filters but it no > > longer works with skip_sw. > > > > I'd argue skip_sw is incorrect for tunnels, because the rule will only > > apply to traffic ingressing on ASIC ports, not all traffic which hits > > the tunnel netdev. > > This argument makes sense, however, skip_sw for tunnel decap rules > **is** allowed since 4.10 and we have some sort of regression here (turns > out that before and after the patch..) As I said it was allowed in 4 releases, which was a mistake, in last 3 it wasn't. I understand your use case, but the semantics of skip_sw are not preserved here so we should find a different solution. > > Therefore we should keep the 4.15 - 4.17 behaviour. > > But that's a side note, I don't think we should be breaking offload on > > shared blocks whether we decide to support skip_sw on tunnels or not. > > skip_sw on tunnels was there before shared-block, newer features should > take care not to break existing ones. Oh, I agree we shouldn't break existing use cases so please don't break the use case I mentioned above. I want to set up shared block between a LAG and its members. Now since the bond will not be offload-capable TC will not even make an attempt to offload to members. I'm gonna test that my reading of the code is correct and send a revert later today, sorry.