[Patch net 0/5] net_sched: tc action fixes and updates

This patchset fixes several regressions caused by the previous
code refactor. Thanks to Jamal for catching them!

Note, patch 3/5 and 4/5 are not strictly necessary, I just
want to carry them together.

Cong Wang (5):
  net_sched: remove the leftover cleanup_a()
  net_sched: remove an unnecessary list_del()
  net_sched: fix a typo in tc_for_each_action()
  net_sched: move tc offload macros to pkt_cls.h
  net_sched: convert tcf_exts from list to flex_array

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   |  4 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 ++++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c  |  4 +-
 include/net/act_api.h                           | 16 -------
 include/net/pkt_cls.h                           | 46 +++++++++++++++++---
 net/sched/act_api.c                             | 23 ++--------
 net/sched/cls_api.c                             | 57 ++++++++++++++++++-------
 7 files changed, 101 insertions(+), 61 deletions(-)

-- 
2.1.0

After refactoring tc_action into tcf_common, we no
longer need to cleanup temporary "actions" in list,
they are permanently stored in the hashtable.

Fixes: a85a970af265 ("net_sched: move tc_action into tcf_common")
Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---
 net/sched/act_api.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index e4a5f26..cce6986 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -754,16 +754,6 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
 	return ERR_PTR(err);
 }
 
-static void cleanup_a(struct list_head *actions)
-{
-	struct tc_action *a, *tmp;
-
-	list_for_each_entry_safe(a, tmp, actions, list) {
-		list_del(&a->list);
-		kfree(a);
-	}
-}
-
 static int tca_action_flush(struct net *net, struct nlattr *nla,
 			    struct nlmsghdr *n, u32 portid)
 {

@@ -905,7 +895,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 		return ret;
 	}
 err:
-	cleanup_a(&actions);
+	tcf_action_destroy(&actions, 0);
 	return ret;
 }
 

@@ -942,15 +932,9 @@ tcf_action_add(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 
 	ret = tcf_action_init(net, nla, NULL, NULL, ovr, 0, &actions);
 	if (ret)
-		goto done;
+		return ret;
 
-	/* dump then free all the actions after update; inserted policy
-	 * stays intact
-	 */
-	ret = tcf_add_notify(net, n, &actions, portid);
-	cleanup_a(&actions);
-done:
-	return ret;
+	return tcf_add_notify(net, n, &actions, portid);
 }
 
 static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n)

-- 
2.1.0

This list_del() for tc action is not needed actually,
because we only use this list to chain bulk operations,
therefore should not be carried for latter operations.

Fixes: ec0595cc4495 ("net_sched: get rid of struct tcf_common")
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---
 net/sched/act_api.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index cce6986..b4c7be3 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -64,7 +64,6 @@ int __tcf_hash_release(struct tc_action *p, bool bind, bool strict)
 		if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) {
 			if (p->ops->cleanup)
 				p->ops->cleanup(p, bind);
-			list_del(&p->list);
 			tcf_hash_destroy(p->hinfo, p);
 			ret = ACT_P_DELETED;
 		}

-- 
2.1.0

It is harmless because all users pass 'a' to this macro.

Fixes: 00175aec941e ("net/sched: Macro instead of CONFIG_NET_CLS_ACT ifdef")
Cc: Amir Vadai <redacted>
Signed-off-by: Cong Wang <redacted>
---
 include/net/act_api.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 41e6a24..f53ee9d 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -193,7 +193,7 @@ int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
 	(list_empty(&(_exts)->actions))
 
 #define tc_for_each_action(_a, _exts) \
-	list_for_each_entry(a, &(_exts)->actions, list)
+	list_for_each_entry(_a, &(_exts)->actions, list)
 
 #define tc_single_action(_exts) \
 	(list_is_singular(&(_exts)->actions))

-- 
2.1.0

struct tcf_exts belongs to filters, should not be visible
to plain tc actions.

Cc: Ido Schimmel <redacted>
Signed-off-by: Cong Wang <redacted>
---
 include/net/act_api.h | 16 ----------------
 include/net/pkt_cls.h | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index f53ee9d..3fc88ad 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -189,15 +189,6 @@ int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
 int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
 int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
 
