RE: [PATCH v4 08/10] flow_offload: add reoffload process to update hw_count

On November 20, 2021 4:09 AM, Vlad Buslov wrote:

On Thu 18 Nov 2021 at 15:08, Simon Horman [off-list ref]
wrote:

quoted

From: Baowen Zheng <redacted>

Add reoffload process to update hw_count when driver is inserted or
removed.

When reoffloading actions, we still offload the actions that are added
independent of filters.

Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
 include/net/act_api.h   |  24 +++++
 net/core/flow_offload.c |   4 +
 net/sched/act_api.c     | 213

++++++++++++++++++++++++++++++++++++----

quoted

 3 files changed, 222 insertions(+), 19 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h index

7900598d2dd3..e5e6e58df618 100644

--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -7,6 +7,7 @@
 */

 #include <linux/refcount.h>
+#include <net/flow_offload.h>
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
 #include <net/net_namespace.h>

@@ -243,11 +244,26 @@ static inline void flow_action_hw_count_set(struct

tc_action *act,

quoted

 	act->in_hw_count = hw_count;
 }

+static inline void flow_action_hw_count_inc(struct tc_action *act,
+					    u32 hw_count)
+{
+	act->in_hw_count += hw_count;
+}
+
+static inline void flow_action_hw_count_dec(struct tc_action *act,
+					    u32 hw_count)
+{
+	act->in_hw_count = act->in_hw_count > hw_count ?
+			   act->in_hw_count - hw_count : 0; }
+
 void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
 			     u64 drops, bool hw);
 int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);

 int tcf_action_update_hw_stats(struct tc_action *action);
+int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
+			    void *cb_priv, bool add);
 int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
 			     struct tcf_chain **handle,
 			     struct netlink_ext_ack *newchain); @@ -259,6

+275,14 @@

quoted

DECLARE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
 #endif

 int tcf_dev_queue_xmit(struct sk_buff *skb, int (*xmit)(struct
sk_buff *skb));
+
+#else /* !CONFIG_NET_CLS_ACT */
+
+static inline int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
+					  void *cb_priv, bool add) {
+	return 0;
+}
+
 #endif /* CONFIG_NET_CLS_ACT */

 static inline void tcf_action_stats_update(struct tc_action *a, u64
bytes, diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 6676431733ef..92000164ac37 100644

--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c

@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */  #include <linux/kernel.h>

#include <linux/slab.h>
+#include <net/act_api.h>
 #include <net/flow_offload.h>
 #include <linux/rtnetlink.h>
 #include <linux/mutex.h>

@@ -418,6 +419,8 @@ int

flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)

quoted

 	existing_qdiscs_register(cb, cb_priv);
 	mutex_unlock(&flow_indr_block_lock);

+	tcf_action_reoffload_cb(cb, cb_priv, true);
+
 	return 0;
 }
 EXPORT_SYMBOL(flow_indr_dev_register);

@@ -470,6 +473,7 @@ void

flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,

quoted

 	__flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
 	mutex_unlock(&flow_indr_block_lock);

+	tcf_action_reoffload_cb(cb, cb_priv, false);
 	flow_block_indr_notify(&cleanup_list);
 	kfree(indr_dev);
 }

diff --git a/net/sched/act_api.c b/net/sched/act_api.c index

f5834d47a392..ada51b2df851 100644

--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -225,15 +225,11 @@ static int flow_action_init(struct

flow_offload_action *fl_action,

quoted

 	return 0;
 }

-static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
-				  u32 *hw_count,
-				  struct netlink_ext_ack *extack)
+static int tcf_action_offload_cmd_ex(struct flow_offload_action *fl_act,
+				     u32 *hw_count)
 {
 	int err;

-	if (IS_ERR(fl_act))
-		return PTR_ERR(fl_act);
-
 	err = flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT,
 					  fl_act, NULL, NULL);
 	if (err < 0)

@@ -245,9 +241,41 @@ static int tcf_action_offload_cmd(struct

flow_offload_action *fl_act,

quoted

 	return 0;
 }

+static int tcf_action_offload_cmd_cb_ex(struct flow_offload_action

*fl_act,

quoted

+					u32 *hw_count,
+					flow_indr_block_bind_cb_t *cb,
+					void *cb_priv)
+{
+	int err;
+
+	err = cb(NULL, NULL, cb_priv, TC_SETUP_ACT, NULL, fl_act, NULL);
+	if (err < 0)
+		return err;
+
+	if (hw_count)
+		*hw_count = 1;
+
+	return 0;
+}
+
+static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
+				  u32 *hw_count,
+				  flow_indr_block_bind_cb_t *cb,
+				  void *cb_priv)
+{
+	if (IS_ERR(fl_act))
+		return PTR_ERR(fl_act);
+
+	return cb ? tcf_action_offload_cmd_cb_ex(fl_act, hw_count,
+						 cb, cb_priv) :
+		    tcf_action_offload_cmd_ex(fl_act, hw_count); }
+
 /* offload the tc command after inserted */ -static int
