Message-ID: <20190708172613.GA2282@nanopsycho.orion>
Date:   Mon, 8 Jul 2019 19:26:13 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        thomas.lendacky@....com, f.fainelli@...il.com,
        ariel.elior@...ium.com, michael.chan@...adcom.com,
        madalin.bucur@....com, yisen.zhuang@...wei.com,
        salil.mehta@...wei.com, jeffrey.t.kirsher@...el.com,
        tariqt@...lanox.com, saeedm@...lanox.com, jiri@...lanox.com,
        idosch@...lanox.com, jakub.kicinski@...ronome.com,
        peppe.cavallaro@...com, grygorii.strashko@...com, andrew@...n.ch,
        vivien.didelot@...il.com, alexandre.torgue@...com,
        joabreu@...opsys.com, linux-net-drivers@...arflare.com,
        ogerlitz@...lanox.com, Manish.Chopra@...ium.com,
        marcelo.leitner@...il.com, mkubecek@...e.cz,
        venkatkumar.duvvuru@...adcom.com, maxime.chevallier@...tlin.com,
        cphealy@...il.com, netfilter-devel@...r.kernel.org
Subject: Re: [PATCH net-next,v3 05/11] net: flow_offload: add list handling
 functions

Mon, Jul 08, 2019 at 06:06:07PM CEST, pablo@...filter.org wrote:
>This patch adds the list handling functions for the flow block API:
>
>* flow_block_cb_lookup() allows drivers to look up for existing flow blocks.
>* flow_block_cb_add() adds a flow block to the list to be registered by the
>  core.

Per driver? You say "per driver" in the "remove" part.


>* flow_block_cb_remove() to remove a flow block from the list of existing
>  flow blocks per driver and to request the core to unregister this.
>
>The flow block API also annotates the netns this flow block belongs to.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
>---
>v3: extracted from former patch "net: flow_offload: add flow_block_cb API".
>
> include/net/flow_offload.h | 20 ++++++++++++++++++++
> net/core/flow_offload.c    | 18 ++++++++++++++++++
> net/sched/cls_api.c        |  3 +++
> 3 files changed, 41 insertions(+)
>
>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>index bcc4e2fef6ba..06acde2960fa 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -251,12 +251,16 @@ struct flow_block_offload {
> 	enum flow_block_command command;
> 	enum flow_block_binder_type binder_type;
> 	struct tcf_block *block;
>+	struct net *net;
>+	struct list_head cb_list;
> 	struct list_head *driver_block_list;
> 	struct netlink_ext_ack *extack;
> };
> 
> struct flow_block_cb {
>+	struct list_head	driver_list;
> 	struct list_head	list;
>+	struct net		*net;
> 	tc_setup_cb_t		*cb;
> 	void			*cb_ident;
> 	void			*cb_priv;
>@@ -269,6 +273,22 @@ struct flow_block_cb *flow_block_cb_alloc(struct net *net, tc_setup_cb_t *cb,
> 					  void (*release)(void *cb_priv));
> void flow_block_cb_free(struct flow_block_cb *block_cb);
> 
>+struct flow_block_cb *flow_block_cb_lookup(struct net *net,
>+					   struct list_head *driver_flow_block_list,
>+					   tc_setup_cb_t *cb, void *cb_ident);
>+
>+static inline void flow_block_cb_add(struct flow_block_cb *block_cb,
>+				     struct flow_block_offload *offload)
>+{
>+	list_add_tail(&block_cb->driver_list, &offload->cb_list);
>+}
>+
>+static inline void flow_block_cb_remove(struct flow_block_cb *block_cb,
>+					struct flow_block_offload *offload)
>+{
>+	list_move(&block_cb->driver_list, &offload->cb_list);
>+}
>+
> int flow_block_cb_setup_simple(struct flow_block_offload *f,
> 			       struct list_head *driver_list, tc_setup_cb_t *cb,
> 			       void *cb_ident, void *cb_priv, bool ingress_only);
>diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
>index d08148cb6953..85fd5f4a1e0f 100644
>--- a/net/core/flow_offload.c
>+++ b/net/core/flow_offload.c
>@@ -176,6 +176,7 @@ struct flow_block_cb *flow_block_cb_alloc(struct net *net, tc_setup_cb_t *cb,
> 	if (!block_cb)
> 		return ERR_PTR(-ENOMEM);
> 
>+	block_cb->net = net;
> 	block_cb->cb = cb;
> 	block_cb->cb_ident = cb_ident;
> 	block_cb->cb_priv = cb_priv;
>@@ -194,6 +195,23 @@ void flow_block_cb_free(struct flow_block_cb *block_cb)
> }
> EXPORT_SYMBOL(flow_block_cb_free);
> 
>+struct flow_block_cb *flow_block_cb_lookup(struct net *net,
>+					   struct list_head *driver_block_list,

In the header, you call this "driver_flow_block_list".

Where is this list coming from? In general, I don't think it is good to
have struct list_head as an arg of exported symbol. Should be contained
in some struct. Looks like this might be the "struct
flow_block_offload"?

Does this have anything to do with "struct list_head
*driver_block_list"? This is very confusing...



>+					   tc_setup_cb_t *cb, void *cb_ident)
>+{
>+	struct flow_block_cb *block_cb;
>+
>+	list_for_each_entry(block_cb, driver_block_list, driver_list) {
>+		if (block_cb->net == net &&
>+		    block_cb->cb == cb &&
>+		    block_cb->cb_ident == cb_ident)
>+			return block_cb;
>+	}
>+
>+	return NULL;
>+}
>+EXPORT_SYMBOL(flow_block_cb_lookup);
>+
> int flow_block_cb_setup_simple(struct flow_block_offload *f,
> 			       struct list_head *driver_block_list,
> 			       tc_setup_cb_t *cb, void *cb_ident, void *cb_priv,
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index fa0c451aca59..72761b43ae41 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -679,6 +679,7 @@ static void tc_indr_block_ing_cmd(struct tc_indr_block_dev *indr_dev,
> 	struct tc_block_offload bo = {
> 		.command	= command,
> 		.binder_type	= FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS,
>+		.net		= dev_net(indr_dev->dev),
> 		.block		= indr_dev->block,
> 	};
> 
>@@ -767,6 +768,7 @@ static void tc_indr_block_call(struct tcf_block *block, struct net_device *dev,
> 	struct tc_block_offload bo = {
> 		.command	= command,
> 		.binder_type	= ei->binder_type,
>+		.net		= dev_net(dev),
> 		.block		= block,
> 		.extack		= extack,
> 	};
>@@ -795,6 +797,7 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
> {
> 	struct tc_block_offload bo = {};
> 
>+	bo.net = dev_net(dev);
> 	bo.command = command;
> 	bo.binder_type = ei->binder_type;
> 	bo.block = block;
>-- 
>2.11.0
>

Powered by blists - more mailing lists