[PATCH net-next 0/3] net/sched: transition actions to pcpu stats and rcu

[PATCH net-next 0/3] net/sched: transition actions to pcpu stats and rcu

[Pedro Tammela]

Following the work done for act_pedit[0], transition the remaining tc
actions to percpu stats and rcu, whenever possible.
Percpu stats make updating the action stats very cheap, while combining
it with rcu action parameters makes it possible to get rid of the per
action lock in the datapath.

For act_connmark and act_nat we run the following tests:
- tc filter add dev ens2f0 ingress matchall action connmark
- tc filter add dev ens2f0 ingress matchall action nat ingress any 10.10.10.10

Our setup consists of a 26 cores Intel CPU and a 25G NIC.
We use TRex to shoot 10mpps TCP packets and take perf measurements.
Both actions improved performance as expected since the datapath lock disappeared.

Actions act_gate and act_ctinfo are changed to use the percpu stats.
act_ctinfo already relied on rcu on the datapath but it was updating the stats wrongly,
act_gate datapath has interactions with hrtimers which might require to take the per
action lock (not sure yet, so we only convert to percpu stats).

[0] https://lore.kernel.org/all/20230131145149.3776656-1-pctammela@mojatatu.com/

Pedro Tammela (3):
  net/sched: act_nat: transition to percpu stats and rcu
  net/sched: act_connmark: transition to percpu stats and rcu
  net/sched: act_gate: use percpu stats

 include/net/tc_act/tc_connmark.h |   9 ++-
 include/net/tc_act/tc_nat.h      |  10 ++-
 net/sched/act_connmark.c         | 109 ++++++++++++++++++++-----------
 net/sched/act_gate.c             |  10 +--
 net/sched/act_nat.c              |  68 +++++++++++++------
 5 files changed, 137 insertions(+), 69 deletions(-)

-- 
2.34.1

[PATCH net-next 1/3] net/sched: act_nat: transition to percpu stats and rcu

[Pedro Tammela]

The tc action act_nat was using shared stats and taking the per action
lock in the datapath. Improve it by using percpu stats and rcu.

perf before:
- 10.48% tcf_nat_act
   - 81.83% _raw_spin_lock
        81.08% native_queued_spin_lock_slowpath

perf after:
- 0.48% tcf_nat_act

tdc results:
1..27
ok 1 7565 - Add nat action on ingress with default control action
ok 2 fd79 - Add nat action on ingress with pipe control action
ok 3 eab9 - Add nat action on ingress with continue control action
ok 4 c53a - Add nat action on ingress with reclassify control action
ok 5 76c9 - Add nat action on ingress with jump control action
ok 6 24c6 - Add nat action on ingress with drop control action
ok 7 2120 - Add nat action on ingress with maximum index value
ok 8 3e9d - Add nat action on ingress with invalid index value
ok 9 f6c9 - Add nat action on ingress with invalid IP address
ok 10 be25 - Add nat action on ingress with invalid argument
ok 11 a7bd - Add nat action on ingress with DEFAULT IP address
ok 12 ee1e - Add nat action on ingress with ANY IP address
ok 13 1de8 - Add nat action on ingress with ALL IP address
ok 14 8dba - Add nat action on egress with default control action
ok 15 19a7 - Add nat action on egress with pipe control action
ok 16 f1d9 - Add nat action on egress with continue control action
ok 17 6d4a - Add nat action on egress with reclassify control action
ok 18 b313 - Add nat action on egress with jump control action
ok 19 d9fc - Add nat action on egress with drop control action
ok 20 a895 - Add nat action on egress with DEFAULT IP address
ok 21 2572 - Add nat action on egress with ANY IP address
ok 22 37f3 - Add nat action on egress with ALL IP address
ok 23 6054 - Add nat action on egress with cookie
ok 24 79d6 - Add nat action on ingress with cookie
ok 25 4b12 - Replace nat action with invalid goto chain control
ok 26 b811 - Delete nat action with valid index
ok 27 a521 - Delete nat action with invalid index

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/net/tc_act/tc_nat.h | 10 ++++--
 net/sched/act_nat.c         | 68 ++++++++++++++++++++++++++-----------
 2 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/include/net/tc_act/tc_nat.h b/include/net/tc_act/tc_nat.h