-#define tc_no_actions(_exts) \
-	(list_empty(&(_exts)->actions))
-
-#define tc_for_each_action(_a, _exts) \
-	list_for_each_entry(_a, &(_exts)->actions, list)
-
-#define tc_single_action(_exts) \
-	(list_is_singular(&(_exts)->actions))
-
 static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
 					   u64 packets, u64 lastuse)
 {

@@ -207,12 +198,5 @@ static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
 	a->ops->stats_update(a, bytes, packets, lastuse);
 }
 
-#else /* CONFIG_NET_CLS_ACT */
-
-#define tc_no_actions(_exts) true
-#define tc_for_each_action(_a, _exts) while ((void)(_a), 0)
-#define tc_single_action(_exts) false
-#define tcf_action_stats_update(a, bytes, packets, lastuse)
-
 #endif /* CONFIG_NET_CLS_ACT */
 #endif

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 6f8d653..f15aa1e 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h

@@ -130,6 +130,26 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
 	return 0;
 }
 
+#ifdef CONFIG_NET_CLS_ACT
+
+#define tc_no_actions(_exts) \
+	(list_empty(&(_exts)->actions))
+
+#define tc_for_each_action(_a, _exts) \
+	list_for_each_entry(_a, &(_exts)->actions, list)
+
+#define tc_single_action(_exts) \
+	(list_is_singular(&(_exts)->actions))
+
+#else /* CONFIG_NET_CLS_ACT */
+
+#define tc_no_actions(_exts) true
+#define tc_for_each_action(_a, _exts) while ((void)(_a), 0)
+#define tc_single_action(_exts) false
+#define tcf_action_stats_update(a, bytes, packets, lastuse)
+
+#endif /* CONFIG_NET_CLS_ACT */
+
 int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
 		      struct nlattr **tb, struct nlattr *rate_tlv,
 		      struct tcf_exts *exts, bool ovr);

-- 
2.1.0

As pointed out by Jamal, an action could be shared by
multiple filters, so we can't use list to chain them
any more after we get rid of the original tc_action.
Instead, we could just save pointers to these actions
in tcf_exts, since they are refcount'ed, so convert
the list to a flex array.

The ugly part is the action API still accepts list
as a parameter, I just introduce a helper function to
convert the flex array of pointers to a list.

