From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [patch net-next v2 01/10] cls_bpf: move prog offload->netdev check into drivers Date: Mon, 13 Nov 2017 00:12:35 -0800 Message-ID: <20171113001235.5d4f4262@cakuba> References: <20171112155604.24061-1-jiri@resnulli.us> <20171112155604.24061-2-jiri@resnulli.us> <20171112181418.3ca6ff54@cakuba> <20171113062538.GA1986@nanopsycho> <20171112231734.57e651d4@cakuba> <20171113075556.GD1986@nanopsycho> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, davem@davemloft.net, jhs@mojatatu.com, xiyou.wangcong@gmail.com, mlxsw@mellanox.com, andrew@lunn.ch, vivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com, ast@kernel.org, daniel@iogearbox.net, simon.horman@netronome.com, pieter.jansenvanvuuren@netronome.com, john.hurley@netronome.com To: Jiri Pirko Return-path: Received: from mail-pg0-f54.google.com ([74.125.83.54]:50805 "EHLO mail-pg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751978AbdKMIMj (ORCPT ); Mon, 13 Nov 2017 03:12:39 -0500 Received: by mail-pg0-f54.google.com with SMTP id y5so12131210pgq.7 for ; Mon, 13 Nov 2017 00:12:39 -0800 (PST) In-Reply-To: <20171113075556.GD1986@nanopsycho> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 13 Nov 2017 08:55:56 +0100, Jiri Pirko wrote: > Mon, Nov 13, 2017 at 08:17:34AM CET, jakub.kicinski@netronome.com wrote: > >On Mon, 13 Nov 2017 07:25:38 +0100, Jiri Pirko wrote: > >> Mon, Nov 13, 2017 at 03:14:18AM CET, jakub.kicinski@netronome.com wrote: > >> >On Sun, 12 Nov 2017 16:55:55 +0100, Jiri Pirko wrote: > >> >> From: Jiri Pirko > >> >> > >> >> In order to remove tp->q usage in cls_bpf, the offload->netdev check > >> >> needs to be moved to individual drivers as only they will have access > >> >> to appropriate struct net_device. > >> >> > >> >> Signed-off-by: Jiri Pirko > >> > > >> >This seems not entirely correct and it adds unnecessary code. I think > >> > >> What is not correct? > > > >From quick reading it looks like you will allow to install the > >dev-specific filter without skip_sw flag. You haven't fixed what > > Right. I see it now. > > > >your previous series broke in cls_bpf offload model and now you > > What do you mean exactly? As explained elsewhere, cls_bpf used to track what's offloaded and issue ADD/REPLACE/DESTORY accordingly. Now drivers need to know what they're offloading, but they still don't. So if you add a filter that offload successfully and then one that doesn't, the spurious DESTORY will kill the wrong offload. > >break it even further. > > > >> >the XDP and cls_bpf handling could be unified, making way for binding > >> >the same program to multiple ports of the same device. Would you mind > >> >waiting a day for me to send corrections to BPF offload? > >> > >> Well I'm trying to get this in before net-next closes... > > > >Right, and I'm surprised by that. I'd hope you'll understand my caution > >here given recent history. > > Sure. I looked through this series and I can't grasp all the details of how things are supposed to work from the code here :( Perhaps important bits went in earlier and I missed them. Starting from the most fundamental thing - if I have a shared block full of skip_sw filters and then bind it to a device which doesn't even have ndo_setup_tc - what prevents that from happening? AFACT tcf_block_offload_cmd() is returning void.