Skip to contentSkip to conversation

net/sched: remove spinlock from 'csum' action

PATCH In review Davide Caratti 3 participants 5 messages Jan 22, 2018 – Jan 24, 2018

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Davide Caratti

author
#0 lore raw

v3 cover net/sched: remove spinlock from 'csum' action

Similarly to what has been done earlier with other actions [1][2], this series tries to improve the performance of 'csum' tc action, removing a spinlock in the data path. Patch 1 lets act_csum use per-CPU counters; patch 2 removes spin_{,un}lock_bh() calls from the act() method.

test procedure (using pktgen from https://github.com/netoptimizer):

 # ip link add name eth1 type dummy
 # ip link set dev eth1 up
 # tc qdisc add dev eth1 root handle 1: prio
 # for a in pass drop; do
Quoted 6 lines

tc filter del dev eth1 parent 1: pref 10 matchall action csum udp tc filter add dev eth1 parent 1: pref 10 matchall action csum udp $a for n in 2 4; do
./pktgen_bench_xmit_mode_queue_xmit.sh -v -s 64 -t $n -n 1000000 -i eth1 done
done

test results:

      |    |  before patch   |   after patch
  $a  | $n | avg. pps/thread | avg. pps/thread
 -----+----+-----------------+----------------
 pass |  2 |    1671463 ± 4% |    1920789 ± 3%
 pass |  4 |     648797 ± 1% |     738190 ± 1%
 drop |  2 |    3212692 ± 2% |    3719811 ± 2%
 drop |  4 |    1078824 ± 1% |    1328099 ± 1%

references:

[1] https://www.spinics.net/lists/netdev/msg334760.html
[2] https://www.spinics.net/lists/netdev/msg465862.html

v3 changes:
- use rtnl_dereference() in place of rcu_dereference() in tcf_csum_dump()

v2 changes:
- add 'drop' test, it produces more contentions
- use RCU-protected struct to store 'action' and 'update_flags', to avoid reading the values from subsequent configurations

Davide Caratti (2):
net/sched: act_csum: use per-core statistics net/sched: act_csum: don't use spinlock in the fast path

 include/net/tc_act/tc_csum.h | 16 +++++++++--
 net/sched/act_csum.c         | 66 ++++++++++++++++++++++++++++++++------------
 2 files changed, 62 insertions(+), 20 deletions(-)
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH v3 1/2 net/sched: act_csum: use per-core statistics

use per-CPU counters, like other TC actions do, instead of maintaining one set of stats across all cores. This allows updating act_csum stats without the need of protecting them using spin_{,un}lock_bh() invocations.

Signed-off-by Davide Caratti <[email protected]>
---
 net/sched/act_csum.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
net/sched/act_csum.c +4 -6
index af4b8ec60d9a..df22da365cd9 100644
@@ -67,7 +67,7 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
 
 	if (!tcf_idr_check(tn, parm->index, a, bind)) {
 		ret = tcf_idr_create(tn, parm->index, est, a,
-				     &act_csum_ops, bind, false);
+				     &act_csum_ops, bind, true);
 		if (ret)
 			return ret;
 		ret = ACT_P_CREATED;

@@ -542,9 +542,9 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
 	int action;
 	u32 update_flags;
 
-	spin_lock(&p->tcf_lock);
 	tcf_lastuse_update(&p->tcf_tm);
-	bstats_update(&p->tcf_bstats, skb);
+	bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb);
+	spin_lock(&p->tcf_lock);
 	action = p->tcf_action;
 	update_flags = p->update_flags;
 	spin_unlock(&p->tcf_lock);

@@ -566,9 +566,7 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
 	return action;
 
 drop:
-	spin_lock(&p->tcf_lock);
-	p->tcf_qstats.drops++;
-	spin_unlock(&p->tcf_lock);
+	qstats_drop_inc(this_cpu_ptr(p->common.cpu_qstats));
 	return TC_ACT_SHOT;
 }
Signature · 1 line
2.14.3
author
#2 lore raw

PATCH v3 2/2 net/sched: act_csum: don't use spinlock in the fast path

In reply to Davide Caratti

use RCU instead of spin_{,unlock}_bh() to protect concurrent read/write on act_csum configuration, to reduce the effects of contention in the data path when multiple readers are present.

Signed-off-by Davide Caratti <[email protected]>
---
 include/net/tc_act/tc_csum.h | 16 ++++++++++--
 net/sched/act_csum.c         | 58 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 59 insertions(+), 15 deletions(-)