Fixes: a85a970af265 ("net_sched: move tc_action into tcf_common")
Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   |  4 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 ++++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c  |  4 +-
 include/net/pkt_cls.h                           | 44 +++++++++++++------
 net/sched/cls_api.c                             | 57 ++++++++++++++++++-------
 5 files changed, 87 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 5418c69a..6ac1254 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -8390,12 +8390,14 @@ static int parse_tc_actions(struct ixgbe_adapter *adapter,
 			    struct tcf_exts *exts, u64 *action, u8 *queue)
 {
 	const struct tc_action *a;
+	LIST_HEAD(actions);
 	int err;
 
 	if (tc_no_actions(exts))
 		return -EINVAL;
 
-	tc_for_each_action(a, exts) {
+	tcf_exts_to_list(exts, &actions);
+	list_for_each_entry(a, &actions, list) {
 
 		/* Drop action */
 		if (is_tcf_gact_shot(a)) {

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 0f19b01..dc8b1cb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

@@ -318,6 +318,7 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
 				u32 *action, u32 *flow_tag)
 {
 	const struct tc_action *a;
+	LIST_HEAD(actions);
 
 	if (tc_no_actions(exts))
 		return -EINVAL;

@@ -325,7 +326,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
 	*flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
 	*action = 0;
 
-	tc_for_each_action(a, exts) {
+	tcf_exts_to_list(exts, &actions);
+	list_for_each_entry(a, &actions, list) {
 		/* Only support a single action per rule */
 		if (*action)
 			return -EINVAL;

@@ -362,13 +364,15 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
 				u32 *action, u32 *dest_vport)
 {
 	const struct tc_action *a;
+	LIST_HEAD(actions);
 
 	if (tc_no_actions(exts))
 		return -EINVAL;
 
 	*action = 0;
 
-	tc_for_each_action(a, exts) {
+	tcf_exts_to_list(exts, &actions);
+	list_for_each_entry(a, &actions, list) {
 		/* Only support a single action per rule */
 		if (*action)
 			return -EINVAL;

@@ -503,6 +507,7 @@ int mlx5e_stats_flower(struct mlx5e_priv *priv,
 	struct mlx5e_tc_flow *flow;
 	struct tc_action *a;
 	struct mlx5_fc *counter;
+	LIST_HEAD(actions);
 	u64 bytes;
 	u64 packets;
 	u64 lastuse;

@@ -518,7 +523,8 @@ int mlx5e_stats_flower(struct mlx5e_priv *priv,
 
 	mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
 
-	tc_for_each_action(a, f->exts)
+	tcf_exts_to_list(f->exts, &actions);
+	list_for_each_entry(a, &actions, list)
 		tcf_action_stats_update(a, bytes, packets, lastuse);
 
 	return 0;

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index c3e6150..df85acd 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c

@@ -1149,6 +1149,7 @@ static int mlxsw_sp_port_add_cls_matchall(struct mlxsw_sp_port *mlxsw_sp_port,
 					  bool ingress)
 {
 	const struct tc_action *a;
+	LIST_HEAD(actions);
 	int err;
 
 	if (!tc_single_action(cls->exts)) {

@@ -1156,7 +1157,8 @@ static int mlxsw_sp_port_add_cls_matchall(struct mlxsw_sp_port *mlxsw_sp_port,
 		return -ENOTSUPP;
 	}
 
-	tc_for_each_action(a, cls->exts) {
+	tcf_exts_to_list(cls->exts, &actions);
+	list_for_each_entry(a, &actions, list) {
 		if (!is_tcf_mirred_mirror(a) || protocol != htons(ETH_P_ALL))
 			return -ENOTSUPP;
 

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index f15aa1e..2b6f0ec 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h

@@ -1,6 +1,7 @@
 #ifndef __NET_PKT_CLS_H
 #define __NET_PKT_CLS_H
 
+#include <linux/flex_array.h>
 #include <linux/pkt_cls.h>
 #include <net/sch_generic.h>
 #include <net/act_api.h>

@@ -59,7 +60,8 @@ tcf_unbind_filter(struct tcf_proto *tp, struct tcf_result *r)
 struct tcf_exts {
 #ifdef CONFIG_NET_CLS_ACT
 	__u32	type; /* for backward compat(TCA_OLD_COMPAT) */
-	struct list_head actions;
+	int nr_actions;
+	struct flex_array *actions;
 #endif
 	/* Map to export classifier specific extension TLV types to the
 	 * generic extensions API. Unsupported extensions must be set to 0.

@@ -72,7 +74,10 @@ static inline void tcf_exts_init(struct tcf_exts *exts, int action, int police)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	exts->type = 0;
-	INIT_LIST_HEAD(&exts->actions);
+	exts->nr_actions = 0;
+	exts->actions = flex_array_alloc(sizeof(struct tc_action *),
+					 TCA_ACT_MAX_PRIO, GFP_KERNEL);
+	WARN_ON(!exts->actions); /* TODO: propagate the error to callers */
 #endif
 	exts->action = action;
 	exts->police = police;

@@ -89,7 +94,7 @@ static inline int
 tcf_exts_is_predicative(struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	return !list_empty(&exts->actions);
+	return exts->nr_actions;
 #else
 	return 0;
 #endif

@@ -108,6 +113,21 @@ tcf_exts_is_available(struct tcf_exts *exts)
 	return tcf_exts_is_predicative(exts);
 }
 
+static inline void
+tcf_exts_to_list(const struct tcf_exts *exts, struct list_head *actions)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	int i;
+
+	for (i = 0; i < exts->nr_actions; i++) {
+		struct tc_action *a;
+
+		a  = *(struct tc_action **)flex_array_get(exts->actions, i);
+		list_add(&a->list, actions);
+	}
+#endif
+}
+
 /**
  * tcf_exts_exec - execute tc filter extensions
  * @skb: socket buffer

@@ -124,27 +144,23 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
 	       struct tcf_result *res)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (!list_empty(&exts->actions))
-		return tcf_action_exec(skb, &exts->actions, res);
+	LIST_HEAD(actions);
+
+	tcf_exts_to_list(exts, &actions);
+	if (exts->nr_actions)
+		return tcf_action_exec(skb, &actions, res);
 #endif
 	return 0;
 }
 
 #ifdef CONFIG_NET_CLS_ACT
 
-#define tc_no_actions(_exts) \
-	(list_empty(&(_exts)->actions))
-
-#define tc_for_each_action(_a, _exts) \
-	list_for_each_entry(_a, &(_exts)->actions, list)
-
-#define tc_single_action(_exts) \
-	(list_is_singular(&(_exts)->actions))
+#define tc_no_actions(_exts)  (&(_exts)->nr_actions == 0)
+#define tc_single_action(_exts) (&(_exts)->nr_actions == 1)
 
 #else /* CONFIG_NET_CLS_ACT */
 
 #define tc_no_actions(_exts) true
-#define tc_for_each_action(_a, _exts) while ((void)(_a), 0)
 #define tc_single_action(_exts) false
 #define tcf_action_stats_update(a, bytes, packets, lastuse)
 

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 843a716..e690c3d 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c

@@ -541,8 +541,12 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
 void tcf_exts_destroy(struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	tcf_action_destroy(&exts->actions, TCA_ACT_UNBIND);
-	INIT_LIST_HEAD(&exts->actions);
+	LIST_HEAD(actions);
+
+	tcf_exts_to_list(exts, &actions);
+	tcf_action_destroy(&actions, TCA_ACT_UNBIND);
+	flex_array_free(exts->actions);
+	exts->nr_actions = 0;
 #endif
 }
 EXPORT_SYMBOL(tcf_exts_destroy);

@@ -554,7 +558,6 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 	{
 		struct tc_action *act;
 
-		INIT_LIST_HEAD(&exts->actions);
 		if (exts->police && tb[exts->police]) {
 			act = tcf_action_init_1(net, tb[exts->police], rate_tlv,
 						"police", ovr,

@@ -563,14 +566,26 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 				return PTR_ERR(act);
 
 			act->type = exts->type = TCA_OLD_COMPAT;
-			list_add(&act->list, &exts->actions);
+			flex_array_put(exts->actions, 0, &act, GFP_KERNEL);
+			exts->nr_actions = 1;
 		} else if (exts->action && tb[exts->action]) {
-			int err;
+			LIST_HEAD(actions);
+			int err, i = 0;
+
 			err = tcf_action_init(net, tb[exts->action], rate_tlv,
 					      NULL, ovr,
-					      TCA_ACT_BIND, &exts->actions);
+					      TCA_ACT_BIND, &actions);
 			if (err)
 				return err;
+			list_for_each_entry(act, &actions, list) {
+				err = flex_array_put(exts->actions, i++, &act,
+						     GFP_KERNEL);
+				if (err) {
+					tcf_action_destroy(&actions, TCA_ACT_UNBIND);
+					return err;
+				}
+			}
+			exts->nr_actions = i;
 		}
 	}
 #else

@@ -587,37 +602,49 @@ void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
 		     struct tcf_exts *src)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	LIST_HEAD(tmp);
+	struct tcf_exts old = *dst;
+
 	tcf_tree_lock(tp);
-	list_splice_init(&dst->actions, &tmp);
-	list_splice(&src->actions, &dst->actions);
+	dst->nr_actions = src->nr_actions;
+	dst->actions = src->actions;
 	dst->type = src->type;
 	tcf_tree_unlock(tp);
-	tcf_action_destroy(&tmp, TCA_ACT_UNBIND);
+
+	tcf_exts_destroy(&old);
 #endif
 }
 EXPORT_SYMBOL(tcf_exts_change);
 
-#define tcf_exts_first_act(ext)					\
-	list_first_entry_or_null(&(exts)->actions,		\
-				 struct tc_action, list)
+#ifdef CONFIG_NET_CLS_ACT
+static struct tc_action *tcf_exts_first_act(struct tcf_exts *exts)
+{
+	if (exts->nr_actions == 0)
+		return NULL;
+	else
+		return *(struct tc_action **)flex_array_get(exts->actions, 0);
+}
+#endif
 
 int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	struct nlattr *nest;
 
-	if (exts->action && !list_empty(&exts->actions)) {
+	if (exts->action && exts->nr_actions) {
 		/*
 		 * again for backward compatible mode - we want
 		 * to work with both old and new modes of entering
 		 * tc data even if iproute2  was newer - jhs
 		 */
 		if (exts->type != TCA_OLD_COMPAT) {
+			LIST_HEAD(actions);
+
 			nest = nla_nest_start(skb, exts->action);
 			if (nest == NULL)
 				goto nla_put_failure;
-			if (tcf_action_dump(skb, &exts->actions, 0, 0) < 0)
+
+			tcf_exts_to_list(exts, &actions);
+			if (tcf_action_dump(skb, &actions, 0, 0) < 0)
 				goto nla_put_failure;
 			nla_nest_end(skb, nest);
 		} else if (exts->police) {

-- 
2.1.0

On Mon, Aug 08, 2016 at 01:46:47PM -0700, Cong Wang wrote:

It is harmless because all users pass 'a' to this macro.

Fixes: 00175aec941e ("net/sched: Macro instead of CONFIG_NET_CLS_ACT ifdef")
Cc: Amir Vadai <redacted>
Signed-off-by: Cong Wang <redacted>
---

Acked-by: Amir Vadai <redacted>

Thanks Cong.

On Mon, Aug 8, 2016 at 11:46 PM, Cong Wang [off-list ref] wrote:

As pointed out by Jamal, an action could be shared by
multiple filters, so we can't use list to chain them
any more after we get rid of the original tc_action.
Instead, we could just save pointers to these actions
in tcf_exts, since they are refcount'ed, so convert
the list to a flex array.

The ugly part is the action API still accepts list
as a parameter, I just introduce a helper function to
convert the flex array of pointers to a list.

Fixes: a85a970af265 ("net_sched: move tc_action into tcf_common")
Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---

[...]

-#define tc_single_action(_exts) \
-       (list_is_singular(&(_exts)->actions))
+#define tc_no_actions(_exts)  (&(_exts)->nr_actions == 0)
+#define tc_single_action(_exts) (&(_exts)->nr_actions == 1)

Should remove the '&' here.

Amir

[...]

On Tue, Aug 9, 2016 at 1:03 AM, Amir Vadai [off-list ref] wrote:

quoted

-#define tc_single_action(_exts) \
-       (list_is_singular(&(_exts)->actions))
+#define tc_no_actions(_exts)  (&(_exts)->nr_actions == 0)
+#define tc_single_action(_exts) (&(_exts)->nr_actions == 1)

Should remove the '&' here.

Good catch! I even didn't notice the '&' there. :-/

I will wait for Jamal's comments before sending v2.

Thanks.

On 16-08-08 04:46 PM, Cong Wang wrote:

After refactoring tc_action into tcf_common, we no
longer need to cleanup temporary "actions" in list,
they are permanently stored in the hashtable.

Fixes: a85a970af265 ("net_sched: move tc_action into tcf_common")
Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>

Cong I will test these patches and provide feedback by end of day
or tommorrow morning.

cheers,
jamal

On 16-08-08 04:46 PM, Cong Wang wrote:

This patchset fixes several regressions caused by the previous
code refactor. Thanks to Jamal for catching them!

Note, patch 3/5 and 4/5 are not strictly necessary, I just
want to carry them together.

Cong - there's good news and bad news.
The good news is that the oopses are fixed.
The bad news is you have now slowed down the system. It is noticeable
at high speed.
I narrowed it down to your use of flex arrays. In particular
tcf_exts_exec() call:
This is the fast path - was flexarray really necessary?
The conversion to list is slowing things down.

As hard as this is for me to say:
I am actually beginning to question this whole patch series.
Either you have a plan to fix this regression or lets just
pull this out for now to regain stability until we get our
act together. I think it would make a lot of sense to just pass
an array instead of a list.

cheers,
jamal

Sorry I have a really bad (expensive) connection and the SMTP
server seems to reject the first message and i retyped it and
sent again - and now notice both were sent.
Message didnt change ;->

cheers,
jamal

On 16-08-08 04:46 PM, Cong Wang wrote:

This patchset fixes several regressions caused by the previous
code refactor. Thanks to Jamal for catching them!

Cong,

Good news: oops gone. I havent done more testing than I did
before; but looks good so far.

Bad news: You have introduced a performance regression which is
noticeable at high speed.

tcf_exts_exec() is the culprit - and conversion to from flexarray
to linked list in the fast problem to be specific.
The regression is problematic (and unacceptable). Two options:
a) You fix the regressions - which i think may require changing
what gets passed around an executed on as an array instead of a
list.
b) I am worried #a will take some work. So the second option is
to back out the patch since there are known stability options;
get regression issues resolved and then go back and submit.

cheers,
jamla

On Wed, Aug 10, 2016 at 7:34 AM, Jamal Hadi Salim [off-list ref] wrote:

On 16-08-08 04:46 PM, Cong Wang wrote:

quoted

This patchset fixes several regressions caused by the previous
code refactor. Thanks to Jamal for catching them!

Cong,

Good news: oops gone. I havent done more testing than I did
before; but looks good so far.

Bad news: You have introduced a performance regression which is
noticeable at high speed.

tcf_exts_exec() is the culprit - and conversion to from flexarray
to linked list in the fast problem to be specific.

Ah, this reminds me that I don't have to use flex_array, initially
I thought the tcf_exts could hold as many actions as it wants,
but actually there is a upper bound, TCA_ACT_MAX_PRIO.
IOW, a regular dynamic array is just enough here.

I just replaced the flex_array with a regular one, it works fine
for me too, at least no crash with all of my test cases.

Please try v2, since you have more test cases that I do.
Or it would be great if you can share your test cases with
me or us.

Be patient, every big change could have regression. :)

Thanks.

On 16-08-10 04:06 PM, Cong Wang wrote:

On Wed, Aug 10, 2016 at 7:34 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-08-08 04:46 PM, Cong Wang wrote:

quoted

tcf_exts_exec() is the culprit - and conversion to from flexarray
to linked list in the fast problem to be specific.

Ah, this reminds me that I don't have to use flex_array, initially
I thought the tcf_exts could hold as many actions as it wants,
but actually there is a upper bound, TCA_ACT_MAX_PRIO.
IOW, a regular dynamic array is just enough here.

Yes, a regular array would be enough.

I just replaced the flex_array with a regular one, it works fine
for me too, at least no crash with all of my test cases.

Ok, I did a quick look at your patch - I still see you converting
from array to list. I mean get tcf_exts_exec() to take an array
and walk it. That will restore the perf numbers to the same level.

Please try v2, since you have more test cases that I do.
Or it would be great if you can share your test cases with
me or us.

Be patient, every big change could have regression. :)

No problem Cong - except we have a kernel that crashes right now.
BTW: I just thought of another test which uses a different code
path
# add a policer rule
sudo $TC actions add action police rate 1kbit burst 90k drop
#dump rules..
sudo $TC -s actions ls action police

cheers,
jamal

On Thu, Aug 11, 2016 at 9:20 AM, Jamal Hadi Salim [off-list ref] wrote:

On 16-08-10 04:06 PM, Cong Wang wrote:

quoted

On Wed, Aug 10, 2016 at 7:34 AM, Jamal Hadi Salim [off-list ref]
wrote:

quoted

On 16-08-08 04:46 PM, Cong Wang wrote:

quoted

quoted

tcf_exts_exec() is the culprit - and conversion to from flexarray
to linked list in the fast problem to be specific.

Ah, this reminds me that I don't have to use flex_array, initially
I thought the tcf_exts could hold as many actions as it wants,
but actually there is a upper bound, TCA_ACT_MAX_PRIO.
IOW, a regular dynamic array is just enough here.

Yes, a regular array would be enough.

quoted

I just replaced the flex_array with a regular one, it works fine
for me too, at least no crash with all of my test cases.

Ok, I did a quick look at your patch - I still see you converting
from array to list. I mean get tcf_exts_exec() to take an array
and walk it. That will restore the perf numbers to the same level.

Makes sense! I just did this change as you suggest.

quoted

Please try v2, since you have more test cases that I do.
Or it would be great if you can share your test cases with
me or us.

Be patient, every big change could have regression. :)

No problem Cong - except we have a kernel that crashes right now.
BTW: I just thought of another test which uses a different code
path
# add a policer rule
sudo $TC actions add action police rate 1kbit burst 90k drop
#dump rules..
sudo $TC -s actions ls action police

Passed:

[root@localhost ~]# tc actions add action police rate 1kbit burst 90k drop
[root@localhost ~]# tc actions ls action police

action order 0:  police 0x1 rate 1000bit burst 23440b mtu 2Kb action
drop overhead 0b
ref 1 bind 0
[root@localhost ~]# tc -s actions ls action police

action order 0:  police 0x1 rate 1000bit burst 23440b mtu 2Kb action
drop overhead 0b
ref 1 bind 0
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0


Thanks.

On 16-08-11 08:08 PM, Cong Wang wrote:

On Thu, Aug 11, 2016 at 9:20 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-08-10 04:06 PM, Cong Wang wrote:

quoted

Ok, I did a quick look at your patch - I still see you converting
from array to list. I mean get tcf_exts_exec() to take an array
and walk it. That will restore the perf numbers to the same level.

Makes sense! I just did this change as you suggest.

Ok, took look at it. Looks promising! Thanks Cong. I will test
and get back to you (sorry, likely by end of day)

cheers,
jamal

On 16-08-11 08:08 PM, Cong Wang wrote:

On Thu, Aug 11, 2016 at 9:20 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-08-10 04:06 PM, Cong Wang wrote:

quoted

On Wed, Aug 10, 2016 at 7:34 AM, Jamal Hadi Salim [off-list ref]
wrote:

quoted

On 16-08-08 04:46 PM, Cong Wang wrote:

quoted

quoted

tcf_exts_exec() is the culprit - and conversion to from flexarray
to linked list in the fast problem to be specific.

Ah, this reminds me that I don't have to use flex_array, initially
I thought the tcf_exts could hold as many actions as it wants,
but actually there is a upper bound, TCA_ACT_MAX_PRIO.
IOW, a regular dynamic array is just enough here.

Yes, a regular array would be enough.

quoted

I just replaced the flex_array with a regular one, it works fine
for me too, at least no crash with all of my test cases.

quoted

No problem Cong - except we have a kernel that crashes right now.
BTW: I just thought of another test which uses a different code
path
# add a policer rule
sudo $TC actions add action police rate 1kbit burst 90k drop
#dump rules..
sudo $TC -s actions ls action police

I tested a lot more this time.
Good news: performance regression now resolved.
Some bad news - there's still one more oops:

sudo $TC actions add action police rate 1kbit burst 90k drop index 1

note how i explicitly specified the index.
If i leave out the index, all works fine. I'll continue to see
if there are any other issue for the next while and will email.
I think you are close so  I will also make small comments on the
patches because you are going to make another update.

cheers,
jamal

On Sat, Aug 13, 2016 at 4:05 AM, Jamal Hadi Salim [off-list ref] wrote:

I tested a lot more this time.
Good news: performance regression now resolved.
Some bad news - there's still one more oops:

sudo $TC actions add action police rate 1kbit burst 90k drop index 1

Relax, Jamal, it is caused by other commit, not my previous one.

But I can definitely fix it since you are so busy. ;)