index c14407160..c869274ac 100644
--- a/include/net/tc_act/tc_nat.h
+++ b/include/net/tc_act/tc_nat.h
@@ -5,13 +5,17 @@
 #include <linux/types.h>
 #include <net/act_api.h>
 
-struct tcf_nat {
-	struct tc_action common;
-
+struct tcf_nat_parms {
 	__be32 old_addr;
 	__be32 new_addr;
 	__be32 mask;
 	u32 flags;
+	struct rcu_head rcu;
+};
+
+struct tcf_nat {
+	struct tc_action common;
+	struct tcf_nat_parms __rcu *parms;
 };
 
 #define to_tcf_nat(a) ((struct tcf_nat *)a)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 74c74be33..fb986d97c 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -40,6 +40,7 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	bool bind = flags & TCA_ACT_FLAGS_BIND;
 	struct nlattr *tb[TCA_NAT_MAX + 1];
 	struct tcf_chain *goto_ch = NULL;
+	struct tcf_nat_parms *nparm, *oparm;
 	struct tc_nat *parm;
 	int ret = 0, err;
 	struct tcf_nat *p;
@@ -59,8 +60,8 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	index = parm->index;
 	err = tcf_idr_check_alloc(tn, &index, a, bind);
 	if (!err) {
-		ret = tcf_idr_create(tn, index, est, a,
-				     &act_nat_ops, bind, false, flags);
+		ret = tcf_idr_create_from_flags(tn, index, est, a, &act_nat_ops,
+						bind, flags);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
 			return ret;
@@ -79,19 +80,31 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
 	if (err < 0)
 		goto release_idr;
+
+	nparm = kzalloc(sizeof(*nparm), GFP_KERNEL);
+	if (!nparm) {
+		err = -ENOMEM;
+		goto release_idr;
+	}
+
+	nparm->old_addr = parm->old_addr;
+	nparm->new_addr = parm->new_addr;
+	nparm->mask = parm->mask;
+	nparm->flags = parm->flags;
+
 	p = to_tcf_nat(*a);
 
 	spin_lock_bh(&p->tcf_lock);
-	p->old_addr = parm->old_addr;
-	p->new_addr = parm->new_addr;
-	p->mask = parm->mask;
-	p->flags = parm->flags;
-
 	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);
+
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
+	if (oparm)
+		kfree_rcu(oparm, rcu);
+
 	return ret;
 release_idr:
 	tcf_idr_release(*a, bind);
@@ -103,6 +116,7 @@ TC_INDIRECT_SCOPE int tcf_nat_act(struct sk_buff *skb,
 				  struct tcf_result *res)
 {
 	struct tcf_nat *p = to_tcf_nat(a);
+	struct tcf_nat_parms *parms;
 	struct iphdr *iph;
 	__be32 old_addr;
 	__be32 new_addr;
@@ -113,18 +127,16 @@ TC_INDIRECT_SCOPE int tcf_nat_act(struct sk_buff *skb,
 	int ihl;
 	int noff;
 
-	spin_lock(&p->tcf_lock);
-
 	tcf_lastuse_update(&p->tcf_tm);
-	old_addr = p->old_addr;
-	new_addr = p->new_addr;
-	mask = p->mask;
-	egress = p->flags & TCA_NAT_FLAG_EGRESS;
-	action = p->tcf_action;
+	tcf_action_update_bstats(&p->common, skb);
 
-	bstats_update(&p->tcf_bstats, skb);
+	action = READ_ONCE(p->tcf_action);
 
-	spin_unlock(&p->tcf_lock);
+	parms = rcu_dereference_bh(p->parms);
+	old_addr = parms->old_addr;
+	new_addr = parms->new_addr;
+	mask = parms->mask;
+	egress = parms->flags & TCA_NAT_FLAG_EGRESS;
 
 	if (unlikely(action == TC_ACT_SHOT))
 		goto drop;
@@ -264,15 +276,20 @@ static int tcf_nat_dump(struct sk_buff *skb, struct tc_action *a,
 		.refcnt   = refcount_read(&p->tcf_refcnt) - ref,
 		.bindcnt  = atomic_read(&p->tcf_bindcnt) - bind,
 	};
+	struct tcf_nat_parms *parms;
 	struct tcf_t t;
 
 	spin_lock_bh(&p->tcf_lock);
-	opt.old_addr = p->old_addr;
-	opt.new_addr = p->new_addr;
-	opt.mask = p->mask;
-	opt.flags = p->flags;
+
 	opt.action = p->tcf_action;
 
+	parms = rcu_dereference_protected(p->parms, lockdep_is_held(&p->tcf_lock));
+
+	opt.old_addr = parms->old_addr;
+	opt.new_addr = parms->new_addr;
+	opt.mask = parms->mask;
+	opt.flags = parms->flags;
+
 	if (nla_put(skb, TCA_NAT_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
 
@@ -289,6 +306,16 @@ static int tcf_nat_dump(struct sk_buff *skb, struct tc_action *a,
 	return -1;
 }
 
+static void tcf_nat_cleanup(struct tc_action *a)
+{
+	struct tcf_nat_parms *parms;
+	struct tcf_nat *p = to_tcf_nat(a);
+
+	parms = rcu_dereference_protected(p->parms, 1);
+	if (parms)
+		kfree_rcu(parms, rcu);
+}
+
 static struct tc_action_ops act_nat_ops = {
 	.kind		=	"nat",
 	.id		=	TCA_ID_NAT,
@@ -296,6 +323,7 @@ static struct tc_action_ops act_nat_ops = {
 	.act		=	tcf_nat_act,
 	.dump		=	tcf_nat_dump,
 	.init		=	tcf_nat_init,
+	.cleanup	=	tcf_nat_cleanup,
 	.size		=	sizeof(struct tcf_nat),
 };
 
-- 
2.34.1

Re: [PATCH net-next 1/3] net/sched: act_nat: transition to percpu stats and rcu

[Paolo Abeni]

On Fri, 2023-02-10 at 17:27 -0300, Pedro Tammela wrote:
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index 74c74be33..fb986d97c 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -40,6 +40,7 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>  	bool bind = flags & TCA_ACT_FLAGS_BIND;
>  	struct nlattr *tb[TCA_NAT_MAX + 1];
>  	struct tcf_chain *goto_ch = NULL;
> +	struct tcf_nat_parms *nparm, *oparm;
>  	struct tc_nat *parm;
>  	int ret = 0, err;
>  	struct tcf_nat *p;

Please respect the reverse x-mas tree above.

> @@ -289,6 +306,16 @@ static int tcf_nat_dump(struct sk_buff *skb, struct tc_action *a,
>  	return -1;
>  }
>  
> +static void tcf_nat_cleanup(struct tc_action *a)
> +{
> +	struct tcf_nat_parms *parms;
> +	struct tcf_nat *p = to_tcf_nat(a);

Same here.

Thanks,

Paolo

[PATCH net-next 2/3] net/sched: act_connmark: transition to percpu stats and rcu

[Pedro Tammela]

The tc action act_connmark was using shared stats and taking the per
action lock in the datapath. Improve it by using percpu stats and rcu.

perf before:
- 13.55% tcf_connmark_act
   - 81.18% _raw_spin_lock
       80.46% native_queued_spin_lock_slowpath

perf after:
- 3.12% tcf_connmark_act

tdc results:
1..15
ok 1 2002 - Add valid connmark action with defaults
ok 2 56a5 - Add valid connmark action with control pass
ok 3 7c66 - Add valid connmark action with control drop
ok 4 a913 - Add valid connmark action with control pipe
ok 5 bdd8 - Add valid connmark action with control reclassify
ok 6 b8be - Add valid connmark action with control continue
ok 7 d8a6 - Add valid connmark action with control jump
ok 8 aae8 - Add valid connmark action with zone argument
ok 9 2f0b - Add valid connmark action with invalid zone argument
ok 10 9305 - Add connmark action with unsupported argument
ok 11 71ca - Add valid connmark action and replace it
ok 12 5f8f - Add valid connmark action with cookie
ok 13 c506 - Replace connmark with invalid goto chain control
ok 14 6571 - Delete connmark action with valid index
ok 15 3426 - Delete connmark action with invalid index

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/net/tc_act/tc_connmark.h |   9 ++-
 net/sched/act_connmark.c         | 109 ++++++++++++++++++++-----------
 2 files changed, 77 insertions(+), 41 deletions(-)

diff --git a/include/net/tc_act/tc_connmark.h b/include/net/tc_act/tc_connmark.h
index 1f4cb477b..e8dd77a96 100644
--- a/include/net/tc_act/tc_connmark.h
+++ b/include/net/tc_act/tc_connmark.h
@@ -4,10 +4,15 @@
 
 #include <net/act_api.h>
 
-struct tcf_connmark_info {
-	struct tc_action common;
+struct tcf_connmark_parms {
 	struct net *net;
 	u16 zone;
+	struct rcu_head rcu;
+};
+
+struct tcf_connmark_info {
+	struct tc_action common;
+	struct tcf_connmark_parms __rcu *parms;
 };
 
 #define to_connmark(a) ((struct tcf_connmark_info *)a)
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 7e63ff7e3..541e1c556 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -36,13 +36,15 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
 	struct nf_conntrack_tuple tuple;
 	enum ip_conntrack_info ctinfo;
 	struct tcf_connmark_info *ca = to_connmark(a);
+	struct tcf_connmark_parms *parms;
 	struct nf_conntrack_zone zone;
 	struct nf_conn *c;
 	int proto;
 
-	spin_lock(&ca->tcf_lock);
 	tcf_lastuse_update(&ca->tcf_tm);
-	bstats_update(&ca->tcf_bstats, skb);
+	tcf_action_update_bstats(&ca->common, skb);
+
+	parms = rcu_dereference_bh(ca->parms);
 
 	switch (skb_protocol(skb, true)) {
 	case htons(ETH_P_IP):
@@ -64,31 +66,31 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
 	c = nf_ct_get(skb, &ctinfo);
 	if (c) {
 		skb->mark = READ_ONCE(c->mark);
-		/* using overlimits stats to count how many packets marked */
-		ca->tcf_qstats.overlimits++;
-		goto out;
+		goto count;
 	}
 
-	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
-			       proto, ca->net, &tuple))
+	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, parms->net,
+			       &tuple))
 		goto out;
 
-	zone.id = ca->zone;
+	zone.id = parms->zone;
 	zone.dir = NF_CT_DEFAULT_ZONE_DIR;
 
-	thash = nf_conntrack_find_get(ca->net, &zone, &tuple);
+	thash = nf_conntrack_find_get(parms->net, &zone, &tuple);
 	if (!thash)
 		goto out;
 
 	c = nf_ct_tuplehash_to_ctrack(thash);
-	/* using overlimits stats to count how many packets marked */
-	ca->tcf_qstats.overlimits++;
 	skb->mark = READ_ONCE(c->mark);
 	nf_ct_put(c);
 
-out:
+count:
+	/* using overlimits stats to count how many packets marked */
+	spin_lock(&ca->tcf_lock);
+	ca->tcf_qstats.overlimits++;
 	spin_unlock(&ca->tcf_lock);
-	return ca->tcf_action;
+out:
+	return READ_ONCE(ca->tcf_action);
 }
 
 static const struct nla_policy connmark_policy[TCA_CONNMARK_MAX + 1] = {
@@ -104,6 +106,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_CONNMARK_MAX + 1];
 	bool bind = flags & TCA_ACT_FLAGS_BIND;
 	struct tcf_chain *goto_ch = NULL;
+	struct tcf_connmark_parms *nparms, *oparms;
 	struct tcf_connmark_info *ci;
 	struct tc_connmark *parm;
 	int ret = 0, err;
@@ -120,52 +123,66 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
 	if (!tb[TCA_CONNMARK_PARMS])
 		return -EINVAL;
 
+	nparms = kzalloc(sizeof(*nparms), GFP_KERNEL);
+	if (!nparms)
+		return -ENOMEM;
+
 	parm = nla_data(tb[TCA_CONNMARK_PARMS]);
 	index = parm->index;
 	ret = tcf_idr_check_alloc(tn, &index, a, bind);
 	if (!ret) {
-		ret = tcf_idr_create(tn, index, est, a,
-				     &act_connmark_ops, bind, false, flags);
+		ret = tcf_idr_create_from_flags(tn, index, est, a,
+						&act_connmark_ops, bind, flags);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
-			return ret;
+			err = ret;
+			goto out_free;
 		}
 
 		ci = to_connmark(*a);
-		err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch,
-					       extack);
-		if (err < 0)
-			goto release_idr;
-		tcf_action_set_ctrlact(*a, parm->action, goto_ch);
-		ci->net = net;
-		ci->zone = parm->zone;
+
+		nparms->net = net;
+		nparms->zone = parm->zone;
 
 		ret = ACT_P_CREATED;
 	} else if (ret > 0) {
 		ci = to_connmark(*a);
-		if (bind)
-			return 0;
-		if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
-			tcf_idr_release(*a, bind);
-			return -EEXIST;
+		if (bind) {
+			err = 0;
+			goto out_free;
 		}
-		err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch,
-					       extack);
-		if (err < 0)
+		if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
+			err = -EEXIST;
 			goto release_idr;
-		/* replacing action and zone */
-		spin_lock_bh(&ci->tcf_lock);
-		goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
-		ci->zone = parm->zone;
-		spin_unlock_bh(&ci->tcf_lock);
-		if (goto_ch)
-			tcf_chain_put_by_act(goto_ch);
+		}
+
+		nparms->net = rtnl_dereference(ci->parms)->net;
+		nparms->zone = parm->zone;
+
 		ret = 0;
 	}
 
