All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.