From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8B442C433E0 for ; Fri, 10 Jul 2020 15:26:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5FCC720720 for ; Fri, 10 Jul 2020 15:26:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727861AbgGJP0y (ORCPT ); Fri, 10 Jul 2020 11:26:54 -0400 Received: from correo.us.es ([193.147.175.20]:56720 "EHLO mail.us.es" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726925AbgGJP0y (ORCPT ); Fri, 10 Jul 2020 11:26:54 -0400 Received: from antivirus1-rhel7.int (unknown [192.168.2.11]) by mail.us.es (Postfix) with ESMTP id 071F7508CDB for ; Fri, 10 Jul 2020 17:26:52 +0200 (CEST) Received: from antivirus1-rhel7.int (localhost [127.0.0.1]) by antivirus1-rhel7.int (Postfix) with ESMTP id DA661DA84A for ; Fri, 10 Jul 2020 17:26:51 +0200 (CEST) Received: by antivirus1-rhel7.int (Postfix, from userid 99) id C8158DA7B6; Fri, 10 Jul 2020 17:26:51 +0200 (CEST) Received: from antivirus1-rhel7.int (localhost [127.0.0.1]) by antivirus1-rhel7.int (Postfix) with ESMTP id 57397DA78D; Fri, 10 Jul 2020 17:26:49 +0200 (CEST) Received: from 192.168.1.97 (192.168.1.97) by antivirus1-rhel7.int (F-Secure/fsigk_smtp/550/antivirus1-rhel7.int); Fri, 10 Jul 2020 17:26:49 +0200 (CEST) X-Virus-Status: clean(F-Secure/fsigk_smtp/550/antivirus1-rhel7.int) Received: from us.es (unknown [90.77.255.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: 1984lsi) by entrada.int (Postfix) with ESMTPSA id 1789D4265A2F; Fri, 10 Jul 2020 17:26:49 +0200 (CEST) Date: Fri, 10 Jul 2020 17:26:48 +0200 X-SMTPAUTHUS: auth mail.us.es From: Pablo Neira Ayuso To: Petr Machata Cc: Ido Schimmel , netdev@vger.kernel.org, davem@davemloft.net, kuba@kernel.org, jiri@mellanox.com, mlxsw@mellanox.com, michael.chan@broadcom.com, saeedm@mellanox.com, leon@kernel.org, kadlec@netfilter.org, fw@strlen.de, jhs@mojatatu.com, xiyou.wangcong@gmail.com, simon.horman@netronome.com, Ido Schimmel Subject: Re: [PATCH net-next 01/13] net: sched: Pass qdisc reference in struct flow_block_offload Message-ID: <20200710152648.GA14902@salvia> References: <20200710135706.601409-1-idosch@idosch.org> <20200710135706.601409-2-idosch@idosch.org> <20200710141500.GA12659@salvia> <87sgdzflk4.fsf@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87sgdzflk4.fsf@mellanox.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Virus-Scanned: ClamAV using ClamSMTP Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Jul 10, 2020 at 05:15:39PM +0200, Petr Machata wrote: > > Pablo Neira Ayuso writes: > > > On Fri, Jul 10, 2020 at 04:56:54PM +0300, Ido Schimmel wrote: > >> From: Petr Machata > >> > >> Previously, shared blocks were only relevant for the pseudo-qdiscs ingress > >> and clsact. Recently, a qevent facility was introduced, which allows to > >> bind blocks to well-defined slots of a qdisc instance. RED in particular > >> got two qevents: early_drop and mark. Drivers that wish to offload these > >> blocks will be sent the usual notification, and need to know which qdisc it > >> is related to. > >> > >> To that end, extend flow_block_offload with a "sch" pointer, and initialize > >> as appropriate. This prompts changes in the indirect block facility, which > >> now tracks the scheduler instead of the netdevice. Update signatures of > >> several functions similarly. Deduce the device from the scheduler when > >> necessary. > >> > >> Signed-off-by: Petr Machata > >> Reviewed-by: Jiri Pirko > >> Signed-off-by: Ido Schimmel > >> --- > >> drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 11 ++++++---- > >> .../ethernet/mellanox/mlx5/core/en/rep/tc.c | 11 +++++----- > >> .../net/ethernet/netronome/nfp/flower/main.h | 2 +- > >> .../ethernet/netronome/nfp/flower/offload.c | 11 ++++++---- > >> include/net/flow_offload.h | 9 ++++---- > >> net/core/flow_offload.c | 12 +++++------ > >> net/netfilter/nf_flow_table_offload.c | 17 +++++++-------- > >> net/netfilter/nf_tables_offload.c | 20 ++++++++++-------- > >> net/sched/cls_api.c | 21 +++++++++++-------- > >> 9 files changed, 63 insertions(+), 51 deletions(-) > >> > > [...] > >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c > >> index eefeb1cdc2ee..4fc42c1955ff 100644 > >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c > >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c > >> @@ -404,7 +404,7 @@ static void mlx5e_rep_indr_block_unbind(void *cb_priv) > >> static LIST_HEAD(mlx5e_block_cb_list); > >> > >> static int > >> -mlx5e_rep_indr_setup_block(struct net_device *netdev, > >> +mlx5e_rep_indr_setup_block(struct Qdisc *sch, > >> struct mlx5e_rep_priv *rpriv, > >> struct flow_block_offload *f, > >> flow_setup_cb_t *setup_cb, > >> @@ -412,6 +412,7 @@ mlx5e_rep_indr_setup_block(struct net_device *netdev, > >> void (*cleanup)(struct flow_block_cb *block_cb)) > >> { > >> struct mlx5e_priv *priv = netdev_priv(rpriv->netdev); > >> + struct net_device *netdev = sch->dev_queue->dev; > > > > This break indirect block support for netfilter since the driver > > is assuming a Qdisc object. > > Sorry, I don't follow. You mean mlx5 driver? What does it mean to > "assume a qdisc object"? > > Is it incorrect to rely on the fact that the netdevice can be deduced > from a qdisc, or that there is always a qdisc associated with a block > binding point? The drivers assume that the xyz_indr_setup_block() always gets a sch object, which is not always true. Are you really sure this will work for the TC CT offload? --- a/net/netfilter/nf_flow_table_offload.c +++ b/net/netfilter/nf_flow_table_offload.c @@ -928,26 +928,27 @@ static int nf_flow_table_block_setup(struct nf_flowtable *flowtable, } static void nf_flow_table_block_offload_init(struct flow_block_offload *bo, - struct net *net, + struct net_device *dev, enum flow_block_command cmd, struct nf_flowtable *flowtable, struct netlink_ext_ack *extack) { memset(bo, 0, sizeof(*bo)); - bo->net = net; + bo->net = dev_net(dev); bo->block = &flowtable->flow_block; bo->command = cmd; bo->binder_type = FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS; bo->extack = extack; + bo->sch = dev_ingress_queue(dev)->qdisc_sleeping; INIT_LIST_HEAD(&bo->cb_list); } static void nf_flow_table_indr_cleanup(struct flow_block_cb *block_cb) { struct nf_flowtable *flowtable = block_cb->indr.data; - struct net_device *dev = block_cb->indr.dev; + struct Qdisc *sch = block_cb->indr.sch; - nf_flow_table_gc_cleanup(flowtable, dev); + nf_flow_table_gc_cleanup(flowtable, sch->dev_queue->dev); down_write(&flowtable->flow_block_lock); list_del(&block_cb->list); list_del(&block_cb->driver_list); Moreover, the flow_offload infrastructure should also remain independent from the front-end, either tc/netfilter/ethtool, this is pulling in tc specific stuff into it, eg. diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index de395498440d..fda29140bdc5 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -444,6 +444,7 @@ struct flow_block_offload { struct list_head cb_list; struct list_head *driver_block_list; struct netlink_ext_ack *extack; + struct Qdisc *sch; }; enum tc_setup_type; @@ -454,7 +455,7 @@ struct flow_block_cb; struct flow_block_indr { struct list_head list; - struct net_device *dev; + struct Qdisc *sch; enum flow_block_binder_type binder_type; void *data; void *cb_priv; @@ -479,7 +480,7 @@ struct flow_block_cb *flow_indr_block_cb_alloc(flow_setup_cb_t *cb, void *cb_ident, void *cb_priv, void (*release)(void *cb_priv), struct flow_block_offload *bo, - struct net_device *dev, void *data, + struct Qdisc *sch, void *data, void *indr_cb_priv, void (*cleanup)(struct flow_block_cb *block_cb)); void flow_block_cb_free(struct flow_block_cb *block_cb);