netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch net 0/9] net_sched: pending clean up and bug fixes
@ 2018-08-19 19:22 Cong Wang
  2018-08-19 19:22 ` [Patch net 1/9] net_sched: improve and refactor tcf_action_put_many() Cong Wang
                   ` (9 more replies)
  0 siblings, 10 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang

This patchset aims to clean up and fixes some bugs in current
merge window, this is why it is targeting -net.

Patch 1-5 are clean up Vlad's patches merged in current merge
window, patch 6 is just a trivial cleanup.

Patch 7 reverts a lockdep warning fix and patch 8 provides a better
fix for it.

Patch 9 fixes a potential deadlock found by me during code review.

Please see each patch for details.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Cong Wang (9):
  net_sched: improve and refactor tcf_action_put_many()
  net_sched: remove unnecessary ops->delete()
  net_sched: remove unused parameter for tcf_action_delete()
  net_sched: remove unused tcf_idr_check()
  net_sched: remove list_head from tc_action
  net_sched: remove unused tcfa_capab
  Revert "net: sched: act_ife: disable bh when taking ife_mod_lock"
  act_ife: move tcfa_lock down to where necessary
  act_ife: fix a potential deadlock

 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c       |  6 +-
 .../net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c   | 10 +--
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c  |  5 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |  6 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    | 19 +++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     |  3 +-
 .../net/ethernet/mellanox/mlxsw/spectrum_flower.c  |  6 +-
 drivers/net/ethernet/netronome/nfp/flower/action.c |  6 +-
 drivers/net/ethernet/qlogic/qede/qede_filter.c     |  6 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  5 +-
 include/net/act_api.h                              |  7 --
 include/net/pkt_cls.h                              | 25 +++---
 net/dsa/slave.c                                    |  4 +-
 net/sched/act_api.c                                | 70 ++++++----------
 net/sched/act_bpf.c                                |  8 --
 net/sched/act_connmark.c                           |  8 --
 net/sched/act_csum.c                               |  8 --
 net/sched/act_gact.c                               |  8 --
 net/sched/act_ife.c                                | 92 ++++++++++------------
 net/sched/act_ipt.c                                | 16 ----
 net/sched/act_mirred.c                             |  8 --
 net/sched/act_nat.c                                |  8 --
 net/sched/act_pedit.c                              |  8 --
 net/sched/act_police.c                             |  8 --
 net/sched/act_sample.c                             |  8 --
 net/sched/act_simple.c                             |  8 --
 net/sched/act_skbedit.c                            |  8 --
 net/sched/act_skbmod.c                             |  8 --
 net/sched/act_tunnel_key.c                         |  8 --
 net/sched/act_vlan.c                               |  8 --
 30 files changed, 108 insertions(+), 290 deletions(-)

