Date:   Tue, 24 Oct 2017 09:01:34 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Jiri Pirko <jiri@...nulli.us>, Netdev <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>, mlxsw@...lanox.com,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Ganesh Goudar <ganeshgr@...lsio.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Matan Barak <matanb@...lanox.com>,
        Leon Romanovsky <leonro@...lanox.com>, idosch@...lanox.com,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Simon Horman <simon.horman@...ronome.com>,
        pieter.jansenvanvuuren@...ronome.com, john.hurley@...ronome.com,
        "Duyck, Alexander H" <alexander.h.duyck@...el.com>,
        Amritha Nambiar <amritha.nambiar@...el.com>
Subject: Re: [patch net-next v2 00/20] net: sched: convert cls ndo_setup_tc
 offload calls to per-block callbacks

On Fri, Oct 20, 2017 at 7:04 PM, David Miller <davem@...emloft.net> wrote:
> From: Jiri Pirko <jiri@...nulli.us>
> 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’t 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

Powered by blists - more mailing lists