Re: [PATCH net-next,v3 05/11] net: flow_offload: add list handling functions

[Jiri Pirko <jiri@resnulli.us>]

Mon, Jul 08, 2019 at 06:06:07PM CEST, pablo@netfilter.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@netfilter.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
>