[PATCH net-next 0/4] net_sched: extend RCU use in dump() methods (II)

[PATCH net-next 0/4] net_sched: extend RCU use in dump() methods (II)

[Eric Dumazet]

Second series adding RCU dump() to three actions

First patch removes BH blocking on modules done in the first series.

Eric Dumazet (4):
  net_sched: remove BH blocking in eight actions
  net_sched: act_vlan: use RCU in tcf_vlan_dump()
  net_sched: act_tunnel_key: use RCU in tunnel_key_dump()
  net_sched: act_skbmod: use RCU in tcf_skbmod_dump()

 include/net/tc_act/tc_skbmod.h     |  1 +
 include/net/tc_act/tc_tunnel_key.h |  1 +
 include/net/tc_act/tc_vlan.h       |  1 +
 net/sched/act_connmark.c           |  4 ++--
 net/sched/act_csum.c               |  4 ++--
 net/sched/act_ct.c                 |  4 ++--
 net/sched/act_ctinfo.c             |  4 ++--
 net/sched/act_mpls.c               |  4 ++--
 net/sched/act_nat.c                |  4 ++--
 net/sched/act_pedit.c              |  4 ++--
 net/sched/act_skbedit.c            |  4 ++--
 net/sched/act_skbmod.c             | 26 ++++++++++++--------------
 net/sched/act_tunnel_key.c         | 20 +++++++++-----------
 net/sched/act_vlan.c               | 20 +++++++++-----------
 14 files changed, 49 insertions(+), 52 deletions(-)

-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Eric Dumazet]