+	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
+	if (err < 0)
+		goto release_idr;
+
+	spin_lock_bh(&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);
+
+	if (goto_ch)
+		tcf_chain_put_by_act(goto_ch);
+
+	if (oparms)
+		kfree_rcu(oparms, rcu);
+
 	return ret;
+
 release_idr:
 	tcf_idr_release(*a, bind);
+out_free:
+	kfree(nparms);
 	return err;
 }
 
@@ -179,11 +196,14 @@ static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_action *a,
 		.refcnt  = refcount_read(&ci->tcf_refcnt) - ref,
 		.bindcnt = atomic_read(&ci->tcf_bindcnt) - bind,
 	};
+	struct tcf_connmark_parms *parms;
 	struct tcf_t t;
 
 	spin_lock_bh(&ci->tcf_lock);
+	parms = rcu_dereference_protected(ci->parms, lockdep_is_held(&ci->tcf_lock));
+
 	opt.action = ci->tcf_action;
-	opt.zone = ci->zone;
+	opt.zone = parms->zone;
 	if (nla_put(skb, TCA_CONNMARK_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
 
@@ -201,6 +221,16 @@ static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_action *a,
 	return -1;
 }
 
+static void tcf_connmark_cleanup(struct tc_action *a)
+{
+	struct tcf_connmark_parms *parms;
+	struct tcf_connmark_info *ci = to_connmark(a);
+
+	parms = rcu_dereference_protected(ci->parms, 1);
+	if (parms)
+		kfree_rcu(parms, rcu);
+}
+
 static struct tc_action_ops act_connmark_ops = {
 	.kind		=	"connmark",
 	.id		=	TCA_ID_CONNMARK,
@@ -208,6 +238,7 @@ static struct tc_action_ops act_connmark_ops = {
 	.act		=	tcf_connmark_act,
 	.dump		=	tcf_connmark_dump,
 	.init		=	tcf_connmark_init,
+	.cleanup	=	tcf_connmark_cleanup,
 	.size		=	sizeof(struct tcf_connmark_info),
 };
 
-- 
2.34.1

Re: [PATCH net-next 2/3] net/sched: act_connmark: transition to percpu stats and rcu

[Paolo Abeni]

On Fri, 2023-02-10 at 17:27 -0300, Pedro Tammela wrote:
> The tc action act_connmark was using shared stats and taking the per
> action lock in the datapath. Improve it by using percpu stats and rcu.
> 
> perf before:
> - 13.55% tcf_connmark_act
>    - 81.18% _raw_spin_lock
>        80.46% native_queued_spin_lock_slowpath
> 
> perf after:
> - 3.12% tcf_connmark_act
> 
> tdc results:
> 1..15
> ok 1 2002 - Add valid connmark action with defaults
> ok 2 56a5 - Add valid connmark action with control pass
> ok 3 7c66 - Add valid connmark action with control drop
> ok 4 a913 - Add valid connmark action with control pipe
> ok 5 bdd8 - Add valid connmark action with control reclassify
> ok 6 b8be - Add valid connmark action with control continue
> ok 7 d8a6 - Add valid connmark action with control jump
> ok 8 aae8 - Add valid connmark action with zone argument
> ok 9 2f0b - Add valid connmark action with invalid zone argument
> ok 10 9305 - Add connmark action with unsupported argument
> ok 11 71ca - Add valid connmark action and replace it
> ok 12 5f8f - Add valid connmark action with cookie
> ok 13 c506 - Replace connmark with invalid goto chain control
> ok 14 6571 - Delete connmark action with valid index
> ok 15 3426 - Delete connmark action with invalid index
> 
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> ---
>  include/net/tc_act/tc_connmark.h |   9 ++-
>  net/sched/act_connmark.c         | 109 ++++++++++++++++++++-----------
>  2 files changed, 77 insertions(+), 41 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_connmark.h b/include/net/tc_act/tc_connmark.h
> index 1f4cb477b..e8dd77a96 100644
> --- a/include/net/tc_act/tc_connmark.h
> +++ b/include/net/tc_act/tc_connmark.h
> @@ -4,10 +4,15 @@
>  
>  #include <net/act_api.h>
>  
> -struct tcf_connmark_info {
> -	struct tc_action common;
> +struct tcf_connmark_parms {
>  	struct net *net;
>  	u16 zone;
> +	struct rcu_head rcu;
> +};
> +
> +struct tcf_connmark_info {
> +	struct tc_action common;
> +	struct tcf_connmark_parms __rcu *parms;
>  };
>  
>  #define to_connmark(a) ((struct tcf_connmark_info *)a)
> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> index 7e63ff7e3..541e1c556 100644
> --- a/net/sched/act_connmark.c
> +++ b/net/sched/act_connmark.c
> @@ -36,13 +36,15 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
>  	struct nf_conntrack_tuple tuple;
>  	enum ip_conntrack_info ctinfo;
>  	struct tcf_connmark_info *ca = to_connmark(a);
> +	struct tcf_connmark_parms *parms;
>  	struct nf_conntrack_zone zone;
>  	struct nf_conn *c;
>  	int proto;
>  
> -	spin_lock(&ca->tcf_lock);
>  	tcf_lastuse_update(&ca->tcf_tm);
> -	bstats_update(&ca->tcf_bstats, skb);
> +	tcf_action_update_bstats(&ca->common, skb);
> +
> +	parms = rcu_dereference_bh(ca->parms);
>  
>  	switch (skb_protocol(skb, true)) {
>  	case htons(ETH_P_IP):
> @@ -64,31 +66,31 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
>  	c = nf_ct_get(skb, &ctinfo);
>  	if (c) {
>  		skb->mark = READ_ONCE(c->mark);
> -		/* using overlimits stats to count how many packets marked */
> -		ca->tcf_qstats.overlimits++;
> -		goto out;
> +		goto count;
>  	}
>  
> -	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
> -			       proto, ca->net, &tuple))
> +	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, parms->net,
> +			       &tuple))
>  		goto out;
>  
> -	zone.id = ca->zone;
> +	zone.id = parms->zone;
>  	zone.dir = NF_CT_DEFAULT_ZONE_DIR;
>  
> -	thash = nf_conntrack_find_get(ca->net, &zone, &tuple);
> +	thash = nf_conntrack_find_get(parms->net, &zone, &tuple);
>  	if (!thash)
>  		goto out;
>  
>  	c = nf_ct_tuplehash_to_ctrack(thash);
> -	/* using overlimits stats to count how many packets marked */
> -	ca->tcf_qstats.overlimits++;
>  	skb->mark = READ_ONCE(c->mark);
>  	nf_ct_put(c);
>  
> -out:
> +count:
> +	/* using overlimits stats to count how many packets marked */
> +	spin_lock(&ca->tcf_lock);
> +	ca->tcf_qstats.overlimits++;
>  	spin_unlock(&ca->tcf_lock);

