All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra
@ 2020-02-17 10:12 Vlad Buslov
  2020-02-17 10:12 ` [RESEND PATCH net-next 1/4] net: sched: lock action when translating it to " Vlad Buslov
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Vlad Buslov @ 2020-02-17 10:12 UTC (permalink / raw)
  To: netdev, davem
  Cc: jhs, xiyou.wangcong, jiri, pablo, marcelo.leitner, Vlad Buslov

Currently, TC flow_action infrastructure code obtain rtnl lock before
accessing action state in tc_setup_flow_action() function and releases
it afterwards. This behavior is not supposed to impact TC filter
insertion rate because filling flow_action representation is only a
small part of creating new filter and expensive operations (hardware
offload callbacks, classifiers, cls API code that creates chains and
classifiers instances) already support unlocked execution. However,
typical vswitch implementation might need to also dump TC filters
concurrently, for example to age out unused flows or update flow
counters. TC dump is fully serialized and holds rtnl lock during its
whole execution in kernel space. As such, it can significantly impact
concurrent tasks that try to intermittently obtain rtnl lock when
filling intermediate representation for new filter offload (performance
evaluation at the end of this mail).

Refactor flow_action cls API infrastructure and its dependencies to not
rely on rtnl lock for synchronization. Patch set overview:

- Refactor tc_setup_flow_action() to obtain action tcf_lock when
  accessing action state. Fix its dependencies to not obtain tcf_lock
  themselves and assume that caller already holds it (needs to be done
  in same patch to prevent deadlock) and not to call sleeping functions
  (needs to be done in same patch to prevent "sleeping while atomic"
  dmesg warnings).

- Refactor action helper functions to require tcf_lock instead of rtnl.
  Internally, all of the actions already use tcf_lock for
  synchronization to accommodate unlocked classifier API, so this change
  relies on already existing functionality.

- Remove rtnl lock and "rtnl_held" argument from tc_setup_flow_action()
  function.


To test the change, multiple concurrent TC instances are invoked with
following command:

time ls add* | xargs -n 1 -P 100 sudo tc -b

Ten batch files with following typical rules (100k each) are used:

filter add dev ens1f0_0 protocol ip ingress prio 1 handle 1 flower
	src_mac e4:11:0:0:0:0 dst_mac e4:12:0:0:0:0 src_ip 192.168.111.1
	dst_ip 192.168.111.2 ip_proto udp dst_port 1 src_port 1 action
	tunnel_key set id 1 src_ip 2.2.2.2 dst_ip 2.2.2.3 dst_port 4789
	no_percpu action mirred egress redirect dev vxlan1 no_percpu

TC dump of same device is called in infinite loop from five concurrent
instances:

while true do tc -s filter show dev $NIC ingress >/dev/null done

Results obtained on current net-next commit 9f68e3655aae ("Merge tag
'drm-next-2020-01-30' of git://anongit.freedesktop.org/drm/drm"):

               | net-next | this change 
---------------+----------+-------------
 TC add        | 6.3s     | 6.3s        
 TC add + dump | 29.3s    | 6.8s        

Test results confirm significant impact of concurrent TC dump. The
impact is almost fully mitigated by proposed change (differences can be
attributed to contention for chain and tp locks between add and dump TC
instances).

Vlad Buslov (4):
  net: sched: lock action when translating it to flow_action infra
  net: sched: refactor police action helpers to require tcf_lock
  net: sched: refactor ct action helpers to require tcf_lock
  net: sched: don't take rtnl lock during flow_action setup

 include/net/pkt_cls.h              |  2 +-
 include/net/tc_act/tc_ct.h         |  6 ++++--
 include/net/tc_act/tc_police.h     |  6 ++++--
 include/net/tc_act/tc_tunnel_key.h |  2 +-
 net/sched/act_sample.c             |  2 --
 net/sched/cls_api.c                | 25 ++++++++++++-------------
 net/sched/cls_flower.c             |  6 ++----
 net/sched/cls_matchall.c           |  4 ++--
 8 files changed, 26 insertions(+), 27 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RESEND PATCH net-next 1/4] net: sched: lock action when translating it to flow_action infra
  2020-02-17 10:12 [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra Vlad Buslov
@ 2020-02-17 10:12 ` Vlad Buslov
  2020-02-17 10:12 ` [RESEND PATCH net-next 2/4] net: sched: refactor police action helpers to require tcf_lock Vlad Buslov
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Vlad Buslov @ 2020-02-17 10:12 UTC (permalink / raw)
  To: netdev, davem
  Cc: jhs, xiyou.wangcong, jiri, pablo, marcelo.leitner, Vlad Buslov,
	Jiri Pirko

In order to remove dependency on rtnl lock, take action's tcfa_lock when
constructing its representation as flow_action_entry structure.

Refactor tcf_sample_get_group() to assume that caller holds tcf_lock and
don't take it manually. This callback is only called from flow_action infra
representation translator which now calls it with tcf_lock held, so this
refactoring is necessary to prevent deadlock.

Allocate memory with GFP_ATOMIC flag for ip_tunnel_info copy because
tcf_tunnel_info_copy() is only called from flow_action representation infra
code with tcf_lock spinlock taken.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/tc_act/tc_tunnel_key.h |  2 +-
 net/sched/act_sample.c             |  2 --
 net/sched/cls_api.c                | 17 +++++++++++------
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
index 0689d9bcdf84..2b3df076e5b6 100644
--- a/include/net/tc_act/tc_tunnel_key.h
+++ b/include/net/tc_act/tc_tunnel_key.h
@@ -69,7 +69,7 @@ tcf_tunnel_info_copy(const struct tc_action *a)
 	if (tun) {
 		size_t tun_size = sizeof(*tun) + tun->options_len;
 		struct ip_tunnel_info *tun_copy = kmemdup(tun, tun_size,
-							  GFP_KERNEL);
+							  GFP_ATOMIC);
 
 		return tun_copy;
 	}
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index ce948c1e24dc..5e2df590bb58 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -267,14 +267,12 @@ tcf_sample_get_group(const struct tc_action *a,
 	struct tcf_sample *s = to_sample(a);
 	struct psample_group *group;
 
-	spin_lock_bh(&s->tcf_lock);
 	group = rcu_dereference_protected(s->psample_group,
 					  lockdep_is_held(&s->tcf_lock));
 	if (group) {
 		psample_group_take(group);
 		*destructor = tcf_psample_group_put;
 	}
-	spin_unlock_bh(&s->tcf_lock);
 
 	return group;
 }
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index c2cdd0fc2e70..610505117780 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3435,7 +3435,7 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
 int tc_setup_flow_action(struct flow_action *flow_action,
 			 const struct tcf_exts *exts, bool rtnl_held)
 {
-	const struct tc_action *act;
+	struct tc_action *act;
 	int i, j, k, err = 0;
 
 	if (!exts)
@@ -3449,6 +3449,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 		struct flow_action_entry *entry;
 
 		entry = &flow_action->entries[j];
+		spin_lock_bh(&act->tcfa_lock);
 		if (is_tcf_gact_ok(act)) {
 			entry->id = FLOW_ACTION_ACCEPT;
 		} else if (is_tcf_gact_shot(act)) {
@@ -3489,13 +3490,13 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 				break;
 			default:
 				err = -EOPNOTSUPP;
-				goto err_out;
+				goto err_out_locked;
 			}
 		} else if (is_tcf_tunnel_set(act)) {
 			entry->id = FLOW_ACTION_TUNNEL_ENCAP;
 			err = tcf_tunnel_encap_get_tunnel(entry, act);
 			if (err)
-				goto err_out;
+				goto err_out_locked;
 		} else if (is_tcf_tunnel_release(act)) {
 			entry->id = FLOW_ACTION_TUNNEL_DECAP;
 		} else if (is_tcf_pedit(act)) {
@@ -3509,7 +3510,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 					break;
 				default:
 					err = -EOPNOTSUPP;
-					goto err_out;
+					goto err_out_locked;
 				}
 				entry->mangle.htype = tcf_pedit_htype(act, k);
 				entry->mangle.mask = tcf_pedit_mask(act, k);
@@ -3560,15 +3561,16 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 				entry->mpls_mangle.ttl = tcf_mpls_ttl(act);
 				break;
 			default:
-				goto err_out;
+				goto err_out_locked;
 			}
 		} else if (is_tcf_skbedit_ptype(act)) {
 			entry->id = FLOW_ACTION_PTYPE;
 			entry->ptype = tcf_skbedit_ptype(act);
 		} else {
 			err = -EOPNOTSUPP;
-			goto err_out;
+			goto err_out_locked;
 		}
+		spin_unlock_bh(&act->tcfa_lock);
 
 		if (!is_tcf_pedit(act))
 			j++;
@@ -3582,6 +3584,9 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 		tc_cleanup_flow_action(flow_action);
 
 	return err;
+err_out_locked:
+	spin_unlock_bh(&act->tcfa_lock);
+	goto err_out;
 }
 EXPORT_SYMBOL(tc_setup_flow_action);
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [RESEND PATCH net-next 2/4] net: sched: refactor police action helpers to require tcf_lock
  2020-02-17 10:12 [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra Vlad Buslov
  2020-02-17 10:12 ` [RESEND PATCH net-next 1/4] net: sched: lock action when translating it to " Vlad Buslov
