From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [patch net-next v4 05/10] net: sched: keep track of offloaded filters and check tc offload feature Date: Sat, 23 Dec 2017 18:20:45 -0800 Message-ID: <20171223182045.610b04e4@cakuba.netronome.com> References: <20171223155436.9014-1-jiri@resnulli.us> <20171223155436.9014-6-jiri@resnulli.us> 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, 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: Jiri Pirko Return-path: Received: from mx3.wp.pl ([212.77.101.10]:34675 "EHLO mx3.wp.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751040AbdLXCU4 (ORCPT ); Sat, 23 Dec 2017 21:20:56 -0500 In-Reply-To: <20171223155436.9014-6-jiri@resnulli.us> Sender: netdev-owner@vger.kernel.org List-ID: 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? 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..