I think above you could use tcf_action_inc_overlimit_qstats() and avoid
acquiring the spin lock in most cases.

Side note: it looks like pedit could use a similar change, too - sorry
for missing that point before.

> -	return ca->tcf_action;
> +out:
> +	return READ_ONCE(ca->tcf_action);
>  }
>  
>  static const struct nla_policy connmark_policy[TCA_CONNMARK_MAX + 1] = {
> @@ -104,6 +106,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
>  	struct nlattr *tb[TCA_CONNMARK_MAX + 1];
>  	bool bind = flags & TCA_ACT_FLAGS_BIND;
>  	struct tcf_chain *goto_ch = NULL;
> +	struct tcf_connmark_parms *nparms, *oparms;
>  	struct tcf_connmark_info *ci;
>  	struct tc_connmark *parm;
>  	int ret = 0, err;

Please respect the reverse x-mas tree above.


Thanks,

Paolo

Re: [PATCH net-next 2/3] net/sched: act_connmark: transition to percpu stats and rcu

[Pedro Tammela]

On 14/02/2023 06:03, Paolo Abeni wrote:
> On Fri, 2023-02-10 at 17:27 -0300, Pedro Tammela wrote:
>> The tc action act_connmark was using shared stats and taking the per
>> action lock in the datapath. Improve it by using percpu stats and rcu.
>>
>> perf before:
>> - 13.55% tcf_connmark_act
>>     - 81.18% _raw_spin_lock
>>         80.46% native_queued_spin_lock_slowpath
>>
>> perf after:
>> - 3.12% tcf_connmark_act
>>
>> tdc results:
>> 1..15
>> ok 1 2002 - Add valid connmark action with defaults
>> ok 2 56a5 - Add valid connmark action with control pass
>> ok 3 7c66 - Add valid connmark action with control drop
>> ok 4 a913 - Add valid connmark action with control pipe
>> ok 5 bdd8 - Add valid connmark action with control reclassify
>> ok 6 b8be - Add valid connmark action with control continue
>> ok 7 d8a6 - Add valid connmark action with control jump
>> ok 8 aae8 - Add valid connmark action with zone argument
>> ok 9 2f0b - Add valid connmark action with invalid zone argument
>> ok 10 9305 - Add connmark action with unsupported argument
>> ok 11 71ca - Add valid connmark action and replace it
>> ok 12 5f8f - Add valid connmark action with cookie
>> ok 13 c506 - Replace connmark with invalid goto chain control
>> ok 14 6571 - Delete connmark action with valid index
>> ok 15 3426 - Delete connmark action with invalid index
>>
>> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
>> ---
>>   include/net/tc_act/tc_connmark.h |   9 ++-
>>   net/sched/act_connmark.c         | 109 ++++++++++++++++++++-----------
>>   2 files changed, 77 insertions(+), 41 deletions(-)
>>
>> diff --git a/include/net/tc_act/tc_connmark.h b/include/net/tc_act/tc_connmark.h
>> index 1f4cb477b..e8dd77a96 100644
>> --- a/include/net/tc_act/tc_connmark.h
>> +++ b/include/net/tc_act/tc_connmark.h
>> @@ -4,10 +4,15 @@
>>   
>>   #include <net/act_api.h>
>>   
>> -struct tcf_connmark_info {
>> -	struct tc_action common;
>> +struct tcf_connmark_parms {
>>   	struct net *net;
>>   	u16 zone;
>> +	struct rcu_head rcu;
>> +};
>> +
>> +struct tcf_connmark_info {
>> +	struct tc_action common;
>> +	struct tcf_connmark_parms __rcu *parms;
>>   };
>>   
>>   #define to_connmark(a) ((struct tcf_connmark_info *)a)
>> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
>> index 7e63ff7e3..541e1c556 100644
>> --- a/net/sched/act_connmark.c
>> +++ b/net/sched/act_connmark.c
>> @@ -36,13 +36,15 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
>>   	struct nf_conntrack_tuple tuple;
>>   	enum ip_conntrack_info ctinfo;
>>   	struct tcf_connmark_info *ca = to_connmark(a);
>> +	struct tcf_connmark_parms *parms;
>>   	struct nf_conntrack_zone zone;
>>   	struct nf_conn *c;
>>   	int proto;
>>   
>> -	spin_lock(&ca->tcf_lock);
>>   	tcf_lastuse_update(&ca->tcf_tm);
>> -	bstats_update(&ca->tcf_bstats, skb);
>> +	tcf_action_update_bstats(&ca->common, skb);
>> +
>> +	parms = rcu_dereference_bh(ca->parms);
>>   
>>   	switch (skb_protocol(skb, true)) {
>>   	case htons(ETH_P_IP):
>> @@ -64,31 +66,31 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
>>   	c = nf_ct_get(skb, &ctinfo);
>>   	if (c) {
>>   		skb->mark = READ_ONCE(c->mark);
>> -		/* using overlimits stats to count how many packets marked */
>> -		ca->tcf_qstats.overlimits++;
>> -		goto out;
>> +		goto count;
>>   	}
>>   
>> -	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
>> -			       proto, ca->net, &tuple))
>> +	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, parms->net,
>> +			       &tuple))
>>   		goto out;
>>   
>> -	zone.id = ca->zone;
>> +	zone.id = parms->zone;
>>   	zone.dir = NF_CT_DEFAULT_ZONE_DIR;
>>   
>> -	thash = nf_conntrack_find_get(ca->net, &zone, &tuple);
>> +	thash = nf_conntrack_find_get(parms->net, &zone, &tuple);
>>   	if (!thash)
>>   		goto out;
>>   
>>   	c = nf_ct_tuplehash_to_ctrack(thash);
>> -	/* using overlimits stats to count how many packets marked */
>> -	ca->tcf_qstats.overlimits++;
>>   	skb->mark = READ_ONCE(c->mark);
>>   	nf_ct_put(c);
>>   
>> -out:
>> +count:
>> +	/* using overlimits stats to count how many packets marked */
>> +	spin_lock(&ca->tcf_lock);
>> +	ca->tcf_qstats.overlimits++;
>>   	spin_unlock(&ca->tcf_lock);
> 
> I think above you could use tcf_action_inc_overlimit_qstats() and avoid
> acquiring the spin lock in most cases.
> 
> Side note: it looks like pedit could use a similar change, too - sorry
> for missing that point before.
> 