tcf_action_offload_add(struct tc_action *action,
-				  struct netlink_ext_ack *extack)
+static int tcf_action_offload_add_ex(struct tc_action *action,
+				     struct netlink_ext_ack *extack,
+				     flow_indr_block_bind_cb_t *cb,
+				     void *cb_priv)
 {
 	bool skip_sw = tc_act_skip_sw(action->tcfa_flags);
 	struct tc_action *actions[TCA_ACT_MAX_PRIO] = { @@ -275,9 +303,10

@@

quoted

static int tcf_action_offload_add(struct tc_action *action,
 		goto fl_err;
 	}

-	err = tcf_action_offload_cmd(fl_action, &in_hw_count, extack);
+	err = tcf_action_offload_cmd(fl_action, &in_hw_count, cb, cb_priv);
 	if (!err)
-		flow_action_hw_count_set(action, in_hw_count);
+		cb ? flow_action_hw_count_inc(action, in_hw_count) :
+		     flow_action_hw_count_set(action, in_hw_count);

 	if (skip_sw && !tc_act_in_hw(action))
 		err = -EINVAL;

@@ -290,6 +319,12 @@ static int tcf_action_offload_add(struct tc_action

*action,

quoted

 	return err;
 }

+static int tcf_action_offload_add(struct tc_action *action,
+				  struct netlink_ext_ack *extack) {
+	return tcf_action_offload_add_ex(action, extack, NULL, NULL); }
+
 int tcf_action_update_hw_stats(struct tc_action *action)  {
 	struct flow_offload_action fl_act = {}; @@ -302,7 +337,7 @@ int
tcf_action_update_hw_stats(struct tc_action *action)
 	if (err)
 		return err;

-	err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
+	err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
 	if (!err) {
 		preempt_disable();
 		tcf_action_stats_update(action, fl_act.stats.bytes, @@ -321,7
+356,9 @@ int tcf_action_update_hw_stats(struct tc_action *action)  }
EXPORT_SYMBOL(tcf_action_update_hw_stats);

-static int tcf_action_offload_del(struct tc_action *action)
+static int tcf_action_offload_del_ex(struct tc_action *action,
+				     flow_indr_block_bind_cb_t *cb,
+				     void *cb_priv)
 {
 	struct flow_offload_action fl_act;
 	u32 in_hw_count = 0;

@@ -337,16 +374,25 @@ static int tcf_action_offload_del(struct tc_action

*action)

quoted

 	if (err)
 		return err;

-	err = tcf_action_offload_cmd(&fl_act, &in_hw_count, NULL);
-	if (err)
+	err = tcf_action_offload_cmd(&fl_act, &in_hw_count, cb, cb_priv);
+	if (err < 0)
 		return err;

-	if (action->in_hw_count != in_hw_count)
+	if (!cb && action->in_hw_count != in_hw_count)
 		return -EINVAL;

+	/* do not need to update hw state when deleting action */
+	if (cb && in_hw_count)
+		flow_action_hw_count_dec(action, in_hw_count);
+
 	return 0;
 }

+static int tcf_action_offload_del(struct tc_action *action) {
+	return tcf_action_offload_del_ex(action, NULL, NULL); }
+
 static void tcf_action_cleanup(struct tc_action *p)  {
 	tcf_action_offload_del(p);

@@ -841,6 +887,59 @@ EXPORT_SYMBOL(tcf_idrinfo_destroy);

 static LIST_HEAD(act_base);
 static DEFINE_RWLOCK(act_mod_lock);
+/* since act ops id is stored in pernet subsystem list,
+ * then there is no way to walk through only all the action
+ * subsystem, so we keep tc action pernet ops id for
+ * reoffload to walk through.
+ */
+static LIST_HEAD(act_pernet_id_list); static
+DEFINE_MUTEX(act_id_mutex); struct tc_act_pernet_id {
+	struct list_head list;
+	unsigned int id;
+};
+
+static int tcf_pernet_add_id_list(unsigned int id) {
+	struct tc_act_pernet_id *id_ptr;
+	int ret = 0;
+
+	mutex_lock(&act_id_mutex);
+	list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
+		if (id_ptr->id == id) {
+			ret = -EEXIST;
+			goto err_out;
+		}
+	}
+
+	id_ptr = kzalloc(sizeof(*id_ptr), GFP_KERNEL);
+	if (!id_ptr) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+	id_ptr->id = id;
+
+	list_add_tail(&id_ptr->list, &act_pernet_id_list);
+
+err_out:
+	mutex_unlock(&act_id_mutex);
+	return ret;
+}
+
+static void tcf_pernet_del_id_list(unsigned int id) {
+	struct tc_act_pernet_id *id_ptr;
+
+	mutex_lock(&act_id_mutex);
+	list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
+		if (id_ptr->id == id) {
+			list_del(&id_ptr->list);
+			kfree(id_ptr);
+			break;
+		}
+	}
+	mutex_unlock(&act_id_mutex);
+}

 int tcf_register_action(struct tc_action_ops *act,
 			struct pernet_operations *ops)

@@ -859,18 +958,30 @@ int tcf_register_action(struct tc_action_ops *act,
 	if (ret)
 		return ret;

+	if (ops->id) {
+		ret = tcf_pernet_add_id_list(*ops->id);
+		if (ret)
+			goto id_err;
+	}
+
 	write_lock(&act_mod_lock);
 	list_for_each_entry(a, &act_base, head) {
 		if (act->id == a->id || (strcmp(act->kind, a->kind) == 0)) {
-			write_unlock(&act_mod_lock);
-			unregister_pernet_subsys(ops);
-			return -EEXIST;
+			ret = -EEXIST;
+			goto err_out;
 		}
 	}
 	list_add_tail(&act->head, &act_base);
 	write_unlock(&act_mod_lock);

 	return 0;
+
+err_out:
+	write_unlock(&act_mod_lock);
+	tcf_pernet_del_id_list(*ops->id);
+id_err:

Rename to 'err_id' to harmonize label naming.

Ok, will make the change.

quoted

+	unregister_pernet_subsys(ops);
+	return ret;
 }
 EXPORT_SYMBOL(tcf_register_action);

@@ -889,12 +1000,76 @@ int tcf_unregister_action(struct tc_action_ops

*act,

quoted

 		}
 	}
 	write_unlock(&act_mod_lock);
-	if (!err)
+	if (!err) {
 		unregister_pernet_subsys(ops);
+		if (ops->id)
+			tcf_pernet_del_id_list(*ops->id);
+	}
 	return err;
 }
 EXPORT_SYMBOL(tcf_unregister_action);

+int tcf_action_reoffload_cb(flow_indr_block_bind_cb_t *cb,
+			    void *cb_priv, bool add)
+{
+	struct tc_act_pernet_id *id_ptr;
+	struct tcf_idrinfo *idrinfo;
+	struct tc_action_net *tn;
+	struct tc_action *p;
+	unsigned int act_id;
+	unsigned long tmp;
+	unsigned long id;
+	struct idr *idr;
+	struct net *net;
+	int ret;
+
+	if (!cb)
+		return -EINVAL;
+
+	down_read(&net_rwsem);
+	mutex_lock(&act_id_mutex);
+
+	for_each_net(net) {
+		list_for_each_entry(id_ptr, &act_pernet_id_list, list) {
+			act_id = id_ptr->id;
+			tn = net_generic(net, act_id);
+			if (!tn)
+				continue;
+			idrinfo = tn->idrinfo;
+			if (!idrinfo)
+				continue;
+
+			mutex_lock(&idrinfo->lock);
+			idr = &idrinfo->action_idr;
+			idr_for_each_entry_ul(idr, p, tmp, id) {
+				if (IS_ERR(p) || tc_act_bind(p->tcfa_flags))
+					continue;
+				if (add) {
+					tcf_action_offload_add_ex(p, NULL,

cb,

quoted

+								  cb_priv);
+					continue;
+				}
+
+				/* cb unregister to update hw count */
+				ret = tcf_action_offload_del_ex(p, cb,

cb_priv);

quoted

+				if (ret < 0)
+					continue;
+				if (tc_act_skip_sw(p->tcfa_flags) &&
+				    !tc_act_in_hw(p)) {
+					ret = tcf_idr_release_unsafe(p);

Deleting skip_sw action that is no longer offloaded to any hardware device is
reasonable, but note that in this case no netlink notification is ever generated.
Don't know if it is a problem, just highlighting the fact since the whole
behavior of implicitly deleting such actions is completely omitted from the
change log.

Thanks for bring this to us. We will think about to add the notification implement and also we will
change commit message to mention about this.

quoted

+					if (ret == ACT_P_DELETED)
+						module_put(p->ops->owner);
+				}
+			}
+			mutex_unlock(&idrinfo->lock);
+		}
+	}
+	mutex_unlock(&act_id_mutex);
+	up_read(&net_rwsem);
+
+	return 0;
+}
+
 /* lookup by name */
 static struct tc_action_ops *tc_lookup_action_n(char *kind)  {