Skip to contentSkip to conversation

net/sched: act_skbedit: lockless data path

PATCH In review Davide Caratti 2 participants 4 messages Jul 11, 2018 – Jul 12, 2018

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

Davide Caratti

author
#0 lore raw

v2 cover net/sched: act_skbedit: lockless data path

the data path of act_skbedit can be faster if we avoid using spinlocks:
- patch 1 converts act_skbedit statistics to use per-cpu counters
- patch 2 lets act_skbedit use RCU to read/update its configuration

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 clsact
 # tc filter add dev eth1 egress matchall action skbedit priority c1a0:c1a0
 # for c in 1 2 4 ; do
Quoted 2 lines

./pktgen_bench_xmit_mode_queue_xmit.sh -v -s 64 -t $c -n 5000000 -i eth1 done

test results (avg. pps/thread)

$c | before patch | after patch | improvement
----+--------------+--------------+------------

   1 | 3917464 ± 3% | 4000458 ± 3% |  irrelevant
   2 | 3455367 ± 4% | 3953076 ± 1% |        +14%
   4 | 2496594 ± 2% | 3801123 ± 3% |        +52%

v2: rebased on latest net-next

Davide Caratti (2):
net/sched: skbedit: use per-cpu counters net/sched: act_skbedit: don't use spinlock in the data path

 include/net/tc_act/tc_skbedit.h |  37 ++++++++---
 net/sched/act_skbedit.c         | 113 ++++++++++++++++++++------------
 2 files changed, 98 insertions(+), 52 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v2 1/2 net/sched: skbedit: use per-cpu counters

use per-CPU counters, instead of sharing a single set of stats with all
cores: this removes the need of spinlocks when stats are read/updated.

Signed-off-by Davide Caratti <[email protected]>
---
 net/sched/act_skbedit.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
net/sched/act_skbedit.c +4 -4
index 86521a74ecdd..8651b5bd6b59 100644
@@ -38,10 +38,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 {
 	struct tcf_skbedit *d = to_skbedit(a);
 
-	spin_lock(&d->tcf_lock);
 	tcf_lastuse_update(&d->tcf_tm);
-	bstats_update(&d->tcf_bstats, skb);
+	bstats_cpu_update(this_cpu_ptr(d->common.cpu_bstats), skb);
 
+	spin_lock(&d->tcf_lock);
 	if (d->flags & SKBEDIT_F_PRIORITY)
 		skb->priority = d->priority;
 	if (d->flags & SKBEDIT_F_INHERITDSFIELD) {

@@ -77,8 +77,8 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 	return d->tcf_action;
 
 err:
-	d->tcf_qstats.drops++;
 	spin_unlock(&d->tcf_lock);
+	qstats_drop_inc(this_cpu_ptr(d->common.cpu_qstats));
 	return TC_ACT_SHOT;
 }
 

@@ -169,7 +169,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 
 	if (!exists) {
 		ret = tcf_idr_create(tn, parm->index, est, a,
-				     &act_skbedit_ops, bind, false);
+				     &act_skbedit_ops, bind, true);
 		if (ret) {
 			tcf_idr_cleanup(tn, parm->index);
 			return ret;
Signature · 1 line
2.17.1
author
#2 lore raw

PATCH v2 2/2 net/sched: act_skbedit: don't use spinlock in the data path

In reply to Davide Caratti

use RCU instead of spin_{,un}lock_bh, to protect concurrent read/write on act_skbedit configuration. This reduces the effects of contention in the data path, in case multiple readers are present.

Signed-off-by Davide Caratti <[email protected]>
---
 include/net/tc_act/tc_skbedit.h |  37 ++++++++---
 net/sched/act_skbedit.c         | 107 ++++++++++++++++++++------------
 2 files changed, 95 insertions(+), 49 deletions(-)
include/net/tc_act/tc_skbedit.h +27 -10
index 19cd3d345804..911bbac838a2 100644
@@ -22,14 +22,19 @@
 #include <net/act_api.h>
 #include <linux/tc_act/tc_skbedit.h>
 
+struct tcf_skbedit_params {
+	u32 flags;
+	u32 priority;
+	u32 mark;
+	u32 mask;
+	u16 queue_mapping;
+	u16 ptype;
+	struct rcu_head rcu;
+};
+
 struct tcf_skbedit {
-	struct tc_action	common;
-	u32		flags;
-	u32		priority;
-	u32		mark;
-	u32		mask;
-	u16		queue_mapping;
-	u16		ptype;
+	struct tc_action common;
+	struct tcf_skbedit_params __rcu *params;
 };
 #define to_skbedit(a) ((struct tcf_skbedit *)a)
 

@@ -37,15 +42,27 @@ struct tcf_skbedit {
 static inline bool is_tcf_skbedit_mark(const struct tc_action *a)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	if (a->ops && a->ops->type == TCA_ACT_SKBEDIT)
-		return to_skbedit(a)->flags == SKBEDIT_F_MARK;
+	u32 flags;
+
+	if (a->ops && a->ops->type == TCA_ACT_SKBEDIT) {
+		rcu_read_lock();
+		flags = rcu_dereference(to_skbedit(a)->params)->flags;
+		rcu_read_unlock();
+		return flags == SKBEDIT_F_MARK;
+	}
 #endif
 	return false;
 }
 
 static inline u32 tcf_skbedit_mark(const struct tc_action *a)
 {
-	return to_skbedit(a)->mark;
+	u32 mark;
+
+	rcu_read_lock();
+	mark = rcu_dereference(to_skbedit(a)->params)->mark;
+	rcu_read_unlock();
+
+	return mark;
 }
 
 #endif /* __NET_TC_SKBEDIT_H */
net/sched/act_skbedit.c +68 -39

181 lines not on this page · raw message

Signature · 1 line
2.17.1
maintainer
#3 lore raw
In reply to Davide Caratti

From: Davide Caratti <[email protected]>
Date: Wed, 11 Jul 2018 16:04:48 +0200

Quoted 16 lines

the data path of act_skbedit can be faster if we avoid using spinlocks:
- patch 1 converts act_skbedit statistics to use per-cpu counters
- patch 2 lets act_skbedit use RCU to read/update its configuration

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 clsact
 # tc filter add dev eth1 egress matchall action skbedit priority c1a0:c1a0
 # for c in 1 2 4 ; do
Quoted 2 lines

./pktgen_bench_xmit_mode_queue_xmit.sh -v -s 64 -t $c -n 5000000 -i eth1 done

test results (avg. pps/thread)

Quoted 7 lines

$c | before patch | after patch | improvement
----+--------------+--------------+------------

   1 | 3917464 ± 3% | 4000458 ± 3% |  irrelevant
   2 | 3455367 ± 4% | 3953076 ± 1% |        +14%
   4 | 2496594 ± 2% | 3801123 ± 3% |        +52%

v2: rebased on latest net-next

Series applied, thank you.