@ 2020-02-17 10:12 ` Vlad Buslov
  2020-02-17 10:12 ` [RESEND PATCH net-next 3/4] net: sched: refactor ct " Vlad Buslov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Vlad Buslov @ 2020-02-17 10:12 UTC (permalink / raw)
  To: netdev, davem
  Cc: jhs, xiyou.wangcong, jiri, pablo, marcelo.leitner, Vlad Buslov,
	Jiri Pirko

In order to remove rtnl lock dependency from flow_action representation
translator, change rcu_dereference_bh_rtnl() to rcu_dereference_protected()
in police action helpers that provide external access to rate and burst
values. This is safe to do because the functions are not called from
anywhere else outside flow_action infrastructure which was modified to
obtain tcf_lock when accessing action data in one of previous patches in
the series.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/tc_act/tc_police.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
index cfdc7cb82cad..f098ad4424be 100644
--- a/include/net/tc_act/tc_police.h
+++ b/include/net/tc_act/tc_police.h
@@ -54,7 +54,8 @@ static inline u64 tcf_police_rate_bytes_ps(const struct tc_action *act)
 	struct tcf_police *police = to_police(act);
 	struct tcf_police_params *params;
 
-	params = rcu_dereference_bh_rtnl(police->params);
+	params = rcu_dereference_protected(police->params,
+					   lockdep_is_held(&police->tcf_lock));
 	return params->rate.rate_bytes_ps;
 }
 