Followup of f45b45cbfae3 ("Merge branch
'net_sched-act-extend-rcu-use-in-dump-methods'")

We never grab tcf_lock from BH context in these modules:

 act_connmark
 act_csum
 act_ct
 act_ctinfo
 act_mpls
 act_nat
 act_pedit
 act_skbedit

No longer block BH when acquiring tcf_lock from init functions.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/act_connmark.c | 4 ++--
 net/sched/act_csum.c     | 4 ++--
 net/sched/act_ct.c       | 4 ++--
 net/sched/act_ctinfo.c   | 4 ++--
 net/sched/act_mpls.c     | 4 ++--
 net/sched/act_nat.c      | 4 ++--
 net/sched/act_pedit.c    | 4 ++--
 net/sched/act_skbedit.c  | 4 ++--
 8 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 3e89927d711647d75f31c8d80a3ddd102e3d2e36..bf2d6b6da04223e1acaa7e9f5d29d426db06d705 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -169,10 +169,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
 
 	nparms->action = parm->action;
 
-	spin_lock_bh(&ci->tcf_lock);
+	spin_lock(&ci->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	oparms = rcu_replace_pointer(ci->parms, nparms, lockdep_is_held(&ci->tcf_lock));
-	spin_unlock_bh(&ci->tcf_lock);
+	spin_unlock(&ci->tcf_lock);
 
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 0939e6b2ba4d1947df0f3dcfc09bfaa339a6ace2..8bad91753615a08d78d99086fd6a7ab6e4c8e857 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -101,11 +101,11 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
 	params_new->update_flags = parm->update_flags;
 	params_new->action = parm->action;
 
-	spin_lock_bh(&p->tcf_lock);
+	spin_lock(&p->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	params_new = rcu_replace_pointer(p->params, params_new,
 					 lockdep_is_held(&p->tcf_lock));
-	spin_unlock_bh(&p->tcf_lock);
+	spin_unlock(&p->tcf_lock);
 
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 6749a4a9a9cd0a43897fcd20d228721ce057cb88..6d2355e73b0f55750679b48e562e148d2cd8b7d4 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1410,11 +1410,11 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
 		goto cleanup;
 
 	params->action = parm->action;
-	spin_lock_bh(&c->tcf_lock);
+	spin_lock(&c->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	params = rcu_replace_pointer(c->params, params,
 				     lockdep_is_held(&c->tcf_lock));
-	spin_unlock_bh(&c->tcf_lock);
+	spin_unlock(&c->tcf_lock);
 
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 71efe04d00b5c6195e43f1ea6dab1548f6f97293..6f79eed9a544a49aed35ac0557250a3d5a9fc576 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -258,11 +258,11 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 
 	cp_new->action = actparm->action;
 
-	spin_lock_bh(&ci->tcf_lock);
+	spin_lock(&ci->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
 	cp_new = rcu_replace_pointer(ci->params, cp_new,
 				     lockdep_is_held(&ci->tcf_lock));
-	spin_unlock_bh(&ci->tcf_lock);
+	spin_unlock(&ci->tcf_lock);
 
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index 6654011dcd2ba30769b2f52078373a834e259f88..ed7bdaa23f0d80caef6bd5cd5b9787e24ff2d1af 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -296,10 +296,10 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
 					     htons(ETH_P_MPLS_UC));
 	p->action = parm->action;
 
-	spin_lock_bh(&m->tcf_lock);
+	spin_lock(&m->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	p = rcu_replace_pointer(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
-	spin_unlock_bh(&m->tcf_lock);
+	spin_unlock(&m->tcf_lock);
 
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 26241d80ebe03e74a92e951fb5ae065493b93277..9cc2a1772cf8290a4be7d6e694e2ceccd48c386a 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -95,10 +95,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 
 	p = to_tcf_nat(*a);
 
-	spin_lock_bh(&p->tcf_lock);
+	spin_lock(&p->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	oparm = rcu_replace_pointer(p->parms, nparm, lockdep_is_held(&p->tcf_lock));
-	spin_unlock_bh(&p->tcf_lock);
+	spin_unlock(&p->tcf_lock);
 
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 4b65901397a88864014f74c53d0fa00b40ac6613..8fc8f577cb7a8362fee60cd79cce263edca32a7a 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -280,10 +280,10 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 
 	p = to_pedit(*a);
 	nparms->action = parm->action;
-	spin_lock_bh(&p->tcf_lock);
+	spin_lock(&p->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	oparms = rcu_replace_pointer(p->parms, nparms, 1);
-	spin_unlock_bh(&p->tcf_lock);
+	spin_unlock(&p->tcf_lock);
 
 	if (oparms)
 		call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 8c1d1554f6575d3b0feae4d26ef4865d44a63e59..aa6b1744de21c1dca223cb87a919dc3e29841b83 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -261,11 +261,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		params_new->mask = *mask;
 
 	params_new->action = parm->action;
-	spin_lock_bh(&d->tcf_lock);
+	spin_lock(&d->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	params_new = rcu_replace_pointer(d->params, params_new,
 					 lockdep_is_held(&d->tcf_lock));
-	spin_unlock_bh(&d->tcf_lock);
+	spin_unlock(&d->tcf_lock);
 	if (params_new)
 		kfree_rcu(params_new, rcu);
 	if (goto_ch)
-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH net-next 2/4] net_sched: act_vlan: use RCU in tcf_vlan_dump()

[Eric Dumazet]

Also storing tcf_action into struct tcf_vlan_params
makes sure there is no discrepancy in tcf_vlan_act().

No longer block BH in tcf_vlan_init() when acquiring tcf_lock.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/tc_act/tc_vlan.h |  1 +
 net/sched/act_vlan.c         | 20 +++++++++-----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
index 3f5e9242b5e83d082b8a633b3702feadd5672b47..beadee41669a22d7519adaf348fb602cac7d273f 100644
--- a/include/net/tc_act/tc_vlan.h
+++ b/include/net/tc_act/tc_vlan.h
@@ -10,6 +10,7 @@
 #include <linux/tc_act/tc_vlan.h>
 
 struct tcf_vlan_params {
+	int		  action;
 	int               tcfv_action;
 	unsigned char     tcfv_push_dst[ETH_ALEN];
 	unsigned char     tcfv_push_src[ETH_ALEN];
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 383bf18b6862f9a7a96087f1ed786fb869b70415..b46f980f3b2ae0bc50f1a37442945d281b7abddd 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -25,7 +25,6 @@ TC_INDIRECT_SCOPE int tcf_vlan_act(struct sk_buff *skb,
 {
 	struct tcf_vlan *v = to_vlan(a);
 	struct tcf_vlan_params *p;
-	int action;
 	int err;
 	u16 tci;
 
@@ -38,8 +37,6 @@ TC_INDIRECT_SCOPE int tcf_vlan_act(struct sk_buff *skb,
 	if (skb_at_tc_ingress(skb))
 		skb_push_rcsum(skb, skb->mac_len);
 
-	action = READ_ONCE(v->tcf_action);
-
 	p = rcu_dereference_bh(v->vlan_p);
 
 	switch (p->tcfv_action) {
@@ -97,7 +94,7 @@ TC_INDIRECT_SCOPE int tcf_vlan_act(struct sk_buff *skb,
 		skb_pull_rcsum(skb, skb->mac_len);
 
 	skb_reset_mac_len(skb);
-	return action;
+	return p->action;
 
 drop:
 	tcf_action_inc_drop_qstats(&v->common);
@@ -255,10 +252,11 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
 			   ETH_ALEN);
 	}
 
-	spin_lock_bh(&v->tcf_lock);
+	p->action = parm->action;
+	spin_lock(&v->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	p = rcu_replace_pointer(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
-	spin_unlock_bh(&v->tcf_lock);
+	spin_unlock(&v->tcf_lock);
 
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
@@ -297,9 +295,9 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
 	};
 	struct tcf_t t;
 
-	spin_lock_bh(&v->tcf_lock);
-	opt.action = v->tcf_action;
-	p = rcu_dereference_protected(v->vlan_p, lockdep_is_held(&v->tcf_lock));
+	rcu_read_lock();
+	p = rcu_dereference(v->vlan_p);
+	opt.action = p->action;
 	opt.v_action = p->tcfv_action;
 	if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
@@ -325,12 +323,12 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
 	tcf_tm_dump(&t, &v->tcf_tm);
 	if (nla_put_64bit(skb, TCA_VLAN_TM, sizeof(t), &t, TCA_VLAN_PAD))
 		goto nla_put_failure;
-	spin_unlock_bh(&v->tcf_lock);
+	rcu_read_unlock();
 
 	return skb->len;
 
 nla_put_failure:
-	spin_unlock_bh(&v->tcf_lock);
+	rcu_read_unlock();
 	nlmsg_trim(skb, b);
 	return -1;
 }
-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH net-next 3/4] net_sched: act_tunnel_key: use RCU in tunnel_key_dump()

[Eric Dumazet]

Also storing tcf_action into struct tcf_tunnel_key_params
makes sure there is no discrepancy in tunnel_key_act().

No longer block BH in tunnel_key_init() when acquiring tcf_lock.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/tc_act/tc_tunnel_key.h |  1 +
 net/sched/act_tunnel_key.c         | 20 +++++++++-----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
index 879fe8cff581951c759076b159d3ee16f9db34c1..0f1925f97520209b3a9f110d3c7e9bb3c5ef2968 100644
--- a/include/net/tc_act/tc_tunnel_key.h
+++ b/include/net/tc_act/tc_tunnel_key.h
@@ -14,6 +14,7 @@
 struct tcf_tunnel_key_params {
 	struct rcu_head		rcu;
 	int			tcft_action;
+	int			action;
 	struct metadata_dst     *tcft_enc_metadata;
 };
 
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 2cef4b08befbecf498ea836d45abdef1ee4e06b5..e1c8b48c217c339d5e806d031ba287df2a57b1aa 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -29,13 +29,11 @@ TC_INDIRECT_SCOPE int tunnel_key_act(struct sk_buff *skb,
 {
 	struct tcf_tunnel_key *t = to_tunnel_key(a);
 	struct tcf_tunnel_key_params *params;
-	int action;
 
 	params = rcu_dereference_bh(t->params);
 
 	tcf_lastuse_update(&t->tcf_tm);
 	tcf_action_update_bstats(&t->common, skb);
-	action = READ_ONCE(t->tcf_action);
 
 	switch (params->tcft_action) {
 	case TCA_TUNNEL_KEY_ACT_RELEASE:
@@ -51,7 +49,7 @@ TC_INDIRECT_SCOPE int tunnel_key_act(struct sk_buff *skb,
 		break;
 	}
 
-	return action;
+	return params->action;
 }
 
 static const struct nla_policy
@@ -532,11 +530,12 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 	params_new->tcft_action = parm->t_action;
 	params_new->tcft_enc_metadata = metadata;
 
-	spin_lock_bh(&t->tcf_lock);
+	params_new->action = parm->action;
+	spin_lock(&t->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	params_new = rcu_replace_pointer(t->params, params_new,
 					 lockdep_is_held(&t->tcf_lock));
-	spin_unlock_bh(&t->tcf_lock);
+	spin_unlock(&t->tcf_lock);
 	tunnel_key_release_params(params_new);
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
@@ -726,10 +725,9 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
 	};
 	struct tcf_t tm;
 
-	spin_lock_bh(&t->tcf_lock);
-	params = rcu_dereference_protected(t->params,
-					   lockdep_is_held(&t->tcf_lock));
-	opt.action   = t->tcf_action;
+	rcu_read_lock();
+	params = rcu_dereference(t->params);
+	opt.action   = params->action;
 	opt.t_action = params->tcft_action;
 
 	if (nla_put(skb, TCA_TUNNEL_KEY_PARMS, sizeof(opt), &opt))
@@ -766,12 +764,12 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
 	if (nla_put_64bit(skb, TCA_TUNNEL_KEY_TM, sizeof(tm),
 			  &tm, TCA_TUNNEL_KEY_PAD))
 		goto nla_put_failure;
-	spin_unlock_bh(&t->tcf_lock);
+	rcu_read_unlock();
 
 	return skb->len;
 
 nla_put_failure:
-	spin_unlock_bh(&t->tcf_lock);
+	rcu_read_unlock();
 	nlmsg_trim(skb, b);
 	return -1;
 }
-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH net-next 4/4] net_sched: act_skbmod: use RCU in tcf_skbmod_dump()

[Eric Dumazet]

Also storing tcf_action into struct tcf_skbmod_params
makes sure there is no discrepancy in tcf_skbmod_act().

No longer block BH in tcf_skbmod_init() when acquiring tcf_lock.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/tc_act/tc_skbmod.h |  1 +
 net/sched/act_skbmod.c         | 26 ++++++++++++--------------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/include/net/tc_act/tc_skbmod.h b/include/net/tc_act/tc_skbmod.h
index 7c240d2fed4e3cdf686016588cd78eb52b80765b..626704cd6241b37f20539f9dd1270275ba19e578 100644
--- a/include/net/tc_act/tc_skbmod.h
+++ b/include/net/tc_act/tc_skbmod.h
@@ -12,6 +12,7 @@
 struct tcf_skbmod_params {
 	struct rcu_head	rcu;
 	u64	flags; /*up to 64 types of operations; extend if needed */
+	int	action;
 	u8	eth_dst[ETH_ALEN];
 	u16	eth_type;
 	u8	eth_src[ETH_ALEN];
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index dc022969346188c17a43f3ef40f3c203272954c4..fce625eafcb2b793cba7bebb740b136bf8498aa1 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -27,19 +27,18 @@ TC_INDIRECT_SCOPE int tcf_skbmod_act(struct sk_buff *skb,
 				     struct tcf_result *res)
 {
 	struct tcf_skbmod *d = to_skbmod(a);
-	int action, max_edit_len, err;
 	struct tcf_skbmod_params *p;
+	int max_edit_len, err;
 	u64 flags;
 
 	tcf_lastuse_update(&d->tcf_tm);
 	bstats_update(this_cpu_ptr(d->common.cpu_bstats), skb);
 
-	action = READ_ONCE(d->tcf_action);
-	if (unlikely(action == TC_ACT_SHOT))
+	p = rcu_dereference_bh(d->skbmod_p);
+	if (unlikely(p->action == TC_ACT_SHOT))
 		goto drop;
 
 	max_edit_len = skb_mac_header_len(skb);
-	p = rcu_dereference_bh(d->skbmod_p);
 	flags = p->flags;
 
 	/* tcf_skbmod_init() guarantees "flags" to be one of the following:
@@ -85,7 +84,7 @@ TC_INDIRECT_SCOPE int tcf_skbmod_act(struct sk_buff *skb,
 		INET_ECN_set_ce(skb);
 
 out:
-	return action;
+	return p->action;
 
 drop:
 	qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
@@ -193,9 +192,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
 	}
 
 	p->flags = lflags;
-
+	p->action = parm->action;
 	if (ovr)
-		spin_lock_bh(&d->tcf_lock);
+		spin_lock(&d->tcf_lock);
 	/* Protected by tcf_lock if overwriting existing action. */
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
 	p_old = rcu_dereference_protected(d->skbmod_p, 1);
@@ -209,7 +208,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
 
 	rcu_assign_pointer(d->skbmod_p, p);
 	if (ovr)
-		spin_unlock_bh(&d->tcf_lock);
+		spin_unlock(&d->tcf_lock);
 
 	if (p_old)
 		kfree_rcu(p_old, rcu);
@@ -248,10 +247,9 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
 	opt.index   = d->tcf_index;
 	opt.refcnt  = refcount_read(&d->tcf_refcnt) - ref;
 	opt.bindcnt = atomic_read(&d->tcf_bindcnt) - bind;
-	spin_lock_bh(&d->tcf_lock);
-	opt.action = d->tcf_action;
-	p = rcu_dereference_protected(d->skbmod_p,
-				      lockdep_is_held(&d->tcf_lock));
+	rcu_read_lock();
+	p = rcu_dereference(d->skbmod_p);
+	opt.action = p->action;
 	opt.flags  = p->flags;
 	if (nla_put(skb, TCA_SKBMOD_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
@@ -269,10 +267,10 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
 	if (nla_put_64bit(skb, TCA_SKBMOD_TM, sizeof(t), &t, TCA_SKBMOD_PAD))
 		goto nla_put_failure;
 
-	spin_unlock_bh(&d->tcf_lock);
+	rcu_read_unlock();
 	return skb->len;
 nla_put_failure:
-	spin_unlock_bh(&d->tcf_lock);
+	rcu_read_unlock();
 	nlmsg_trim(skb, b);
 	return -1;
 }
-- 
2.51.0.261.g7ce5a0a67e-goog

Re: [PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Simon Horman]

On Wed, Aug 27, 2025 at 12:53:46PM +0000, Eric Dumazet wrote:
> Followup of f45b45cbfae3 ("Merge branch
> 'net_sched-act-extend-rcu-use-in-dump-methods'")
> 
> We never grab tcf_lock from BH context in these modules:
> 
>  act_connmark
>  act_csum
>  act_ct
>  act_ctinfo
>  act_mpls
>  act_nat
>  act_pedit
>  act_skbedit
> 
> No longer block BH when acquiring tcf_lock from init functions.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 2/4] net_sched: act_vlan: use RCU in tcf_vlan_dump()

[Simon Horman]

On Wed, Aug 27, 2025 at 12:53:47PM +0000, Eric Dumazet wrote:
> Also storing tcf_action into struct tcf_vlan_params
> makes sure there is no discrepancy in tcf_vlan_act().
> 
> No longer block BH in tcf_vlan_init() when acquiring tcf_lock.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 3/4] net_sched: act_tunnel_key: use RCU in tunnel_key_dump()

[Simon Horman]

On Wed, Aug 27, 2025 at 12:53:48PM +0000, Eric Dumazet wrote:
> Also storing tcf_action into struct tcf_tunnel_key_params
> makes sure there is no discrepancy in tunnel_key_act().
> 
> No longer block BH in tunnel_key_init() when acquiring tcf_lock.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 4/4] net_sched: act_skbmod: use RCU in tcf_skbmod_dump()

[Simon Horman]

On Wed, Aug 27, 2025 at 12:53:49PM +0000, Eric Dumazet wrote:
> Also storing tcf_action into struct tcf_skbmod_params
> makes sure there is no discrepancy in tcf_skbmod_act().
> 
> No longer block BH in tcf_skbmod_init() when acquiring tcf_lock.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 0/4] net_sched: extend RCU use in dump() methods (II)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 27 Aug 2025 12:53:45 +0000 you wrote:
> Second series adding RCU dump() to three actions
> 
> First patch removes BH blocking on modules done in the first series.
> 
> Eric Dumazet (4):
>   net_sched: remove BH blocking in eight actions
>   net_sched: act_vlan: use RCU in tcf_vlan_dump()
>   net_sched: act_tunnel_key: use RCU in tunnel_key_dump()
>   net_sched: act_skbmod: use RCU in tcf_skbmod_dump()
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] net_sched: remove BH blocking in eight actions
    https://git.kernel.org/netdev/net-next/c/3133d5c15cb5
  - [net-next,2/4] net_sched: act_vlan: use RCU in tcf_vlan_dump()
    https://git.kernel.org/netdev/net-next/c/48b5e5dbdb23
  - [net-next,3/4] net_sched: act_tunnel_key: use RCU in tunnel_key_dump()
    https://git.kernel.org/netdev/net-next/c/e97ae742972f
  - [net-next,4/4] net_sched: act_skbmod: use RCU in tcf_skbmod_dump()
    https://git.kernel.org/netdev/net-next/c/53df77e78590

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Jamal Hadi Salim]

On Wed, Aug 27, 2025 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
>
> Followup of f45b45cbfae3 ("Merge branch
> 'net_sched-act-extend-rcu-use-in-dump-methods'")
>
> We never grab tcf_lock from BH context in these modules:
>
>  act_connmark
>  act_csum
>  act_ct
>  act_ctinfo
>  act_mpls
>  act_nat
>  act_pedit
>  act_skbedit
>
> No longer block BH when acquiring tcf_lock from init functions.
>

Brief glance: isnt  the lock still held in BH context for some actions
like pedit and nat (albeit in corner cases)? Both actions call
tcf_action_update_bstats in their act callbacks.
i.e if the action instance was not created with percpu stats,
tcf_action_update_bstats will grab the lock.

cheers,
jamal

> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/sched/act_connmark.c | 4 ++--
>  net/sched/act_csum.c     | 4 ++--
>  net/sched/act_ct.c       | 4 ++--
>  net/sched/act_ctinfo.c   | 4 ++--
>  net/sched/act_mpls.c     | 4 ++--
>  net/sched/act_nat.c      | 4 ++--
>  net/sched/act_pedit.c    | 4 ++--
>  net/sched/act_skbedit.c  | 4 ++--
>  8 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> index 3e89927d711647d75f31c8d80a3ddd102e3d2e36..bf2d6b6da04223e1acaa7e9f5d29d426db06d705 100644
> --- a/net/sched/act_connmark.c
> +++ b/net/sched/act_connmark.c
> @@ -169,10 +169,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
>
>         nparms->action = parm->action;
>
> -       spin_lock_bh(&ci->tcf_lock);
> +       spin_lock(&ci->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>         oparms = rcu_replace_pointer(ci->parms, nparms, lockdep_is_held(&ci->tcf_lock));
> -       spin_unlock_bh(&ci->tcf_lock);
> +       spin_unlock(&ci->tcf_lock);
>
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index 0939e6b2ba4d1947df0f3dcfc09bfaa339a6ace2..8bad91753615a08d78d99086fd6a7ab6e4c8e857 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -101,11 +101,11 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
>         params_new->update_flags = parm->update_flags;
>         params_new->action = parm->action;
>
> -       spin_lock_bh(&p->tcf_lock);
> +       spin_lock(&p->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>         params_new = rcu_replace_pointer(p->params, params_new,
>                                          lockdep_is_held(&p->tcf_lock));
> -       spin_unlock_bh(&p->tcf_lock);
> +       spin_unlock(&p->tcf_lock);
>
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 6749a4a9a9cd0a43897fcd20d228721ce057cb88..6d2355e73b0f55750679b48e562e148d2cd8b7d4 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1410,11 +1410,11 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
>                 goto cleanup;
>
>         params->action = parm->action;
> -       spin_lock_bh(&c->tcf_lock);
> +       spin_lock(&c->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>         params = rcu_replace_pointer(c->params, params,
>                                      lockdep_is_held(&c->tcf_lock));
> -       spin_unlock_bh(&c->tcf_lock);
> +       spin_unlock(&c->tcf_lock);
>
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> index 71efe04d00b5c6195e43f1ea6dab1548f6f97293..6f79eed9a544a49aed35ac0557250a3d5a9fc576 100644
> --- a/net/sched/act_ctinfo.c
> +++ b/net/sched/act_ctinfo.c
> @@ -258,11 +258,11 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>
>         cp_new->action = actparm->action;
>
> -       spin_lock_bh(&ci->tcf_lock);
> +       spin_lock(&ci->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
>         cp_new = rcu_replace_pointer(ci->params, cp_new,
>                                      lockdep_is_held(&ci->tcf_lock));
> -       spin_unlock_bh(&ci->tcf_lock);
> +       spin_unlock(&ci->tcf_lock);
>
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> index 6654011dcd2ba30769b2f52078373a834e259f88..ed7bdaa23f0d80caef6bd5cd5b9787e24ff2d1af 100644
> --- a/net/sched/act_mpls.c
> +++ b/net/sched/act_mpls.c
> @@ -296,10 +296,10 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
>                                              htons(ETH_P_MPLS_UC));
>         p->action = parm->action;
>
> -       spin_lock_bh(&m->tcf_lock);
> +       spin_lock(&m->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>         p = rcu_replace_pointer(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
> -       spin_unlock_bh(&m->tcf_lock);
> +       spin_unlock(&m->tcf_lock);
>
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index 26241d80ebe03e74a92e951fb5ae065493b93277..9cc2a1772cf8290a4be7d6e694e2ceccd48c386a 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -95,10 +95,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>
>         p = to_tcf_nat(*a);
>
> -       spin_lock_bh(&p->tcf_lock);
> +       spin_lock(&p->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>         oparm = rcu_replace_pointer(p->parms, nparm, lockdep_is_held(&p->tcf_lock));
> -       spin_unlock_bh(&p->tcf_lock);
> +       spin_unlock(&p->tcf_lock);
>
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 4b65901397a88864014f74c53d0fa00b40ac6613..8fc8f577cb7a8362fee60cd79cce263edca32a7a 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -280,10 +280,10 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>
>         p = to_pedit(*a);
>         nparms->action = parm->action;
> -       spin_lock_bh(&p->tcf_lock);
> +       spin_lock(&p->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>         oparms = rcu_replace_pointer(p->parms, nparms, 1);
> -       spin_unlock_bh(&p->tcf_lock);
> +       spin_unlock(&p->tcf_lock);
>
>         if (oparms)
>                 call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 8c1d1554f6575d3b0feae4d26ef4865d44a63e59..aa6b1744de21c1dca223cb87a919dc3e29841b83 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -261,11 +261,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>                 params_new->mask = *mask;
>
>         params_new->action = parm->action;
> -       spin_lock_bh(&d->tcf_lock);
> +       spin_lock(&d->tcf_lock);
>         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>         params_new = rcu_replace_pointer(d->params, params_new,
>                                          lockdep_is_held(&d->tcf_lock));
> -       spin_unlock_bh(&d->tcf_lock);
> +       spin_unlock(&d->tcf_lock);
>         if (params_new)
>                 kfree_rcu(params_new, rcu);
>         if (goto_ch)
> --
> 2.51.0.261.g7ce5a0a67e-goog
>

Re: [PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Jamal Hadi Salim]

On Thu, Aug 28, 2025 at 11:26 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Wed, Aug 27, 2025 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > Followup of f45b45cbfae3 ("Merge branch
> > 'net_sched-act-extend-rcu-use-in-dump-methods'")
> >
> > We never grab tcf_lock from BH context in these modules:
> >
> >  act_connmark
> >  act_csum
> >  act_ct
> >  act_ctinfo
> >  act_mpls
> >  act_nat
> >  act_pedit
> >  act_skbedit
> >
> > No longer block BH when acquiring tcf_lock from init functions.
> >
>
> Brief glance: isnt  the lock still held in BH context for some actions
> like pedit and nat (albeit in corner cases)? Both actions call
> tcf_action_update_bstats in their act callbacks.
> i.e if the action instance was not created with percpu stats,
> tcf_action_update_bstats will grab the lock.
>

Testing with lockdep should illustrate this..

cheers,
jamal
> cheers,
> jamal
>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  net/sched/act_connmark.c | 4 ++--
> >  net/sched/act_csum.c     | 4 ++--
> >  net/sched/act_ct.c       | 4 ++--
> >  net/sched/act_ctinfo.c   | 4 ++--
> >  net/sched/act_mpls.c     | 4 ++--
> >  net/sched/act_nat.c      | 4 ++--
> >  net/sched/act_pedit.c    | 4 ++--
> >  net/sched/act_skbedit.c  | 4 ++--
> >  8 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> > index 3e89927d711647d75f31c8d80a3ddd102e3d2e36..bf2d6b6da04223e1acaa7e9f5d29d426db06d705 100644
> > --- a/net/sched/act_connmark.c
> > +++ b/net/sched/act_connmark.c
> > @@ -169,10 +169,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
> >
> >         nparms->action = parm->action;
> >
> > -       spin_lock_bh(&ci->tcf_lock);
> > +       spin_lock(&ci->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> >         oparms = rcu_replace_pointer(ci->parms, nparms, lockdep_is_held(&ci->tcf_lock));
> > -       spin_unlock_bh(&ci->tcf_lock);
> > +       spin_unlock(&ci->tcf_lock);
> >
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> > index 0939e6b2ba4d1947df0f3dcfc09bfaa339a6ace2..8bad91753615a08d78d99086fd6a7ab6e4c8e857 100644
> > --- a/net/sched/act_csum.c
> > +++ b/net/sched/act_csum.c
> > @@ -101,11 +101,11 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
> >         params_new->update_flags = parm->update_flags;
> >         params_new->action = parm->action;
> >
> > -       spin_lock_bh(&p->tcf_lock);
> > +       spin_lock(&p->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> >         params_new = rcu_replace_pointer(p->params, params_new,
> >                                          lockdep_is_held(&p->tcf_lock));
> > -       spin_unlock_bh(&p->tcf_lock);
> > +       spin_unlock(&p->tcf_lock);
> >
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > index 6749a4a9a9cd0a43897fcd20d228721ce057cb88..6d2355e73b0f55750679b48e562e148d2cd8b7d4 100644
> > --- a/net/sched/act_ct.c
> > +++ b/net/sched/act_ct.c
> > @@ -1410,11 +1410,11 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
> >                 goto cleanup;
> >
> >         params->action = parm->action;
> > -       spin_lock_bh(&c->tcf_lock);
> > +       spin_lock(&c->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> >         params = rcu_replace_pointer(c->params, params,
> >                                      lockdep_is_held(&c->tcf_lock));
> > -       spin_unlock_bh(&c->tcf_lock);
> > +       spin_unlock(&c->tcf_lock);
> >
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> > index 71efe04d00b5c6195e43f1ea6dab1548f6f97293..6f79eed9a544a49aed35ac0557250a3d5a9fc576 100644
> > --- a/net/sched/act_ctinfo.c
> > +++ b/net/sched/act_ctinfo.c
> > @@ -258,11 +258,11 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> >
> >         cp_new->action = actparm->action;
> >
> > -       spin_lock_bh(&ci->tcf_lock);
> > +       spin_lock(&ci->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
> >         cp_new = rcu_replace_pointer(ci->params, cp_new,
> >                                      lockdep_is_held(&ci->tcf_lock));
> > -       spin_unlock_bh(&ci->tcf_lock);
> > +       spin_unlock(&ci->tcf_lock);
> >
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> > index 6654011dcd2ba30769b2f52078373a834e259f88..ed7bdaa23f0d80caef6bd5cd5b9787e24ff2d1af 100644
> > --- a/net/sched/act_mpls.c
> > +++ b/net/sched/act_mpls.c
> > @@ -296,10 +296,10 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
> >                                              htons(ETH_P_MPLS_UC));
> >         p->action = parm->action;
> >
> > -       spin_lock_bh(&m->tcf_lock);
> > +       spin_lock(&m->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> >         p = rcu_replace_pointer(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
> > -       spin_unlock_bh(&m->tcf_lock);
> > +       spin_unlock(&m->tcf_lock);
> >
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> > index 26241d80ebe03e74a92e951fb5ae065493b93277..9cc2a1772cf8290a4be7d6e694e2ceccd48c386a 100644
> > --- a/net/sched/act_nat.c
> > +++ b/net/sched/act_nat.c
> > @@ -95,10 +95,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
> >
> >         p = to_tcf_nat(*a);
> >
> > -       spin_lock_bh(&p->tcf_lock);
> > +       spin_lock(&p->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> >         oparm = rcu_replace_pointer(p->parms, nparm, lockdep_is_held(&p->tcf_lock));
> > -       spin_unlock_bh(&p->tcf_lock);
> > +       spin_unlock(&p->tcf_lock);
> >
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > index 4b65901397a88864014f74c53d0fa00b40ac6613..8fc8f577cb7a8362fee60cd79cce263edca32a7a 100644
> > --- a/net/sched/act_pedit.c
> > +++ b/net/sched/act_pedit.c
> > @@ -280,10 +280,10 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> >
> >         p = to_pedit(*a);
> >         nparms->action = parm->action;
> > -       spin_lock_bh(&p->tcf_lock);
> > +       spin_lock(&p->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> >         oparms = rcu_replace_pointer(p->parms, nparms, 1);
> > -       spin_unlock_bh(&p->tcf_lock);
> > +       spin_unlock(&p->tcf_lock);
> >
> >         if (oparms)
> >                 call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);
> > diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> > index 8c1d1554f6575d3b0feae4d26ef4865d44a63e59..aa6b1744de21c1dca223cb87a919dc3e29841b83 100644
> > --- a/net/sched/act_skbedit.c
> > +++ b/net/sched/act_skbedit.c
> > @@ -261,11 +261,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> >                 params_new->mask = *mask;
> >
> >         params_new->action = parm->action;
> > -       spin_lock_bh(&d->tcf_lock);
> > +       spin_lock(&d->tcf_lock);
> >         goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> >         params_new = rcu_replace_pointer(d->params, params_new,
> >                                          lockdep_is_held(&d->tcf_lock));
> > -       spin_unlock_bh(&d->tcf_lock);
> > +       spin_unlock(&d->tcf_lock);
> >         if (params_new)
> >                 kfree_rcu(params_new, rcu);
> >         if (goto_ch)
> > --
> > 2.51.0.261.g7ce5a0a67e-goog
> >

Re: [PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Eric Dumazet]

On Thu, Aug 28, 2025 at 8:29 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Thu, Aug 28, 2025 at 11:26 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Wed, Aug 27, 2025 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > Followup of f45b45cbfae3 ("Merge branch
> > > 'net_sched-act-extend-rcu-use-in-dump-methods'")
> > >
> > > We never grab tcf_lock from BH context in these modules:
> > >
> > >  act_connmark
> > >  act_csum
> > >  act_ct
> > >  act_ctinfo
> > >  act_mpls
> > >  act_nat
> > >  act_pedit
> > >  act_skbedit
> > >
> > > No longer block BH when acquiring tcf_lock from init functions.
> > >
> >
> > Brief glance: isnt  the lock still held in BH context for some actions
> > like pedit and nat (albeit in corner cases)? Both actions call
> > tcf_action_update_bstats in their act callbacks.
> > i.e if the action instance was not created with percpu stats,
> > tcf_action_update_bstats will grab the lock.
> >
>
> Testing with lockdep should illustrate this..

Thanks, I will take a look shortly !

Re: [PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Eric Dumazet]

On Fri, Aug 29, 2025 at 12:19 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Aug 28, 2025 at 8:29 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Thu, Aug 28, 2025 at 11:26 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Wed, Aug 27, 2025 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > Followup of f45b45cbfae3 ("Merge branch
> > > > 'net_sched-act-extend-rcu-use-in-dump-methods'")
> > > >
> > > > We never grab tcf_lock from BH context in these modules:
> > > >
> > > >  act_connmark
> > > >  act_csum
> > > >  act_ct
> > > >  act_ctinfo
> > > >  act_mpls
> > > >  act_nat
> > > >  act_pedit
> > > >  act_skbedit
> > > >
> > > > No longer block BH when acquiring tcf_lock from init functions.
> > > >
> > >
> > > Brief glance: isnt  the lock still held in BH context for some actions
> > > like pedit and nat (albeit in corner cases)? Both actions call
> > > tcf_action_update_bstats in their act callbacks.
> > > i.e if the action instance was not created with percpu stats,
> > > tcf_action_update_bstats will grab the lock.
> > >
> >
> > Testing with lockdep should illustrate this..
>
> Thanks, I will take a look shortly !

I guess I missed this because the lock has two names (tcfa_lock and tcf_lock)

Also, it is unclear why a spinlock is taken for updating stats
as dumps do not seem to acquire this lock.

This could be using atomic_inc() and atomic_add()...

Re: [PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Jamal Hadi Salim]

On Fri, Aug 29, 2025 at 12:03 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Aug 29, 2025 at 12:19 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Aug 28, 2025 at 8:29 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Thu, Aug 28, 2025 at 11:26 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > >
> > > > On Wed, Aug 27, 2025 at 8:53 AM Eric Dumazet <edumazet@google.com> wrote:
> > > > >
> > > > > Followup of f45b45cbfae3 ("Merge branch
> > > > > 'net_sched-act-extend-rcu-use-in-dump-methods'")
> > > > >
> > > > > We never grab tcf_lock from BH context in these modules:
> > > > >
> > > > >  act_connmark
> > > > >  act_csum
> > > > >  act_ct
> > > > >  act_ctinfo
> > > > >  act_mpls
> > > > >  act_nat
> > > > >  act_pedit
> > > > >  act_skbedit
> > > > >
> > > > > No longer block BH when acquiring tcf_lock from init functions.
> > > > >
> > > >
> > > > Brief glance: isnt  the lock still held in BH context for some actions
> > > > like pedit and nat (albeit in corner cases)? Both actions call
> > > > tcf_action_update_bstats in their act callbacks.
> > > > i.e if the action instance was not created with percpu stats,
> > > > tcf_action_update_bstats will grab the lock.
> > > >
> > >
> > > Testing with lockdep should illustrate this..
> >
> > Thanks, I will take a look shortly !
>
> I guess I missed this because the lock has two names (tcfa_lock and tcf_lock)
>
> Also, it is unclear why a spinlock is taken for updating stats
> as dumps do not seem to acquire this lock.
>

action stats dump does start in tcf_action_copy_stats which will grab
the lock (in either gnet_stats_start_copy_compat or
gnet_stats_start_copy) and releases when it terminates in
gnet_stats_finish_copy.

> This could be using atomic_inc() and atomic_add()...

Doable - could be involved...

cheers,
jamal

Re: [PATCH net-next 1/4] net_sched: remove BH blocking in eight actions

[Eric Dumazet]

On Sun, Aug 31, 2025 at 9:45 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> action stats dump does start in tcf_action_copy_stats which will grab
> the lock (in either gnet_stats_start_copy_compat or
> gnet_stats_start_copy) and releases when it terminates in
> gnet_stats_finish_copy.
>

Right ! I am back to work and will send a fix ASAP, thanks !