-- 
2.14.4

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Patch net 1/9] net_sched: improve and refactor tcf_action_put_many()
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-19 19:22 ` [Patch net 2/9] net_sched: remove unnecessary ops->delete() Cong Wang
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang, Jiri Pirko, Vlad Buslov

tcf_action_put_many() is mostly called to clean up actions on
failure path, but tcf_action_put_many(&actions[acts_deleted]) is
used in the ugliest way: it passes a slice of the array and
uses an additional NULL at the end to avoid out-of-bound
access.

acts_deleted is completely unnecessary since we can teach
tcf_action_put_many() scan the whole array and checks against
NULL pointer. Which also means tcf_action_delete() should
set deleted action pointers to NULL to avoid double free.

Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers to actions")
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_api.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 229d63c99be2..cd69a6afcf88 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -686,14 +686,18 @@ static int tcf_action_put(struct tc_action *p)
 	return __tcf_action_put(p, false);
 }
 
+/* Put all actions in this array, skip those NULL's. */
 static void tcf_action_put_many(struct tc_action *actions[])
 {
 	int i;
 
-	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
 		struct tc_action *a = actions[i];
-		const struct tc_action_ops *ops = a->ops;
+		const struct tc_action_ops *ops;
 
+		if (!a)
+			continue;
+		ops = a->ops;
 		if (tcf_action_put(a))
 			module_put(ops->owner);
 	}
@@ -1176,7 +1180,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 }
 
 static int tcf_action_delete(struct net *net, struct tc_action *actions[],
-			     int *acts_deleted, struct netlink_ext_ack *extack)
+			     struct netlink_ext_ack *extack)
 {
 	u32 act_index;
 	int ret, i;
@@ -1196,20 +1200,17 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[],
 		} else  {
 			/* now do the delete */
 			ret = ops->delete(net, act_index);
-			if (ret < 0) {
-				*acts_deleted = i + 1;
+			if (ret < 0)
 				return ret;
-			}
 		}
+		actions[i] = NULL;
 	}
-	*acts_deleted = i;
 	return 0;
 }
 
 static int
 tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
-	       int *acts_deleted, u32 portid, size_t attr_size,
-	       struct netlink_ext_ack *extack)
+	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
 {
 	int ret;
 	struct sk_buff *skb;
@@ -1227,7 +1228,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
 	}
 
 	/* now do the delete */
-	ret = tcf_action_delete(net, actions, acts_deleted, extack);
+	ret = tcf_action_delete(net, actions, extack);
 	if (ret < 0) {
 		NL_SET_ERR_MSG(extack, "Failed to delete TC action");
 		kfree_skb(skb);
@@ -1249,8 +1250,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	struct tc_action *act;
 	size_t attr_size = 0;
-	struct tc_action *actions[TCA_ACT_MAX_PRIO + 1] = {};
-	int acts_deleted = 0;
+	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
 
 	ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
 	if (ret < 0)
@@ -1280,14 +1280,13 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	if (event == RTM_GETACTION)
 		ret = tcf_get_notify(net, portid, n, actions, event, extack);
 	else { /* delete */
-		ret = tcf_del_notify(net, n, actions, &acts_deleted, portid,
-				     attr_size, extack);
+		ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
 		if (ret)
 			goto err;
-		return ret;
+		return 0;
 	}
 err:
-	tcf_action_put_many(&actions[acts_deleted]);
+	tcf_action_put_many(actions);
 	return ret;
 }
 
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 2/9] net_sched: remove unnecessary ops->delete()
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
  2018-08-19 19:22 ` [Patch net 1/9] net_sched: improve and refactor tcf_action_put_many() Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-19 19:22 ` [Patch net 3/9] net_sched: remove unused parameter for tcf_action_delete() Cong Wang
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang, Jiri Pirko, Vlad Buslov

All ops->delete() wants is getting the tn->idrinfo, but we already
have tc_action before calling ops->delete(), and tc_action has
a pointer ->idrinfo.

More importantly, each type of action does the same thing, that is,
just calling tcf_idr_delete_index().

So it can be just removed.

Fixes: b409074e6693 ("net: sched: add 'delete' function to action ops")
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h      |  2 --
 net/sched/act_api.c        | 15 +++++++--------
 net/sched/act_bpf.c        |  8 --------
 net/sched/act_connmark.c   |  8 --------
 net/sched/act_csum.c       |  8 --------
 net/sched/act_gact.c       |  8 --------
 net/sched/act_ife.c        |  8 --------
 net/sched/act_ipt.c        | 16 ----------------
 net/sched/act_mirred.c     |  8 --------
 net/sched/act_nat.c        |  8 --------
 net/sched/act_pedit.c      |  8 --------
 net/sched/act_police.c     |  8 --------
 net/sched/act_sample.c     |  8 --------
 net/sched/act_simple.c     |  8 --------
 net/sched/act_skbedit.c    |  8 --------
 net/sched/act_skbmod.c     |  8 --------
 net/sched/act_tunnel_key.c |  8 --------
 net/sched/act_vlan.c       |  8 --------
 18 files changed, 7 insertions(+), 146 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1ad5b19e83a9..e32708491d83 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -102,7 +102,6 @@ struct tc_action_ops {
 	size_t  (*get_fill_size)(const struct tc_action *act);
 	struct net_device *(*get_dev)(const struct tc_action *a);
 	void	(*put_dev)(struct net_device *dev);
-	int     (*delete)(struct net *net, u32 index);
 };
 
 struct tc_action_net {
@@ -158,7 +157,6 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
 void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
 int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
 			struct tc_action **a, int bind);
-int tcf_idr_delete_index(struct tc_action_net *tn, u32 index);
 int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
 
 static inline int tcf_idr_release(struct tc_action *a, bool bind)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index cd69a6afcf88..00bf7d2b0bdd 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -337,9 +337,8 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
 }
 EXPORT_SYMBOL(tcf_idr_check);
 
-int tcf_idr_delete_index(struct tc_action_net *tn, u32 index)
+static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
 {
-	struct tcf_idrinfo *idrinfo = tn->idrinfo;
 	struct tc_action *p;
 	int ret = 0;
 
@@ -370,7 +369,6 @@ int tcf_idr_delete_index(struct tc_action_net *tn, u32 index)
 	spin_unlock(&idrinfo->lock);
 	return ret;
 }
-EXPORT_SYMBOL(tcf_idr_delete_index);
 
 int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
 		   struct tc_action **a, const struct tc_action_ops *ops,
@@ -1182,24 +1180,25 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 static int tcf_action_delete(struct net *net, struct tc_action *actions[],
 			     struct netlink_ext_ack *extack)
 {
-	u32 act_index;
-	int ret, i;
+	int i;
 
 	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
 		struct tc_action *a = actions[i];
 		const struct tc_action_ops *ops = a->ops;
-
 		/* Actions can be deleted concurrently so we must save their
 		 * type and id to search again after reference is released.
 		 */
-		act_index = a->tcfa_index;
+		struct tcf_idrinfo *idrinfo = a->idrinfo;
+		u32 act_index = a->tcfa_index;
 
 		if (tcf_action_put(a)) {
 			/* last reference, action was deleted concurrently */
 			module_put(ops->owner);
 		} else  {
+			int ret;
+
 			/* now do the delete */
-			ret = ops->delete(net, act_index);
+			ret = tcf_idr_delete_index(idrinfo, act_index);
 			if (ret < 0)
 				return ret;
 		}
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index d30b23e42436..0c68bc9cf0b4 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -395,13 +395,6 @@ static int tcf_bpf_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_bpf_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, bpf_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_bpf_ops __read_mostly = {
 	.kind		=	"bpf",
 	.type		=	TCA_ACT_BPF,
@@ -412,7 +405,6 @@ static struct tc_action_ops act_bpf_ops __read_mostly = {
 	.init		=	tcf_bpf_init,
 	.walk		=	tcf_bpf_walker,
 	.lookup		=	tcf_bpf_search,
-	.delete		=	tcf_bpf_delete,
 	.size		=	sizeof(struct tcf_bpf),
 };
 
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 54c0bf54f2ac..6f0f273f1139 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -198,13 +198,6 @@ static int tcf_connmark_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_connmark_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, connmark_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_connmark_ops = {
 	.kind		=	"connmark",
 	.type		=	TCA_ACT_CONNMARK,
@@ -214,7 +207,6 @@ static struct tc_action_ops act_connmark_ops = {
 	.init		=	tcf_connmark_init,
 	.walk		=	tcf_connmark_walker,
 	.lookup		=	tcf_connmark_search,
-	.delete		=	tcf_connmark_delete,
 	.size		=	sizeof(struct tcf_connmark_info),
 };
 
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index e698d3fe2080..b8a67ae3105a 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -659,13 +659,6 @@ static size_t tcf_csum_get_fill_size(const struct tc_action *act)
 	return nla_total_size(sizeof(struct tc_csum));
 }
 
-static int tcf_csum_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, csum_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_csum_ops = {
 	.kind		= "csum",
 	.type		= TCA_ACT_CSUM,
@@ -677,7 +670,6 @@ static struct tc_action_ops act_csum_ops = {
 	.walk		= tcf_csum_walker,
 	.lookup		= tcf_csum_search,
 	.get_fill_size  = tcf_csum_get_fill_size,
-	.delete		= tcf_csum_delete,
 	.size		= sizeof(struct tcf_csum),
 };
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 6a3f25a8ffb3..cd1d9bd32ef9 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -243,13 +243,6 @@ static size_t tcf_gact_get_fill_size(const struct tc_action *act)
 	return sz;
 }
 
-static int tcf_gact_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, gact_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_gact_ops = {
 	.kind		=	"gact",
 	.type		=	TCA_ACT_GACT,
@@ -261,7 +254,6 @@ static struct tc_action_ops act_gact_ops = {
 	.walk		=	tcf_gact_walker,
 	.lookup		=	tcf_gact_search,
 	.get_fill_size	=	tcf_gact_get_fill_size,
-	.delete		=	tcf_gact_delete,
 	.size		=	sizeof(struct tcf_gact),
 };
 
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index d1081bdf1bdb..92fcf8ba5bca 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -853,13 +853,6 @@ static int tcf_ife_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_ife_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, ife_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_ife_ops = {
 	.kind = "ife",
 	.type = TCA_ACT_IFE,
@@ -870,7 +863,6 @@ static struct tc_action_ops act_ife_ops = {
 	.init = tcf_ife_init,
 	.walk = tcf_ife_walker,
 	.lookup = tcf_ife_search,
-	.delete = tcf_ife_delete,
 	.size =	sizeof(struct tcf_ife_info),
 };
 
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 51f235bbeb5b..23273b5303fd 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -337,13 +337,6 @@ static int tcf_ipt_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_ipt_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, ipt_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_ipt_ops = {
 	.kind		=	"ipt",
 	.type		=	TCA_ACT_IPT,
@@ -354,7 +347,6 @@ static struct tc_action_ops act_ipt_ops = {
 	.init		=	tcf_ipt_init,
 	.walk		=	tcf_ipt_walker,
 	.lookup		=	tcf_ipt_search,
-	.delete		=	tcf_ipt_delete,
 	.size		=	sizeof(struct tcf_ipt),
 };
 
@@ -395,13 +387,6 @@ static int tcf_xt_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_xt_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, xt_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_xt_ops = {
 	.kind		=	"xt",
 	.type		=	TCA_ACT_XT,
@@ -412,7 +397,6 @@ static struct tc_action_ops act_xt_ops = {
 	.init		=	tcf_xt_init,
 	.walk		=	tcf_xt_walker,
 	.lookup		=	tcf_xt_search,
-	.delete		=	tcf_xt_delete,
 	.size		=	sizeof(struct tcf_ipt),
 };
 
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 38fd20f10f67..8bf66d0a6800 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -395,13 +395,6 @@ static void tcf_mirred_put_dev(struct net_device *dev)
 	dev_put(dev);
 }
 
-static int tcf_mirred_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, mirred_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_mirred_ops = {
 	.kind		=	"mirred",
 	.type		=	TCA_ACT_MIRRED,
@@ -416,7 +409,6 @@ static struct tc_action_ops act_mirred_ops = {
 	.size		=	sizeof(struct tcf_mirred),
 	.get_dev	=	tcf_mirred_get_dev,
 	.put_dev	=	tcf_mirred_put_dev,
-	.delete		=	tcf_mirred_delete,
 };
 
 static __net_init int mirred_init_net(struct net *net)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 822e903bfc25..4313aa102440 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -300,13 +300,6 @@ static int tcf_nat_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_nat_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, nat_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_nat_ops = {
 	.kind		=	"nat",
 	.type		=	TCA_ACT_NAT,
@@ -316,7 +309,6 @@ static struct tc_action_ops act_nat_ops = {
 	.init		=	tcf_nat_init,
 	.walk		=	tcf_nat_walker,
 	.lookup		=	tcf_nat_search,
-	.delete		=	tcf_nat_delete,
 	.size		=	sizeof(struct tcf_nat),
 };
 
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 8a7a7cb94e83..107034070019 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -460,13 +460,6 @@ static int tcf_pedit_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_pedit_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, pedit_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_pedit_ops = {
 	.kind		=	"pedit",
 	.type		=	TCA_ACT_PEDIT,
@@ -477,7 +470,6 @@ static struct tc_action_ops act_pedit_ops = {
 	.init		=	tcf_pedit_init,
 	.walk		=	tcf_pedit_walker,
 	.lookup		=	tcf_pedit_search,
-	.delete		=	tcf_pedit_delete,
 	.size		=	sizeof(struct tcf_pedit),
 };
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 06f0742db593..5d8bfa878477 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -320,13 +320,6 @@ static int tcf_police_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_police_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, police_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 MODULE_AUTHOR("Alexey Kuznetsov");
 MODULE_DESCRIPTION("Policing actions");
 MODULE_LICENSE("GPL");
@@ -340,7 +333,6 @@ static struct tc_action_ops act_police_ops = {
 	.init		=	tcf_police_init,
 	.walk		=	tcf_police_walker,
 	.lookup		=	tcf_police_search,
-	.delete		=	tcf_police_delete,
 	.size		=	sizeof(struct tcf_police),
 };
 
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 207b4132d1b0..44e9c00657bc 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -232,13 +232,6 @@ static int tcf_sample_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_sample_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, sample_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_sample_ops = {
 	.kind	  = "sample",
 	.type	  = TCA_ACT_SAMPLE,
@@ -249,7 +242,6 @@ static struct tc_action_ops act_sample_ops = {
 	.cleanup  = tcf_sample_cleanup,
 	.walk	  = tcf_sample_walker,
 	.lookup	  = tcf_sample_search,
-	.delete	  = tcf_sample_delete,
 	.size	  = sizeof(struct tcf_sample),
 };
 
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index e616523ba3c1..52400d49f81f 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -196,13 +196,6 @@ static int tcf_simp_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_simp_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, simp_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_simp_ops = {
 	.kind		=	"simple",
 	.type		=	TCA_ACT_SIMP,
@@ -213,7 +206,6 @@ static struct tc_action_ops act_simp_ops = {
 	.init		=	tcf_simp_init,
 	.walk		=	tcf_simp_walker,
 	.lookup		=	tcf_simp_search,
-	.delete		=	tcf_simp_delete,
 	.size		=	sizeof(struct tcf_defact),
 };
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 926d7bc4a89d..73e44ce2a883 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -299,13 +299,6 @@ static int tcf_skbedit_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_skbedit_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, skbedit_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_skbedit_ops = {
 	.kind		=	"skbedit",
 	.type		=	TCA_ACT_SKBEDIT,
@@ -316,7 +309,6 @@ static struct tc_action_ops act_skbedit_ops = {
 	.cleanup	=	tcf_skbedit_cleanup,
 	.walk		=	tcf_skbedit_walker,
 	.lookup		=	tcf_skbedit_search,
-	.delete		=	tcf_skbedit_delete,
 	.size		=	sizeof(struct tcf_skbedit),
 };
 
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index d6a1af0c4171..588077fafd6c 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -259,13 +259,6 @@ static int tcf_skbmod_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_skbmod_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_skbmod_ops = {
 	.kind		=	"skbmod",
 	.type		=	TCA_ACT_SKBMOD,
@@ -276,7 +269,6 @@ static struct tc_action_ops act_skbmod_ops = {
 	.cleanup	=	tcf_skbmod_cleanup,
 	.walk		=	tcf_skbmod_walker,
 	.lookup		=	tcf_skbmod_search,
-	.delete		=	tcf_skbmod_delete,
 	.size		=	sizeof(struct tcf_skbmod),
 };
 
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 8f09cf08d8fe..420759153d5f 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -548,13 +548,6 @@ static int tunnel_key_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tunnel_key_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_tunnel_key_ops = {
 	.kind		=	"tunnel_key",
 	.type		=	TCA_ACT_TUNNEL_KEY,
@@ -565,7 +558,6 @@ static struct tc_action_ops act_tunnel_key_ops = {
 	.cleanup	=	tunnel_key_release,
 	.walk		=	tunnel_key_walker,
 	.lookup		=	tunnel_key_search,
-	.delete		=	tunnel_key_delete,
 	.size		=	sizeof(struct tcf_tunnel_key),
 };
 
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 209e70ad2c09..033d273afe50 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -296,13 +296,6 @@ static int tcf_vlan_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
-static int tcf_vlan_delete(struct net *net, u32 index)
-{
-	struct tc_action_net *tn = net_generic(net, vlan_net_id);
-
-	return tcf_idr_delete_index(tn, index);
-}
-
 static struct tc_action_ops act_vlan_ops = {
 	.kind		=	"vlan",
 	.type		=	TCA_ACT_VLAN,
@@ -313,7 +306,6 @@ static struct tc_action_ops act_vlan_ops = {
 	.cleanup	=	tcf_vlan_cleanup,
 	.walk		=	tcf_vlan_walker,
 	.lookup		=	tcf_vlan_search,
-	.delete		=	tcf_vlan_delete,
 	.size		=	sizeof(struct tcf_vlan),
 };
 
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 3/9] net_sched: remove unused parameter for tcf_action_delete()
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
  2018-08-19 19:22 ` [Patch net 1/9] net_sched: improve and refactor tcf_action_put_many() Cong Wang
  2018-08-19 19:22 ` [Patch net 2/9] net_sched: remove unnecessary ops->delete() Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-19 19:22 ` [Patch net 4/9] net_sched: remove unused tcf_idr_check() Cong Wang
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang, Jiri Pirko, Vlad Buslov