@@ -63,7 +64,8 @@ static inline s64 tcf_police_tcfp_burst(const struct tc_action *act)
 	struct tcf_police *police = to_police(act);
 	struct tcf_police_params *params;
 
-	params = rcu_dereference_bh_rtnl(police->params);
+	params = rcu_dereference_protected(police->params,
+					   lockdep_is_held(&police->tcf_lock));
 	return params->tcfp_burst;
 }
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [RESEND PATCH net-next 3/4] net: sched: refactor ct action helpers to require tcf_lock
  2020-02-17 10:12 [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra Vlad Buslov
  2020-02-17 10:12 ` [RESEND PATCH net-next 1/4] net: sched: lock action when translating it to " Vlad Buslov
  2020-02-17 10:12 ` [RESEND PATCH net-next 2/4] net: sched: refactor police action helpers to require tcf_lock Vlad Buslov
@ 2020-02-17 10:12 ` Vlad Buslov
  2020-02-17 10:12 ` [RESEND PATCH net-next 4/4] net: sched: don't take rtnl lock during flow_action setup Vlad Buslov
  2020-02-17 22:17 ` [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra David Miller
  4 siblings, 0 replies; 6+ messages in thread
From: Vlad Buslov @ 2020-02-17 10:12 UTC (permalink / raw)
  To: netdev, davem
  Cc: jhs, xiyou.wangcong, jiri, pablo, marcelo.leitner, Vlad Buslov,
	Jiri Pirko

In order to remove rtnl lock dependency from flow_action representation
translator, change rtnl_dereference() to rcu_dereference_protected() in ct
action helpers that provide external access to zone and action values. This
is safe to do because the functions are not called from anywhere else
outside flow_action infrastructure which was modified to obtain tcf_lock
when accessing action data in one of previous patches in the series.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/tc_act/tc_ct.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
index bdc20ab3b88d..a8b156402873 100644
--- a/include/net/tc_act/tc_ct.h
+++ b/include/net/tc_act/tc_ct.h
@@ -33,8 +33,10 @@ struct tcf_ct {
 };
 
 #define to_ct(a) ((struct tcf_ct *)a)
-#define to_ct_params(a) ((struct tcf_ct_params *) \
-			 rtnl_dereference((to_ct(a)->params)))
+#define to_ct_params(a)							\
+	((struct tcf_ct_params *)					\
+	 rcu_dereference_protected(to_ct(a)->params,			\
+				   lockdep_is_held(&a->tcfa_lock)))
 
 static inline uint16_t tcf_ct_zone(const struct tc_action *a)
 {
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [RESEND PATCH net-next 4/4] net: sched: don't take rtnl lock during flow_action setup
  2020-02-17 10:12 [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra Vlad Buslov
                   ` (2 preceding siblings ...)
  2020-02-17 10:12 ` [RESEND PATCH net-next 3/4] net: sched: refactor ct " Vlad Buslov
@ 2020-02-17 10:12 ` Vlad Buslov
  2020-02-17 22:17 ` [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra David Miller
  4 siblings, 0 replies; 6+ messages in thread
From: Vlad Buslov @ 2020-02-17 10:12 UTC (permalink / raw)
  To: netdev, davem
  Cc: jhs, xiyou.wangcong, jiri, pablo, marcelo.leitner, Vlad Buslov,
	Jiri Pirko

Refactor tc_setup_flow_action() function not to use rtnl lock and remove
'rtnl_held' argument that is no longer needed.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/pkt_cls.h    | 2 +-
 net/sched/cls_api.c      | 8 +-------
 net/sched/cls_flower.c   | 6 ++----
 net/sched/cls_matchall.c | 4 ++--
 4 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a972244ab193..53946b509b51 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -509,7 +509,7 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
 }
 
 int tc_setup_flow_action(struct flow_action *flow_action,
-			 const struct tcf_exts *exts, bool rtnl_held);
+			 const struct tcf_exts *exts);
 void tc_cleanup_flow_action(struct flow_action *flow_action);
 
 int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 610505117780..13c33eaf1ca1 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3433,7 +3433,7 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
 }
 
 int tc_setup_flow_action(struct flow_action *flow_action,
-			 const struct tcf_exts *exts, bool rtnl_held)
+			 const struct tcf_exts *exts)
 {
 	struct tc_action *act;
 	int i, j, k, err = 0;
@@ -3441,9 +3441,6 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 	if (!exts)
 		return 0;
 
-	if (!rtnl_held)
-		rtnl_lock();
-
 	j = 0;
 	tcf_exts_for_each_action(i, act, exts) {
 		struct flow_action_entry *entry;
@@ -3577,9 +3574,6 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 	}
 
 err_out:
-	if (!rtnl_held)
-		rtnl_unlock();
-
 	if (err)
 		tc_cleanup_flow_action(flow_action);
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index f9c0d1e8d380..d7d3aab53120 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -449,8 +449,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
 	cls_flower.rule->match.key = &f->mkey;
 	cls_flower.classid = f->res.classid;
 
-	err = tc_setup_flow_action(&cls_flower.rule->action, &f->exts,
-				   rtnl_held);
+	err = tc_setup_flow_action(&cls_flower.rule->action, &f->exts);
 	if (err) {
 		kfree(cls_flower.rule);
 		if (skip_sw) {
@@ -1999,8 +1998,7 @@ static int fl_reoffload(struct tcf_proto *tp, bool add, flow_setup_cb_t *cb,
 		cls_flower.rule->match.mask = &f->mask->key;
 		cls_flower.rule->match.key = &f->mkey;
 
-		err = tc_setup_flow_action(&cls_flower.rule->action, &f->exts,
-					   true);
+		err = tc_setup_flow_action(&cls_flower.rule->action, &f->exts);
 		if (err) {
 			kfree(cls_flower.rule);
 			if (tc_skip_sw(f->flags)) {
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 039cc86974f4..bf2d42ee55a3 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -97,7 +97,7 @@ static int mall_replace_hw_filter(struct tcf_proto *tp,
 	cls_mall.command = TC_CLSMATCHALL_REPLACE;
 	cls_mall.cookie = cookie;
 
-	err = tc_setup_flow_action(&cls_mall.rule->action, &head->exts, true);
+	err = tc_setup_flow_action(&cls_mall.rule->action, &head->exts);
 	if (err) {
 		kfree(cls_mall.rule);
 		mall_destroy_hw_filter(tp, head, cookie, NULL);
@@ -301,7 +301,7 @@ static int mall_reoffload(struct tcf_proto *tp, bool add, flow_setup_cb_t *cb,
 		TC_CLSMATCHALL_REPLACE : TC_CLSMATCHALL_DESTROY;
 	cls_mall.cookie = (unsigned long)head;
 
-	err = tc_setup_flow_action(&cls_mall.rule->action, &head->exts, true);
+	err = tc_setup_flow_action(&cls_mall.rule->action, &head->exts);
 	if (err) {
 		kfree(cls_mall.rule);
 		if (add && tc_skip_sw(head->flags)) {
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra
  2020-02-17 10:12 [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra Vlad Buslov
                   ` (3 preceding siblings ...)
  2020-02-17 10:12 ` [RESEND PATCH net-next 4/4] net: sched: don't take rtnl lock during flow_action setup Vlad Buslov
@ 2020-02-17 22:17 ` David Miller
  4 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-02-17 22:17 UTC (permalink / raw)
  To: vladbu; +Cc: netdev, jhs, xiyou.wangcong, jiri, pablo, marcelo.leitner

From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 17 Feb 2020 12:12:08 +0200

 ...
> Refactor flow_action cls API infrastructure and its dependencies to not
> rely on rtnl lock for synchronization. Patch set overview:
 ...

Looks good, series applied, thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-02-17 22:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 10:12 [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra Vlad Buslov
2020-02-17 10:12 ` [RESEND PATCH net-next 1/4] net: sched: lock action when translating it to " Vlad Buslov
2020-02-17 10:12 ` [RESEND PATCH net-next 2/4] net: sched: refactor police action helpers to require tcf_lock Vlad Buslov
2020-02-17 10:12 ` [RESEND PATCH net-next 3/4] net: sched: refactor ct " Vlad Buslov
2020-02-17 10:12 ` [RESEND PATCH net-next 4/4] net: sched: don't take rtnl lock during flow_action setup Vlad Buslov
2020-02-17 22:17 ` [RESEND PATCH net-next 0/4] Remove rtnl lock dependency from flow_action infra David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.