Oh, indeed! I will respin and add the pedit change in this series as well.
Many thanks.

[PATCH net-next 3/3] net/sched: act_gate: use percpu stats

[Pedro Tammela]

The tc action act_gate was using shared stats, move it to percpu stats.

tdc results:
1..12
ok 1 5153 - Add gate action with priority and sched-entry
ok 2 7189 - Add gate action with base-time
ok 3 a721 - Add gate action with cycle-time
ok 4 c029 - Add gate action with cycle-time-ext
ok 5 3719 - Replace gate base-time action
ok 6 d821 - Delete gate action with valid index
ok 7 3128 - Delete gate action with invalid index
ok 8 7837 - List gate actions
ok 9 9273 - Flush gate actions
ok 10 c829 - Add gate action with duplicate index
ok 11 3043 - Add gate action with invalid index
ok 12 2930 - Add gate action with cookie

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_gate.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 9b8def0be..684b7a79f 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -120,10 +120,10 @@ TC_INDIRECT_SCOPE int tcf_gate_act(struct sk_buff *skb,
 {
 	struct tcf_gate *gact = to_gate(a);
 
-	spin_lock(&gact->tcf_lock);
-
 	tcf_lastuse_update(&gact->tcf_tm);
-	bstats_update(&gact->tcf_bstats, skb);
+	tcf_action_update_bstats(&gact->common, skb);
+
+	spin_lock(&gact->tcf_lock);
 
 	if (unlikely(gact->current_gate_status & GATE_ACT_PENDING)) {
 		spin_unlock(&gact->tcf_lock);
@@ -357,8 +357,8 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 		return 0;
 
 	if (!err) {
-		ret = tcf_idr_create(tn, index, est, a,
-				     &act_gate_ops, bind, false, flags);
+		ret = tcf_idr_create_from_flags(tn, index, est, a,
+						&act_gate_ops, bind, flags);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
 			return ret;
-- 
2.34.1

Re: [PATCH net-next 3/3] net/sched: act_gate: use percpu stats

[Paolo Abeni]

On Fri, 2023-02-10 at 17:27 -0300, Pedro Tammela wrote:
> 
> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index 9b8def0be..684b7a79f 100644
> --- a/net/sched/act_gate.c
> +++ b/net/sched/act_gate.c
> @@ -120,10 +120,10 @@ TC_INDIRECT_SCOPE int tcf_gate_act(struct sk_buff *skb,
>  {
>  	struct tcf_gate *gact = to_gate(a);
>  
> -	spin_lock(&gact->tcf_lock);
> -
>  	tcf_lastuse_update(&gact->tcf_tm);
> -	bstats_update(&gact->tcf_bstats, skb);
> +	tcf_action_update_bstats(&gact->common, skb);
> +
> +	spin_lock(&gact->tcf_lock);

I think that RCU-ifying the 'current_gate_status' field, setting its
value with  WRITE_ONCE (both in _init and in gate_timer_func()) and
finally accessing it here with READ_ONCE, you could move pretty much
everything except the code touching *_octets update outside the
spinlock.

I'm not sure how much that will be relevant - e.g. how frequently we
expect to hit the 

	if (gact->current_max_octets >= 0) {

code path.

For sure the current code allocates per_cpu stats, but they are never
used.

Cheers,

Paolo