Fixes: 16af6067392c ("net: sched: implement reference counted action release")
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_api.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 00bf7d2b0bdd..ba55226928a3 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1177,8 +1177,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 	return err;
 }
 
-static int tcf_action_delete(struct net *net, struct tc_action *actions[],
-			     struct netlink_ext_ack *extack)
+static int tcf_action_delete(struct net *net, struct tc_action *actions[])
 {
 	int i;
 
@@ -1227,7 +1226,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
 	}
 
 	/* now do the delete */
-	ret = tcf_action_delete(net, actions, extack);
+	ret = tcf_action_delete(net, actions);
 	if (ret < 0) {
 		NL_SET_ERR_MSG(extack, "Failed to delete TC action");
 		kfree_skb(skb);
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 4/9] net_sched: remove unused tcf_idr_check()
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
                   ` (2 preceding siblings ...)
  2018-08-19 19:22 ` [Patch net 3/9] net_sched: remove unused parameter for tcf_action_delete() Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-19 19:22 ` [Patch net 5/9] net_sched: remove list_head from tc_action Cong Wang
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang, Jiri Pirko, Vlad Buslov

tcf_idr_check() is replaced by tcf_idr_check_alloc(),
and __tcf_idr_check() now can be folded into tcf_idr_search().

Fixes: 0190c1d452a9 ("net: sched: atomically check-allocate action")
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h |  2 --
 net/sched/act_api.c   | 22 +++-------------------
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index e32708491d83..eaa0e8b93d5b 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -147,8 +147,6 @@ int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
 		       const struct tc_action_ops *ops,
 		       struct netlink_ext_ack *extack);
 int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index);
-bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
-		    int bind);
 int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
 		   struct tc_action **a, const struct tc_action_ops *ops,
 		   int bind, bool cpustats);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index ba55226928a3..d76948f02a02 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -300,21 +300,17 @@ int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(tcf_generic_walker);
 
-static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
-			    struct tc_action **a, int bind)
+int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
 {
 	struct tcf_idrinfo *idrinfo = tn->idrinfo;
 	struct tc_action *p;
 
 	spin_lock(&idrinfo->lock);
 	p = idr_find(&idrinfo->action_idr, index);
-	if (IS_ERR(p)) {
+	if (IS_ERR(p))
 		p = NULL;
-	} else if (p) {
+	else if (p)
 		refcount_inc(&p->tcfa_refcnt);
-		if (bind)
-			atomic_inc(&p->tcfa_bindcnt);
-	}
 	spin_unlock(&idrinfo->lock);
 
 	if (p) {
@@ -323,20 +319,8 @@ static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
 	}
 	return false;
 }
-
-int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
-{
-	return __tcf_idr_check(tn, index, a, 0);
-}
 EXPORT_SYMBOL(tcf_idr_search);
 
-bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
-		   int bind)
-{
-	return __tcf_idr_check(tn, index, a, bind);
-}
-EXPORT_SYMBOL(tcf_idr_check);
-
 static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
 {
 	struct tc_action *p;
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 5/9] net_sched: remove list_head from tc_action
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
                   ` (3 preceding siblings ...)
  2018-08-19 19:22 ` [Patch net 4/9] net_sched: remove unused tcf_idr_check() Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-19 19:22 ` [Patch net 6/9] net_sched: remove unused tcfa_capab Cong Wang
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang, Jiri Pirko, Vlad Buslov

