From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [patch net-next v4 05/10] net: sched: keep track of offloaded filters and check tc offload feature Date: Sun, 24 Dec 2017 08:52:33 +0100 Message-ID: <20171224075233.GB1883@nanopsycho> References: <20171223155436.9014-1-jiri@resnulli.us> <20171223155436.9014-6-jiri@resnulli.us> <20171223182045.610b04e4@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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, michael.chan@broadcom.com, ganeshgr@chelsio.com, saeedm@mellanox.com, matanb@mellanox.com, leonro@mellanox.com, idosch@mellanox.com, simon.horman@netronome.com, pieter.jansenvanvuuren@netronome.com, john.hurley@netronome.com, alexander.h.duyck@intel.com, ogerlitz@mellanox.com, john.fastabend@gmail.com, daniel@iogearbox.net, dsahern@gmail.com To: Jakub Kicinski Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:35026 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750805AbdLXHwf (ORCPT ); Sun, 24 Dec 2017 02:52:35 -0500 Received: by mail-wm0-f68.google.com with SMTP id f9so28261702wmh.0 for ; Sat, 23 Dec 2017 23:52:35 -0800 (PST) Content-Disposition: inline In-Reply-To: <20171223182045.610b04e4@cakuba.netronome.com> Sender: netdev-owner@vger.kernel.org List-ID: Sun, Dec 24, 2017 at 03:20:45AM CET, kubakici@wp.pl wrote: >On Sat, 23 Dec 2017 16:54:31 +0100, Jiri Pirko wrote: >> -static void tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, >> - struct tcf_block_ext_info *ei) >> +static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, >> + struct tcf_block_ext_info *ei) >> { >> - tcf_block_offload_cmd(block, q, ei, TC_BLOCK_BIND); >> + struct net_device *dev = q->dev_queue->dev; >> + int err; >> + >> + if (!dev->netdev_ops->ndo_setup_tc) >> + return 0; >> + >> + /* If tc offload feature is disabled and the block we try to bind >> + * to already has some offloaded filters, forbid to bind. >> + */ >> + if (!tc_can_offload(dev) && tcf_block_offload_in_use(block)) >> + return -EOPNOTSUPP; >> + >> + err = tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_BIND); >> + if (err == -EOPNOTSUPP) >> + /* Driver does not support binding. */ >> + return 0; >> + return err; >> } > >Would you mind explaining why those return 0s are safe? > >Say I have 2 netdevs, one dumb NIC without ndo_setup_tc (dnic) and one >NIC that can offload everything (enic). I can share a block between >them (before or after adding any filters) and adding filters with >skip_sw will succeed, even though dnic will not see them ever. There >is only one callback for enic, "all callbacks" != "all devices". >It's fine to share the block in such case, but that block can never >accept a skip_sw filter. Don't we need something like (reverse) patch 3 >for keeping track of netdevs sharing the block which are not OK with >offloads? > >Am I misunderstanding how this is supposed to work? Or simply too nit >picky about providing predictable behaviour? You undestand it correctly. Original plan was to ignore thore devices that does not support offloading. But thinking about it a bit more, you are probably right that they should be taken into consideration when user explicitly says "skip_sw". Will include the accounting you suggest below. Thanks! > >Quick hack to illustrate the idea (untested): > >diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h >index 22a3a1d22ffa..e61e59161243 100644 >--- a/include/net/sch_generic.h >+++ b/include/net/sch_generic.h >@@ -289,6 +289,7 @@ struct tcf_block { > struct list_head cb_list; > struct list_head owner_list; > bool keep_dst; >+ bool nonoffload_taint; > unsigned int offloadcnt; > }; > >diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >index 37eea70d1d72..4e017cbbf356 100644 >--- a/net/sched/cls_api.c >+++ b/net/sched/cls_api.c >@@ -290,7 +290,7 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > int err; > > if (!dev->netdev_ops->ndo_setup_tc) >- return 0; >+ goto mark_no_offload; > > /* If tc offload feature is disabled and the block we try to bind > * to already has some offloaded filters, forbid to bind. >@@ -300,9 +300,14 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q, > > err = tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_BIND); > if (err == -EOPNOTSUPP) >- /* Driver does not support binding. */ >- return 0; >+ goto mark_no_offload; > return err; >+ >+mark_no_offload: >+ if (tcf_block_offload_in_use(block)) >+ return -EOPNOTSUPP; >+ block->nonoffload_taint = true; >+ return 0; > } > > static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q, >@@ -1492,6 +1497,10 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts, > int ok_count; > int ret; > >+ /* Make sure all netdevs sharing this block are offload-capable */ >+ if (block->nonoffload_taint && err_stop) >+ return -EOPNOTSUPP; >+ > ret = tcf_block_cb_call(block, type, type_data, err_stop); > if (ret < 0) > return ret; > > >Here a block once tainted with a bad netdev will never be offloadable >again, so tracking a'la patch 3 would be nicer..