include/net/tc_act/tc_csum.h +14 -2
index 781f3433a0be..9470fd7e4350 100644
@@ -6,10 +6,16 @@
 #include <net/act_api.h>
 #include <linux/tc_act/tc_csum.h>
 
+struct tcf_csum_params {
+	int action;
+	u32 update_flags;
+	struct rcu_head rcu;
+};
+
 struct tcf_csum {
 	struct tc_action common;
 
-	u32 update_flags;
+	struct tcf_csum_params __rcu *params;
 };
 #define to_tcf_csum(a) ((struct tcf_csum *)a)
 

@@ -24,7 +30,13 @@ static inline bool is_tcf_csum(const struct tc_action *a)
 
 static inline u32 tcf_csum_update_flags(const struct tc_action *a)
 {
-	return to_tcf_csum(a)->update_flags;
+	u32 update_flags;
+
+	rcu_read_lock();
+	update_flags = rcu_dereference(to_tcf_csum(a)->params)->update_flags;
+	rcu_read_unlock();
+
+	return update_flags;
 }
 
 #endif /* __NET_TC_CSUM_H */
net/sched/act_csum.c +45 -13

118 lines not on this page · raw message

Signature · 1 line
2.14.3
Acked-by on the v3 series
#3 lore raw
In reply to Davide Caratti
Quoted 20 lines · Davide Caratti

Similarly to what has been done earlier with other actions [1][2], this series tries to improve the performance of 'csum' tc action, removing a spinlock in the data path. Patch 1 lets act_csum use per-CPU counters; patch 2 removes spin_{,un}lock_bh() calls from the act() method.

test procedure (using pktgen from https://github.com/netoptimizer):

 # ip link add name eth1 type dummy
 # ip link set dev eth1 up
 # tc qdisc add dev eth1 root handle 1: prio
 # for a in pass drop; do
Quoted 6 lines

tc filter del dev eth1 parent 1: pref 10 matchall action csum udp tc filter add dev eth1 parent 1: pref 10 matchall action csum udp $a for n in 2 4; do
./pktgen_bench_xmit_mode_queue_xmit.sh -v -s 64 -t $n -n 1000000 -i eth1 done
done

test results:

Quoted 7 lines
      |    |  before patch   |   after patch
  $a  | $n | avg. pps/thread | avg. pps/thread
 -----+----+-----------------+----------------
 pass |  2 |    1671463 ± 4% |    1920789 ± 3%
 pass |  4 |     648797 ± 1% |     738190 ± 1%
 drop |  2 |    3212692 ± 2% |    3719811 ± 2%
 drop |  4 |    1078824 ± 1% |    1328099 ± 1%

Looks good to me,

Acked-by Cong Wang <[email protected]>
maintainer
#4 lore raw
In reply to Davide Caratti

From: Davide Caratti <[email protected]>
Date: Mon, 22 Jan 2018 18:14:30 +0100

Quoted 28 lines

Similarly to what has been done earlier with other actions [1][2], this series tries to improve the performance of 'csum' tc action, removing a spinlock in the data path. Patch 1 lets act_csum use per-CPU counters; patch 2 removes spin_{,un}lock_bh() calls from the act() method.

test procedure (using pktgen from https://github.com/netoptimizer):

 # ip link add name eth1 type dummy
 # ip link set dev eth1 up
 # tc qdisc add dev eth1 root handle 1: prio
 # for a in pass drop; do
Quoted 6 lines

tc filter del dev eth1 parent 1: pref 10 matchall action csum udp tc filter add dev eth1 parent 1: pref 10 matchall action csum udp $a for n in 2 4; do
./pktgen_bench_xmit_mode_queue_xmit.sh -v -s 64 -t $n -n 1000000 -i eth1 done
done

test results:

      |    |  before patch   |   after patch
  $a  | $n | avg. pps/thread | avg. pps/thread
 -----+----+-----------------+----------------
 pass |  2 |    1671463 ± 4% |    1920789 ± 3%
 pass |  4 |     648797 ± 1% |     738190 ± 1%
 drop |  2 |    3212692 ± 2% |    3719811 ± 2%
 drop |  4 |    1078824 ± 1% |    1328099 ± 1%
Quoted 4 lines

references:

[1] https://www.spinics.net/lists/netdev/msg334760.html
[2] https://www.spinics.net/lists/netdev/msg465862.html

Series applied, thanks Davide.