[PATCH net-next 0/2] net/sched: remove spinlock from 'csum' action

[PATCH net-next 0/2] net/sched: remove spinlock from 'csum' action

[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 plane. Patch 1 lets act_csum use per-CPU counters;
patch 2 removes spin_{,un}lock_bh() calls from act() method.

test procedure:

 # ip link add name eth1 type dummy
 # ip link set dev eth1 up
 # tc qdisc add dev eth1 root handle 1: prio
 # tc filter add dev eth1 parent 1: matchall action csum udp
 # for n in 2 4 6 8 10 12 14 16; do
 > ./pktgen_bench_xmit_mode_queue_xmit.sh -v -s 64 -t $n -n 1000000 -i eth1
 > done

test results:

 $n |  avg. pps/core  | avg. pps/core | delta
    | (without patch) | (with patch)  |  (%)
 ---+-----------------+---------------+------
  2 |          484915 |        547716 |   13
  4 |          209551 |        254439 |   21
  6 |          143901 |        164695 |   14
  8 |          112423 |        127821 |   14
 10 |           91134 |        102950 |   13
 12 |           75374 |         85499 |   13
 14 |           64586 |         73426 |   14
 16 |           56635 |         64111 |   13

references:

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

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

 net/sched/act_csum.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

-- 
2.13.6

[PATCH net-next 1/2] net/sched: act_csum: use per-core statistics

[Davide Caratti]

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 <dcaratti@redhat.com>
---
 net/sched/act_csum.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index d836f998117b..22a555ba3985 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -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;
 }
 
-- 
2.13.6

[PATCH net-next 2/2] net/sched: act_csum: don't use spinlock in the fast path

[Davide Caratti]

there is no need for using spin_{,unlock}_bh() to protect simultaneous
read/write of act_csum configuration: it's sufficient to ensure that RTNL
lock has been taken when tcf_action and update_flags are written, so that
configuration is not racy. Then, in the data path, use READ_ONCE() to
read those values, to avoid lock contention among multiple readers.

Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/act_csum.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 22a555ba3985..ac8402d53cd5 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -80,10 +80,9 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
 	}
 
 	p = to_tcf_csum(*a);
-	spin_lock_bh(&p->tcf_lock);
+	ASSERT_RTNL();
 	p->tcf_action = parm->action;
 	p->update_flags = parm->update_flags;
-	spin_unlock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED)
 		tcf_idr_insert(tn, *a);
@@ -544,14 +543,12 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
 
 	tcf_lastuse_update(&p->tcf_tm);
 	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);
 
+	action = READ_ONCE(p->tcf_action);
 	if (unlikely(action == TC_ACT_SHOT))
 		goto drop;
 
+	update_flags = READ_ONCE(p->update_flags);
 	switch (tc_skb_protocol(skb)) {
 	case cpu_to_be16(ETH_P_IP):
 		if (!tcf_csum_ipv4(skb, update_flags))
-- 
2.13.6

Re: [PATCH net-next 2/2] net/sched: act_csum: don't use spinlock in the fast path

[David Miller]

From: Davide Caratti <dcaratti@redhat.com>
Date: Wed, 13 Dec 2017 10:48:38 +0100

> Then, in the data path, use READ_ONCE() to
> read those values, to avoid lock contention among multiple readers.
 ...
> @@ -544,14 +543,12 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
>  
>  	tcf_lastuse_update(&p->tcf_tm);
>  	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);
>  
> +	action = READ_ONCE(p->tcf_action);
>  	if (unlikely(action == TC_ACT_SHOT))
>  		goto drop;
>  
> +	update_flags = READ_ONCE(p->update_flags);
>  	switch (tc_skb_protocol(skb)) {
>  	case cpu_to_be16(ETH_P_IP):
>  		if (!tcf_csum_ipv4(skb, update_flags))

That's not why the lock is here.

We must read both action and flags atomically so that they are consistent
with eachother.

We must never use action from one configuration change and flags from
yet another.

Find a way to load both of these values with a single cpu load, then you
can legally remove the lock.

Re: [PATCH net-next 2/2] net/sched: act_csum: don't use spinlock in the fast path

[Davide Caratti]

On Wed, 2017-12-13 at 16:23 -0500, David Miller wrote:
> From: Davide Caratti <dcaratti@redhat.com>
> Date: Wed, 13 Dec 2017 10:48:38 +0100
> 
> > Then, in the data path, use READ_ONCE() to
> > read those values, to avoid lock contention among multiple readers.
>  ...
> > @@ -544,14 +543,12 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
> >  
> >       tcf_lastuse_update(&p->tcf_tm);
> >       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);
> >  
> > +     action = READ_ONCE(p->tcf_action);
> >       if (unlikely(action == TC_ACT_SHOT))
> >               goto drop;
> >  
> > +     update_flags = READ_ONCE(p->update_flags);
> >       switch (tc_skb_protocol(skb)) {
> >       case cpu_to_be16(ETH_P_IP):
> >               if (!tcf_csum_ipv4(skb, update_flags))

hi David, thank you for replying!

> That's not why the lock is here.
> 
> We must read both action and flags atomically so that they are consistent
> with eachother.
> 
> We must never use action from one configuration change and flags from
> yet another.

I was (erroneously) assuming that such behavior was acceptable, since it's
present almost in all other TC actions, even those where tcf_lock is used.
But agree, it's better not to introduce a race in a place where it's not
present.

> Find a way to load both of these values with a single cpu load, then you
> can legally remove the lock.

act_tunnel_key seems a good example for this, I will send a v2 soon.

-- 
davide