After commit 90b73b77d08e, list_head is no longer needed.
Now we just need to convert the list iteration to array
iteration for drivers.

Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers to actions")
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c       |  6 ++----
 .../net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c   | 10 ++++-----
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c  |  5 ++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |  6 ++----
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    | 19 ++++++++--------
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     |  3 +--
 .../net/ethernet/mellanox/mlxsw/spectrum_flower.c  |  6 ++----
 drivers/net/ethernet/netronome/nfp/flower/action.c |  6 ++----
 drivers/net/ethernet/qlogic/qede/qede_filter.c     |  6 ++----
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  5 ++---
 include/net/act_api.h                              |  1 -
 include/net/pkt_cls.h                              | 25 ++++++++++++----------
 net/dsa/slave.c                                    |  4 +---
 net/sched/act_api.c                                |  1 -
 14 files changed, 43 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 139d96c5a023..092c817f8f11 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -110,16 +110,14 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
 				 struct tcf_exts *tc_exts)
 {
 	const struct tc_action *tc_act;
-	LIST_HEAD(tc_actions);
-	int rc;
+	int i, rc;
 
 	if (!tcf_exts_has_actions(tc_exts)) {
 		netdev_info(bp->dev, "no actions");
 		return -EINVAL;
 	}
 
-	tcf_exts_to_list(tc_exts, &tc_actions);
-	list_for_each_entry(tc_act, &tc_actions, list) {
+	tcf_exts_for_each_action(i, tc_act, tc_exts) {
 		/* Drop action */
 		if (is_tcf_gact_shot(tc_act)) {
 			actions->flags |= BNXT_TC_ACTION_FLAG_DROP;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index 623f73dd7738..c116f96956fe 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -417,10 +417,9 @@ static void cxgb4_process_flow_actions(struct net_device *in,
 				       struct ch_filter_specification *fs)
 {
 	const struct tc_action *a;
-	LIST_HEAD(actions);
+	int i;
 
-	tcf_exts_to_list(cls->exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, cls->exts) {
 		if (is_tcf_gact_ok(a)) {
 			fs->action = FILTER_PASS;
 		} else if (is_tcf_gact_shot(a)) {
@@ -591,10 +590,9 @@ static int cxgb4_validate_flow_actions(struct net_device *dev,
 	bool act_redir = false;
 	bool act_pedit = false;
 	bool act_vlan = false;
-	LIST_HEAD(actions);
+	int i;
 
-	tcf_exts_to_list(cls->exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, cls->exts) {
 		if (is_tcf_gact_ok(a)) {
 			/* Do nothing */
 		} else if (is_tcf_gact_shot(a)) {
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
index 18eb2aedd4cb..c7d2b4dc7568 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
@@ -93,14 +93,13 @@ static int fill_action_fields(struct adapter *adap,
 	unsigned int num_actions = 0;
 	const struct tc_action *a;
 	struct tcf_exts *exts;
-	LIST_HEAD(actions);
+	int i;
 
 	exts = cls->knode.exts;
 	if (!tcf_exts_has_actions(exts))
 		return -EINVAL;
 
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, exts) {
 		/* Don't allow more than one action per rule. */
 		if (num_actions)
 			return -EINVAL;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 447098005490..af4c9ae7f432 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9171,14 +9171,12 @@ 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 i;
 
 	if (!tcf_exts_has_actions(exts))
 		return -EINVAL;
 
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(a, &actions, list) {
-
+	tcf_exts_for_each_action(i, a, exts) {
 		/* Drop action */
 		if (is_tcf_gact_shot(a)) {
 			*action = IXGBE_FDIR_DROP_QUEUE;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 9131a1376e7d..9fed54017659 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1982,14 +1982,15 @@ static bool modify_header_match_supported(struct mlx5_flow_spec *spec,
 		goto out_ok;
 
 	modify_ip_header = false;
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, exts) {
+		int k;
+
 		if (!is_tcf_pedit(a))
 			continue;
 
 		nkeys = tcf_pedit_nkeys(a);
-		for (i = 0; i < nkeys; i++) {
-			htype = tcf_pedit_htype(a, i);
+		for (k = 0; k < nkeys; k++) {
+			htype = tcf_pedit_htype(a, k);
 			if (htype == TCA_PEDIT_KEY_EX_HDR_TYPE_IP4 ||
 			    htype == TCA_PEDIT_KEY_EX_HDR_TYPE_IP6) {
 				modify_ip_header = true;
@@ -2053,15 +2054,14 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
 	const struct tc_action *a;
 	LIST_HEAD(actions);
 	u32 action = 0;
-	int err;
+	int err, i;
 
 	if (!tcf_exts_has_actions(exts))
 		return -EINVAL;
 
 	attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
 
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, exts) {
 		if (is_tcf_gact_shot(a)) {
 			action |= MLX5_FLOW_CONTEXT_ACTION_DROP;
 			if (MLX5_CAP_FLOWTABLE(priv->mdev,
@@ -2666,7 +2666,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
 	LIST_HEAD(actions);
 	bool encap = false;
 	u32 action = 0;
-	int err;
+	int err, i;
 
 	if (!tcf_exts_has_actions(exts))
 		return -EINVAL;
@@ -2674,8 +2674,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
 	attr->in_rep = rpriv->rep;
 	attr->in_mdev = priv->mdev;
 
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, exts) {
 		if (is_tcf_gact_shot(a)) {
 			action |= MLX5_FLOW_CONTEXT_ACTION_DROP |
 				  MLX5_FLOW_CONTEXT_ACTION_COUNT;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 6070d1591d1e..930700413b1d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -1346,8 +1346,7 @@ static int mlxsw_sp_port_add_cls_matchall(struct mlxsw_sp_port *mlxsw_sp_port,
 		return -ENOMEM;
 	mall_tc_entry->cookie = f->cookie;
 
-	tcf_exts_to_list(f->exts, &actions);
-	a = list_first_entry(&actions, struct tc_action, list);
+	a = tcf_exts_first_action(f->exts);
 
 	if (is_tcf_mirred_egress_mirror(a) && protocol == htons(ETH_P_ALL)) {
 		struct mlxsw_sp_port_mall_mirror_tc_entry *mirror;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index ebd1b24ebaa5..8d211972c5e9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -21,8 +21,7 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 					 struct netlink_ext_ack *extack)
 {
 	const struct tc_action *a;
-	LIST_HEAD(actions);
-	int err;
+	int err, i;
 
 	if (!tcf_exts_has_actions(exts))
 		return 0;
@@ -32,8 +31,7 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 	if (err)
 		return err;
 
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, exts) {
 		if (is_tcf_gact_ok(a)) {
 			err = mlxsw_sp_acl_rulei_act_terminate(rulei);
 			if (err) {
diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c b/drivers/net/ethernet/netronome/nfp/flower/action.c
index 0ba0356ec4e6..9044496803e6 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/action.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
@@ -796,11 +796,10 @@ int nfp_flower_compile_action(struct nfp_app *app,
 			      struct net_device *netdev,
 			      struct nfp_fl_payload *nfp_flow)
 {
-	int act_len, act_cnt, err, tun_out_cnt, out_cnt;
+	int act_len, act_cnt, err, tun_out_cnt, out_cnt, i;
 	enum nfp_flower_tun_type tun_type;
 	const struct tc_action *a;
 	u32 csum_updated = 0;
-	LIST_HEAD(actions);
 
 	memset(nfp_flow->action_data, 0, NFP_FL_MAX_A_SIZ);
 	nfp_flow->meta.act_len = 0;
@@ -810,8 +809,7 @@ int nfp_flower_compile_action(struct nfp_app *app,
 	tun_out_cnt = 0;
 	out_cnt = 0;
 
-	tcf_exts_to_list(flow->exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, flow->exts) {
 		err = nfp_flower_loop_action(app, a, flow, nfp_flow, &act_len,
 					     netdev, &tun_type, &tun_out_cnt,
 					     &out_cnt, &csum_updated);
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 9673d19308e6..b16ce7d93caf 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -2006,18 +2006,16 @@ int qede_get_arfs_filter_count(struct qede_dev *edev)
 static int qede_parse_actions(struct qede_dev *edev,
 			      struct tcf_exts *exts)
 {
-	int rc = -EINVAL, num_act = 0;
+	int rc = -EINVAL, num_act = 0, i;
 	const struct tc_action *a;
 	bool is_drop = false;
-	LIST_HEAD(actions);
 
 	if (!tcf_exts_has_actions(exts)) {
 		DP_NOTICE(edev, "No tc actions received\n");
 		return rc;
 	}
 
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(a, &actions, list) {
+	tcf_exts_for_each_action(i, a, exts) {
 		num_act++;
 
 		if (is_tcf_gact_shot(a))
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 1a96dd9c1091..531294f4978b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -61,7 +61,7 @@ static int tc_fill_actions(struct stmmac_tc_entry *entry,
 	struct stmmac_tc_entry *action_entry = entry;
 	const struct tc_action *act;
 	struct tcf_exts *exts;
-	LIST_HEAD(actions);
+	int i;
 
 	exts = cls->knode.exts;
 	if (!tcf_exts_has_actions(exts))
@@ -69,8 +69,7 @@ static int tc_fill_actions(struct stmmac_tc_entry *entry,
 	if (frag)
 		action_entry = frag;
 
-	tcf_exts_to_list(exts, &actions);
-	list_for_each_entry(act, &actions, list) {
+	tcf_exts_for_each_action(i, act, exts) {
 		/* Accept */
 		if (is_tcf_gact_ok(act)) {
 			action_entry->val.af = 1;
diff --git a/include/net/act_api.h b/include/net/act_api.h
index eaa0e8b93d5b..f9c4b871af88 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -23,7 +23,6 @@ struct tc_action {
 	const struct tc_action_ops	*ops;
 	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */
 	__u32				order;
-	struct list_head		list;
 	struct tcf_idrinfo		*idrinfo;
 
 	u32				tcfa_index;
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index ef727f71336e..c17d51865469 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -298,19 +298,13 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 #endif
 }
 
-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 = exts->actions[i];
-
-		list_add_tail(&a->list, actions);
-	}
+#define tcf_exts_for_each_action(i, a, exts) \
+	for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = (exts)->actions[i]); i++)
+#else
+#define tcf_exts_for_each_action(i, a, exts) \
+	for (; 0; )
 #endif
-}
 
 static inline void
 tcf_exts_stats_update(const struct tcf_exts *exts,
@@ -361,6 +355,15 @@ static inline bool tcf_exts_has_one_action(struct tcf_exts *exts)
 #endif
 }
 
+static inline struct tc_action *tcf_exts_first_action(struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	return exts->actions[0];
+#else
+	return NULL;
+#endif
+}
+
 /**
  * tcf_exts_exec - execute tc filter extensions
  * @skb: socket buffer
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 962c4fd338ba..1c45c1d6d241 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -767,7 +767,6 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 	const struct tc_action *a;
 	struct dsa_port *to_dp;
 	int err = -EOPNOTSUPP;
-	LIST_HEAD(actions);
 
 	if (!ds->ops->port_mirror_add)
 		return err;
@@ -775,8 +774,7 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 	if (!tcf_exts_has_one_action(cls->exts))
 		return err;
 
-	tcf_exts_to_list(cls->exts, &actions);
-	a = list_first_entry(&actions, struct tc_action, list);
+	a = tcf_exts_first_action(cls->exts);
 
 	if (is_tcf_mirred_egress_mirror(a) && protocol == htons(ETH_P_ALL)) {
 		struct dsa_mall_mirror_tc_entry *mirror;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d76948f02a02..db83dac1e7f4 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -391,7 +391,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
 
 	p->idrinfo = idrinfo;
 	p->ops = ops;
-	INIT_LIST_HEAD(&p->list);
 	*a = p;
 	return 0;
 err3:
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 6/9] net_sched: remove unused tcfa_capab
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
                   ` (4 preceding siblings ...)
  2018-08-19 19:22 ` [Patch net 5/9] net_sched: remove list_head from tc_action Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-19 19:22 ` [Patch net 7/9] Revert "net: sched: act_ife: disable bh when taking ife_mod_lock" Cong Wang
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index f9c4b871af88..970303448c90 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -28,7 +28,6 @@ struct tc_action {
 	u32				tcfa_index;
 	refcount_t			tcfa_refcnt;
 	atomic_t			tcfa_bindcnt;
-	u32				tcfa_capab;
 	int				tcfa_action;
 	struct tcf_t			tcfa_tm;
 	struct gnet_stats_basic_packed	tcfa_bstats;
@@ -43,7 +42,6 @@ struct tc_action {
 #define tcf_index	common.tcfa_index
 #define tcf_refcnt	common.tcfa_refcnt
 #define tcf_bindcnt	common.tcfa_bindcnt
-#define tcf_capab	common.tcfa_capab
 #define tcf_action	common.tcfa_action
 #define tcf_tm		common.tcfa_tm
 #define tcf_bstats	common.tcfa_bstats
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 7/9] Revert "net: sched: act_ife: disable bh when taking ife_mod_lock"
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
                   ` (5 preceding siblings ...)
  2018-08-19 19:22 ` [Patch net 6/9] net_sched: remove unused tcfa_capab Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-19 19:22 ` [Patch net 8/9] act_ife: move tcfa_lock down to where necessary Cong Wang
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang, Vlad Buslov

This reverts commit 42c625a486f3 ("net: sched: act_ife: disable bh
when taking ife_mod_lock"), because what ife_mod_lock protects
is absolutely not touched in rate est timer BH context, they have
no race.

A better fix is following up.

Cc: Vlad Buslov <vladbu@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_ife.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 92fcf8ba5bca..9decbb74b3ac 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -167,16 +167,16 @@ static struct tcf_meta_ops *find_ife_oplist(u16 metaid)
 {
 	struct tcf_meta_ops *o;
 
-	read_lock_bh(&ife_mod_lock);
+	read_lock(&ife_mod_lock);
 	list_for_each_entry(o, &ifeoplist, list) {
 		if (o->metaid == metaid) {
 			if (!try_module_get(o->owner))
 				o = NULL;
-			read_unlock_bh(&ife_mod_lock);
+			read_unlock(&ife_mod_lock);
 			return o;
 		}
 	}
-	read_unlock_bh(&ife_mod_lock);
+	read_unlock(&ife_mod_lock);
 
 	return NULL;
 }
@@ -190,12 +190,12 @@ int register_ife_op(struct tcf_meta_ops *mops)
 	    !mops->get || !mops->alloc)
 		return -EINVAL;
 
-	write_lock_bh(&ife_mod_lock);
+	write_lock(&ife_mod_lock);
 
 	list_for_each_entry(m, &ifeoplist, list) {
 		if (m->metaid == mops->metaid ||
 		    (strcmp(mops->name, m->name) == 0)) {
-			write_unlock_bh(&ife_mod_lock);
+			write_unlock(&ife_mod_lock);
 			return -EEXIST;
 		}
 	}
@@ -204,7 +204,7 @@ int register_ife_op(struct tcf_meta_ops *mops)
 		mops->release = ife_release_meta_gen;
 
 	list_add_tail(&mops->list, &ifeoplist);
-	write_unlock_bh(&ife_mod_lock);
+	write_unlock(&ife_mod_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(unregister_ife_op);
@@ -214,7 +214,7 @@ int unregister_ife_op(struct tcf_meta_ops *mops)
 	struct tcf_meta_ops *m;
 	int err = -ENOENT;
 
-	write_lock_bh(&ife_mod_lock);
+	write_lock(&ife_mod_lock);
 	list_for_each_entry(m, &ifeoplist, list) {
 		if (m->metaid == mops->metaid) {
 			list_del(&mops->list);
@@ -222,7 +222,7 @@ int unregister_ife_op(struct tcf_meta_ops *mops)
 			break;
 		}
 	}
-	write_unlock_bh(&ife_mod_lock);
+	write_unlock(&ife_mod_lock);
 
 	return err;
 }
@@ -343,13 +343,13 @@ static int use_all_metadata(struct tcf_ife_info *ife)
 	int rc = 0;
 	int installed = 0;
 
-	read_lock_bh(&ife_mod_lock);
+	read_lock(&ife_mod_lock);
 	list_for_each_entry(o, &ifeoplist, list) {
 		rc = add_metainfo(ife, o->metaid, NULL, 0, true);
 		if (rc == 0)
 			installed += 1;
 	}
-	read_unlock_bh(&ife_mod_lock);
+	read_unlock(&ife_mod_lock);
 
 	if (installed)
 		return 0;
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 8/9] act_ife: move tcfa_lock down to where necessary
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
                   ` (6 preceding siblings ...)
  2018-08-19 19:22 ` [Patch net 7/9] Revert "net: sched: act_ife: disable bh when taking ife_mod_lock" Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-20 18:29   ` David Miller
  2018-08-19 19:22 ` [Patch net 9/9] act_ife: fix a potential deadlock Cong Wang
  2018-08-21 20:00 ` [Patch net 0/9] net_sched: pending clean up and bug fixes David Miller
  9 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang, Vlad Buslov

The only time we need to take tcfa_lock is when adding
a new metainfo to an existing ife->metalist. We don't need
to take tcfa_lock so early and so broadly in tcf_ife_init().

This means we can always take ife_mod_lock first, avoid the
reverse locking ordering warning as reported by Vlad.

Reported-by: Vlad Buslov <vladbu@mellanox.com>
Tested-by: Vlad Buslov <vladbu@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_ife.c | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 9decbb74b3ac..244a8cf48183 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -265,11 +265,8 @@ static const char *ife_meta_id2name(u32 metaid)
 #endif
 
 /* called when adding new meta information
- * under ife->tcf_lock for existing action
 */
-static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
-				void *val, int len, bool exists,
-				bool rtnl_held)
+static int load_metaops_and_vet(u32 metaid, void *val, int len, bool rtnl_held)
 {
 	struct tcf_meta_ops *ops = find_ife_oplist(metaid);
 	int ret = 0;
@@ -277,15 +274,11 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
 	if (!ops) {
 		ret = -ENOENT;
 #ifdef CONFIG_MODULES
-		if (exists)
-			spin_unlock_bh(&ife->tcf_lock);
 		if (rtnl_held)
 			rtnl_unlock();
 		request_module("ife-meta-%s", ife_meta_id2name(metaid));
 		if (rtnl_held)
 			rtnl_lock();
-		if (exists)
-			spin_lock_bh(&ife->tcf_lock);
 		ops = find_ife_oplist(metaid);
 #endif
 	}
@@ -302,10 +295,9 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
 }
 
 /* called when adding new meta information
- * under ife->tcf_lock for existing action
 */
 static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
-			int len, bool atomic)
+			int len, bool atomic, bool exists)
 {
 	struct tcf_meta_info *mi = NULL;
 	struct tcf_meta_ops *ops = find_ife_oplist(metaid);
@@ -332,12 +324,16 @@ static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
 		}
 	}
 
+	if (exists)
+		spin_lock_bh(&ife->tcf_lock);
 	list_add_tail(&mi->metalist, &ife->metalist);
+	if (exists)
+		spin_unlock_bh(&ife->tcf_lock);
 
 	return ret;
 }
 
-static int use_all_metadata(struct tcf_ife_info *ife)
+static int use_all_metadata(struct tcf_ife_info *ife, bool exists)
 {
 	struct tcf_meta_ops *o;
 	int rc = 0;
@@ -345,7 +341,7 @@ static int use_all_metadata(struct tcf_ife_info *ife)
 
 	read_lock(&ife_mod_lock);
 	list_for_each_entry(o, &ifeoplist, list) {
-		rc = add_metainfo(ife, o->metaid, NULL, 0, true);
+		rc = add_metainfo(ife, o->metaid, NULL, 0, true, exists);
 		if (rc == 0)
 			installed += 1;
 	}
@@ -422,7 +418,6 @@ static void tcf_ife_cleanup(struct tc_action *a)
 		kfree_rcu(p, rcu);
 }
 
-/* under ife->tcf_lock for existing action */
 static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
 			     bool exists, bool rtnl_held)
 {
@@ -436,12 +431,11 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
 			val = nla_data(tb[i]);
 			len = nla_len(tb[i]);
 
-			rc = load_metaops_and_vet(ife, i, val, len, exists,
-						  rtnl_held);
+			rc = load_metaops_and_vet(i, val, len, rtnl_held);
 			if (rc != 0)
 				return rc;
 
-			rc = add_metainfo(ife, i, val, len, exists);
+			rc = add_metainfo(ife, i, val, len, false, exists);
 			if (rc)
 				return rc;
 		}
@@ -540,8 +534,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
 		p->eth_type = ife_type;
 	}
 
-	if (exists)
-		spin_lock_bh(&ife->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
 		INIT_LIST_HEAD(&ife->metalist);
@@ -551,10 +543,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
 				       NULL, NULL);
 		if (err) {
 metadata_parse_err:
-			if (exists)
-				spin_unlock_bh(&ife->tcf_lock);
 			tcf_idr_release(*a, bind);
-
 			kfree(p);
 			return err;
 		}
@@ -569,17 +558,16 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
 		 * as we can. You better have at least one else we are
 		 * going to bail out
 		 */
-		err = use_all_metadata(ife);
+		err = use_all_metadata(ife, exists);
 		if (err) {
-			if (exists)
-				spin_unlock_bh(&ife->tcf_lock);
 			tcf_idr_release(*a, bind);
-
 			kfree(p);
 			return err;
 		}
 	}
 
+	if (exists)
+		spin_lock_bh(&ife->tcf_lock);
 	ife->tcf_action = parm->action;
 	/* protected by tcf_lock when modifying existing action */
 	rcu_swap_protected(ife->params, p, 1);
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net 9/9] act_ife: fix a potential deadlock
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
                   ` (7 preceding siblings ...)
  2018-08-19 19:22 ` [Patch net 8/9] act_ife: move tcfa_lock down to where necessary Cong Wang
@ 2018-08-19 19:22 ` Cong Wang
  2018-08-21 20:00 ` [Patch net 0/9] net_sched: pending clean up and bug fixes David Miller
  9 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2018-08-19 19:22 UTC (permalink / raw)
  To: netdev; +Cc: jhs, Cong Wang

use_all_metadata() acquires read_lock(&ife_mod_lock), then calls
add_metainfo() which calls find_ife_oplist() which acquires the same
lock again. Deadlock!

Introduce __add_metainfo() which accepts struct tcf_meta_ops *ops
as an additional parameter and let its callers to decide how
to find it. For use_all_metadata(), it already has ops, no
need to find it again, just call __add_metainfo() directly.

And, as ife_mod_lock is only needed for find_ife_oplist(),
this means we can make non-atomic allocation for populate_metalist()
now.

Fixes: 817e9f2c5c26 ("act_ife: acquire ife_mod_lock before reading ifeoplist")
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_ife.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 244a8cf48183..196430aefe87 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -296,22 +296,16 @@ static int load_metaops_and_vet(u32 metaid, void *val, int len, bool rtnl_held)
 
 /* called when adding new meta information
 */
-static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
-			int len, bool atomic, bool exists)
+static int __add_metainfo(const struct tcf_meta_ops *ops,
+			  struct tcf_ife_info *ife, u32 metaid, void *metaval,
+			  int len, bool atomic, bool exists)
 {
 	struct tcf_meta_info *mi = NULL;
-	struct tcf_meta_ops *ops = find_ife_oplist(metaid);
 	int ret = 0;
 
-	if (!ops)
-		return -ENOENT;
-
 	mi = kzalloc(sizeof(*mi), atomic ? GFP_ATOMIC : GFP_KERNEL);
-	if (!mi) {
-		/*put back what find_ife_oplist took */
-		module_put(ops->owner);
+	if (!mi)
 		return -ENOMEM;
-	}
 
 	mi->metaid = metaid;
 	mi->ops = ops;
@@ -319,7 +313,6 @@ static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
 		ret = ops->alloc(mi, metaval, atomic ? GFP_ATOMIC : GFP_KERNEL);
 		if (ret != 0) {
 			kfree(mi);
-			module_put(ops->owner);
 			return ret;
 		}
 	}
@@ -333,6 +326,21 @@ static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
 	return ret;
 }
 
+static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
+			int len, bool exists)
+{
+	const struct tcf_meta_ops *ops = find_ife_oplist(metaid);
+	int ret;
+
+	if (!ops)
+		return -ENOENT;
+	ret = __add_metainfo(ops, ife, metaid, metaval, len, false, exists);
+	if (ret)
+		/*put back what find_ife_oplist took */
+		module_put(ops->owner);
+	return ret;
+}
+
 static int use_all_metadata(struct tcf_ife_info *ife, bool exists)
 {
 	struct tcf_meta_ops *o;
@@ -341,7 +349,7 @@ static int use_all_metadata(struct tcf_ife_info *ife, bool exists)
 
 	read_lock(&ife_mod_lock);
 	list_for_each_entry(o, &ifeoplist, list) {
-		rc = add_metainfo(ife, o->metaid, NULL, 0, true, exists);
+		rc = __add_metainfo(o, ife, o->metaid, NULL, 0, true, exists);
 		if (rc == 0)
 			installed += 1;
 	}
@@ -435,7 +443,7 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
 			if (rc != 0)
 				return rc;
 
-			rc = add_metainfo(ife, i, val, len, false, exists);
+			rc = add_metainfo(ife, i, val, len, exists);
 			if (rc)
 				return rc;
 		}
-- 
2.14.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Patch net 8/9] act_ife: move tcfa_lock down to where necessary
  2018-08-19 19:22 ` [Patch net 8/9] act_ife: move tcfa_lock down to where necessary Cong Wang
@ 2018-08-20 18:29   ` David Miller
  2018-08-20 23:57     ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: David Miller @ 2018-08-20 18:29 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev, jhs, vladbu

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sun, 19 Aug 2018 12:22:12 -0700

> The only time we need to take tcfa_lock is when adding
> a new metainfo to an existing ife->metalist. We don't need
> to take tcfa_lock so early and so broadly in tcf_ife_init().
> 
> This means we can always take ife_mod_lock first, avoid the
> reverse locking ordering warning as reported by Vlad.
> 
> Reported-by: Vlad Buslov <vladbu@mellanox.com>
> Tested-by: Vlad Buslov <vladbu@mellanox.com>
> Cc: Vlad Buslov <vladbu@mellanox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

After this change we no longer call populate_metalist() in an atomic
context via tcf_ife_init(), and populate_metalist passes 'exists'
down to add_metainfo() as an 'atomic' indicator.  It doesn't have this
meaning if you aren't holding the tcfa_lock in the callers with BH
disabled.

Therefore, add_metainfo()'s 'atomic' indication is inaccurate in this
call chain and will use GFP_ATOMIC unnecessarily.

Probably the thing to just is just pass 'false' down to add_metainfo()
in populate_metalist().

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net 8/9] act_ife: move tcfa_lock down to where necessary
  2018-08-20 18:29   ` David Miller
@ 2018-08-20 23:57     ` Cong Wang
  2018-08-21 19:43       ` David Miller
  0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2018-08-20 23:57 UTC (permalink / raw)
  To: David Miller
  Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Vlad Buslov

On Mon, Aug 20, 2018 at 11:29 AM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Sun, 19 Aug 2018 12:22:12 -0700
>
> > The only time we need to take tcfa_lock is when adding
> > a new metainfo to an existing ife->metalist. We don't need
> > to take tcfa_lock so early and so broadly in tcf_ife_init().
> >
> > This means we can always take ife_mod_lock first, avoid the
> > reverse locking ordering warning as reported by Vlad.
> >
> > Reported-by: Vlad Buslov <vladbu@mellanox.com>
> > Tested-by: Vlad Buslov <vladbu@mellanox.com>
> > Cc: Vlad Buslov <vladbu@mellanox.com>
> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> After this change we no longer call populate_metalist() in an atomic
> context via tcf_ife_init(), and populate_metalist passes 'exists'
> down to add_metainfo() as an 'atomic' indicator.  It doesn't have this
> meaning if you aren't holding the tcfa_lock in the callers with BH
> disabled.

Passing 'exists' as 'atomic' is prior to my change. With my change,
they are separated as two parameters:

 static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
-                       int len, bool atomic)
+                       int len, bool atomic, bool exists)


Or I misunderstand your point here?


>
> Therefore, add_metainfo()'s 'atomic' indication is inaccurate in this
> call chain and will use GFP_ATOMIC unnecessarily.
>
> Probably the thing to just is just pass 'false' down to add_metainfo()
> in populate_metalist().

This is exactly what this patch does?


 static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
                             bool exists, bool rtnl_held)
 {
@@ -436,12 +431,11 @@ static int populate_metalist(struct tcf_ife_info
*ife, struct nlattr **tb,
...
-                       rc = add_metainfo(ife, i, val, len, exists);
+                       rc = add_metainfo(ife, i, val, len, false, exists);

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net 8/9] act_ife: move tcfa_lock down to where necessary
  2018-08-20 23:57     ` Cong Wang
@ 2018-08-21 19:43       ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2018-08-21 19:43 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev, jhs, vladbu

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 20 Aug 2018 16:57:46 -0700

> Passing 'exists' as 'atomic' is prior to my change. With my change,
> they are separated as two parameters:

I mis-read the patch, thanks for explaining :)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net 0/9] net_sched: pending clean up and bug fixes
  2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
                   ` (8 preceding siblings ...)
  2018-08-19 19:22 ` [Patch net 9/9] act_ife: fix a potential deadlock Cong Wang
@ 2018-08-21 20:00 ` David Miller
  9 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2018-08-21 20:00 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev, jhs

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sun, 19 Aug 2018 12:22:04 -0700

> This patchset aims to clean up and fixes some bugs in current
> merge window, this is why it is targeting -net.
> 
> Patch 1-5 are clean up Vlad's patches merged in current merge
> window, patch 6 is just a trivial cleanup.
> 
> Patch 7 reverts a lockdep warning fix and patch 8 provides a better
> fix for it.
> 
> Patch 9 fixes a potential deadlock found by me during code review.
> 
> Please see each patch for details.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Series applied and patches #8 and #9 queued up for -stable.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-08-21 23:21 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-19 19:22 [Patch net 0/9] net_sched: pending clean up and bug fixes Cong Wang
2018-08-19 19:22 ` [Patch net 1/9] net_sched: improve and refactor tcf_action_put_many() Cong Wang
2018-08-19 19:22 ` [Patch net 2/9] net_sched: remove unnecessary ops->delete() Cong Wang
2018-08-19 19:22 ` [Patch net 3/9] net_sched: remove unused parameter for tcf_action_delete() Cong Wang
2018-08-19 19:22 ` [Patch net 4/9] net_sched: remove unused tcf_idr_check() Cong Wang
2018-08-19 19:22 ` [Patch net 5/9] net_sched: remove list_head from tc_action Cong Wang
2018-08-19 19:22 ` [Patch net 6/9] net_sched: remove unused tcfa_capab Cong Wang
2018-08-19 19:22 ` [Patch net 7/9] Revert "net: sched: act_ife: disable bh when taking ife_mod_lock" Cong Wang
2018-08-19 19:22 ` [Patch net 8/9] act_ife: move tcfa_lock down to where necessary Cong Wang
2018-08-20 18:29   ` David Miller
2018-08-20 23:57     ` Cong Wang
2018-08-21 19:43       ` David Miller
2018-08-19 19:22 ` [Patch net 9/9] act_ife: fix a potential deadlock Cong Wang
2018-08-21 20:00 ` [Patch net 0/9] net_sched: pending clean up and bug fixes David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).