From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc offload calls to per-block callbacks Date: Tue, 24 Oct 2017 09:24:01 -0700 Message-ID: References: <20171019135048.4306-1-jiri@resnulli.us> <20171021.030446.967534081159629805.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: Jiri Pirko , Netdev , Jamal Hadi Salim , Cong Wang , mlxsw@mellanox.com, Andrew Lunn , Vivien Didelot , Florian Fainelli , Michael Chan , Ganesh Goudar , Jeff Kirsher , Saeed Mahameed , Matan Barak , Leon Romanovsky , idosch@mellanox.com, Jakub Kicinski , Alexei Starovoitov , Daniel Borkmann , Simon Horman , pieter.jansenvanvuuren@netronome.com, john.hurley@netronome. To: David Miller Return-path: Received: from mail-qt0-f196.google.com ([209.85.216.196]:49066 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751369AbdJXQYD (ORCPT ); Tue, 24 Oct 2017 12:24:03 -0400 Received: by mail-qt0-f196.google.com with SMTP id f8so31158809qta.5 for ; Tue, 24 Oct 2017 09:24:02 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Oct 24, 2017 at 9:01 AM, Alexander Duyck wrote: > On Fri, Oct 20, 2017 at 7:04 PM, David Miller wrote= : >> From: Jiri Pirko >> Date: Thu, 19 Oct 2017 15:50:28 +0200 >> >>> This patchset is a bit bigger, but most of the patches are doing the >>> same changes in multiple classifiers and drivers. I could do some >>> squashes, but I think it is better split. >>> >>> This is another dependency on the way to shared block implementation. >>> The goal is to remove use of tp->q in classifiers code. >>> >>> Also, this provides drivers possibility to track binding of blocks to >>> qdiscs. Legacy drivers which do not support shared block offloading. >>> register one callback per binding. That maintains the current >>> functionality we have with ndo_setup_tc. Drivers which support block >>> sharing offload register one callback per block which safes overhead. >>> >>> Patches 1-4 introduce the binding notifications and per-block callbacks >>> Patches 5-8 add block callbacks calls to classifiers >>> Patches 9-17 do convert from ndo_setup_tc calls to block callbacks for >>> classifier offloads in drivers >>> Patches 18-20 do cleanup >> >> Series applied. > > We are getting internal reports of regressions being seen with this > patch set applied. Specifically the issues below have been pointed out > to me. My understanding is that these issues are all being reported on > ixgbe: > > 1. To offload filter into HW, the hw-tc-offload feature flag has > to be turned on before creating the ingress qdisc. > > Previously, this could also be turned on after the qdisc was created > and the filters could still be offloaded. Looks like this is because, > previously the offload flag was checked as a part of filter > integration in the classifier, and now it is checked as part of qdisc > creation (ingress_init). So, if no offload capability is advertised at > ingress qdisc creation time then hardware will not be asked to offload > filters later if the flag is enabled. > > 2. Deleting the ingress qdisc fails to remove filters added in > HW. Filters in SW gets deleted. > > We haven=E2=80=99t exactly root-caused this, the changes being extensive,= but > our guess is again something wrong with the offload check or similar > while unregistering the block callback (tcf_block_cb_unregister) and > further to the classifier (CLS_U32/CLS_FLOWER etc.) with the > DESTROY/REMOVE command. > > 3. Deleting u32 filters using handle fails to remove filter from > HW, filter in SW gets deleted. > > Probably similar reasons, also we see some u32 specific patches as > well for remove nodes. > > We are still digging into this further, but wanted to put this out > there so we can address the issues before we go much further down this > path. > > Thanks. > > - Alex So a quick update. Item 3 is no longer an issue, it was a configuration issue on our side. So only items 1 and 2 are left to be addressed. Thanks. - Alex