netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc
@ 2022-11-28 15:44 Pedro Tammela
  2022-11-28 15:44 ` [PATCH net-next v2 1/3] net/sched: add retpoline wrapper " Pedro Tammela
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Pedro Tammela @ 2022-11-28 15:44 UTC (permalink / raw)
  To: netdev
  Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri, kuniyu,
	Pedro Tammela

In tc all qdics, classifiers and actions can be compiled as modules.
This results today in indirect calls in all transitions in the tc hierarchy.
Due to CONFIG_RETPOLINE, CPUs with mitigations=on might pay an extra cost on
indirect calls. For newer Intel cpus with IBRS the extra cost is
nonexistent, but AMD Zen cpus and older x86 cpus still go through the
retpoline thunk.

Known built-in symbols can be optimized into direct calls, thus
avoiding the retpoline thunk. So far, tc has not been leveraging this
build information and leaving out a performance optimization for some
CPUs. In this series we wire up 'tcf_classify()' and 'tcf_action_exec()'
with direct calls when known modules are compiled as built-in as an
opt-in optimization.

We measured these changes in one AMD Zen 3 cpu (Retpoline), one Intel 10th
Gen CPU (IBRS), one Intel 3rd Gen cpu (Retpoline) and one Intel Xeon CPU (IBRS)
using pktgen with 64b udp packets. Our test setup is a dummy device with
clsact and matchall in a kernel compiled with every tc module as built-in.
We observed a 3-6% speed up on the retpoline CPUs, when going through 1
tc filter, and a 60-100% speed up when going through 100 filters.
For the IBRS cpus we observed a 1-2% degradation in both scenarios, we believe
the extra branches checks introduced a small overhead therefore we added
a Kconfig option to make these changes opt-in even in CONFIG_RETPOLINE kernels.

We are continuing to test on other hardware variants as we find them:

1 filter:
CPU        | before (pps) | after (pps) | diff
R9 5950X   | 4237838      | 4412241     | +4.1%
R9 5950X   | 4265287      | 4413757     | +3.4%   [*]
i5-3337U   | 1580565      | 1682406     | +6.4%
i5-10210U  | 3006074      | 3006857     | +0.0%
i5-10210U  | 3160245      | 3179945     | +0.6%   [*]
Xeon 6230R | 3196906      | 3197059     | +0.0%
Xeon 6230R | 3190392      | 3196153     | +0.01%  [*]

100 filters:
CPU        | before (pps) | after (pps) | diff
R9 5950X   | 313469       | 633303      | +102.03%
R9 5950X   | 313797       | 633150      | +101.77% [*]
i5-3337U   | 127454       | 211210      | +65.71%
i5-10210U  | 389259       | 381765      | -1.9%
i5-10210U  | 408812       | 412730      | +0.9%    [*]
Xeon 6230R | 415420       | 406612      | -2.1%
Xeon 6230R | 416705       | 405869      | -2.6%    [*]

[*] In these tests we ran pktgen with clone set to 1000.

v1->v2:
- Fix build errors found by the bots
- Address Kuniyuki Iwashima suggestions

Pedro Tammela (3):
  net/sched: add retpoline wrapper for tc
  net/sched: avoid indirect act functions on retpoline kernels
  net/sched: avoid indirect classify functions on retpoline kernels

 include/net/tc_wrapper.h   | 232 +++++++++++++++++++++++++++++++++++++
 net/sched/Kconfig          |  13 +++
 net/sched/act_api.c        |   3 +-
 net/sched/act_bpf.c        |   6 +-
 net/sched/act_connmark.c   |   6 +-
 net/sched/act_csum.c       |   6 +-
 net/sched/act_ct.c         |   5 +-
 net/sched/act_ctinfo.c     |   6 +-
 net/sched/act_gact.c       |   6 +-
 net/sched/act_gate.c       |   6 +-
 net/sched/act_ife.c        |   6 +-
 net/sched/act_ipt.c        |   6 +-
 net/sched/act_mirred.c     |   6 +-
 net/sched/act_mpls.c       |   6 +-
 net/sched/act_nat.c        |   7 +-
 net/sched/act_pedit.c      |   6 +-
 net/sched/act_police.c     |   6 +-
 net/sched/act_sample.c     |   6 +-
 net/sched/act_simple.c     |   6 +-
 net/sched/act_skbedit.c    |   6 +-
 net/sched/act_skbmod.c     |   6 +-
 net/sched/act_tunnel_key.c |   6 +-
 net/sched/act_vlan.c       |   6 +-
 net/sched/cls_api.c        |   3 +-
 net/sched/cls_basic.c      |   6 +-
 net/sched/cls_bpf.c        |   6 +-
 net/sched/cls_cgroup.c     |   6 +-
 net/sched/cls_flow.c       |   6 +-
 net/sched/cls_flower.c     |   6 +-
 net/sched/cls_fw.c         |   6 +-
 net/sched/cls_matchall.c   |   6 +-
 net/sched/cls_route.c      |   6 +-
 net/sched/cls_rsvp.c       |   2 +
 net/sched/cls_rsvp.h       |   6 +-
 net/sched/cls_rsvp6.c      |   2 +
 net/sched/cls_tcindex.c    |   7 +-
 net/sched/cls_u32.c        |   6 +-
 37 files changed, 375 insertions(+), 67 deletions(-)
 create mode 100644 include/net/tc_wrapper.h

-- 
2.34.1


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

* [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-11-28 15:44 [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Pedro Tammela
@ 2022-11-28 15:44 ` Pedro Tammela
  2022-12-01  5:16   ` Jakub Kicinski
                     ` (2 more replies)
  2022-11-28 15:44 ` [PATCH net-next v2 2/3] net/sched: avoid indirect act functions on retpoline kernels Pedro Tammela
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 14+ messages in thread
From: Pedro Tammela @ 2022-11-28 15:44 UTC (permalink / raw)
  To: netdev
  Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri, kuniyu,
	Pedro Tammela

On kernels compiled with CONFIG_RETPOLINE and CONFIG_NET_TC_INDIRECT_WRAPPER,
optimize actions and filters that are compiled as built-ins into a direct call.
The calls are ordered alphabetically, but new ones should be ideally
added last.

On subsequent patches we expose the classifiers and actions functions
and wire up the wrapper into tc.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/net/tc_wrapper.h | 232 +++++++++++++++++++++++++++++++++++++++
 net/sched/Kconfig        |  13 +++
 2 files changed, 245 insertions(+)
 create mode 100644 include/net/tc_wrapper.h

diff --git a/include/net/tc_wrapper.h b/include/net/tc_wrapper.h
new file mode 100644
index 000000000000..bd2b4789db2b
--- /dev/null
+++ b/include/net/tc_wrapper.h
@@ -0,0 +1,232 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __NET_TC_WRAPPER_H
+#define __NET_TC_WRAPPER_H
+
+#include <linux/indirect_call_wrapper.h>
+#include <net/pkt_cls.h>
+
+#if IS_ENABLED(CONFIG_RETPOLINE) && IS_ENABLED(CONFIG_NET_TC_INDIRECT_WRAPPER)
+
+#define TC_INDIRECT_SCOPE
+
+/* TC Actions */
+#ifdef CONFIG_NET_CLS_ACT
+
+#define TC_INDIRECT_ACTION_DECLARE(fname)                              \
+	INDIRECT_CALLABLE_DECLARE(int fname(struct sk_buff *skb,       \
+					    const struct tc_action *a, \
+					    struct tcf_result *res))
+
+TC_INDIRECT_ACTION_DECLARE(tcf_bpf_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_connmark_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_csum_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_ct_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_ctinfo_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_gact_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_gate_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_ife_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_ipt_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_mirred_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_mpls_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_nat_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_pedit_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_police_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_sample_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_simp_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_skbedit_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_skbmod_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_vlan_act);
+TC_INDIRECT_ACTION_DECLARE(tunnel_key_act);
+
+static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
+			   struct tcf_result *res)
+{
+	if (0) { /* noop */ }
+#if IS_BUILTIN(CONFIG_NET_ACT_BPF)
+	else if (a->ops->act == tcf_bpf_act)
+		return tcf_bpf_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_CONNMARK)
+	else if (a->ops->act == tcf_connmark_act)
+		return tcf_connmark_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_CSUM)
+	else if (a->ops->act == tcf_csum_act)
+		return tcf_csum_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_CT)
+	else if (a->ops->act == tcf_ct_act)
+		return tcf_ct_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_CTINFO)
+	else if (a->ops->act == tcf_ctinfo_act)
+		return tcf_ctinfo_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_GACT)
+	else if (a->ops->act == tcf_gact_act)
+		return tcf_gact_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_GATE)
+	else if (a->ops->act == tcf_gate_act)
+		return tcf_gate_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_IFE)
+	else if (a->ops->act == tcf_ife_act)
+		return tcf_ife_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_IPT)
+	else if (a->ops->act == tcf_ipt_act)
+		return tcf_ipt_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_MIRRED)
+	else if (a->ops->act == tcf_mirred_act)
+		return tcf_mirred_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_MPLS)
+	else if (a->ops->act == tcf_mpls_act)
+		return tcf_mpls_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_NAT)
+	else if (a->ops->act == tcf_nat_act)
+		return tcf_nat_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_PEDIT)
+	else if (a->ops->act == tcf_pedit_act)
+		return tcf_pedit_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_POLICE)
+	else if (a->ops->act == tcf_police_act)
+		return tcf_police_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_SAMPLE)
+	else if (a->ops->act == tcf_sample_act)
+		return tcf_sample_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_SIMP)
+	else if (a->ops->act == tcf_simp_act)
+		return tcf_simp_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_SKBEDIT)
+	else if (a->ops->act == tcf_skbedit_act)
+		return tcf_skbedit_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_SKBMOD)
+	else if (a->ops->act == tcf_skbmod_act)
+		return tcf_skbmod_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_TUNNEL_KEY)
+	else if (a->ops->act == tunnel_key_act)
+		return tunnel_key_act(skb, a, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_ACT_VLAN)
+	else if (a->ops->act == tcf_vlan_act)
+		return tcf_vlan_act(skb, a, res);
+#endif
+	else
+		return a->ops->act(skb, a, res);
+}
+
+#endif /* CONFIG_NET_CLS_ACT */
+
+/* TC Filters */
+#ifdef CONFIG_NET_CLS
+
+#define TC_INDIRECT_FILTER_DECLARE(fname)                               \
+	INDIRECT_CALLABLE_DECLARE(int fname(struct sk_buff *skb,        \
+					    const struct tcf_proto *tp, \
+					    struct tcf_result *res))
+
+TC_INDIRECT_FILTER_DECLARE(basic_classify);
+TC_INDIRECT_FILTER_DECLARE(cls_bpf_classify);
+TC_INDIRECT_FILTER_DECLARE(cls_cgroup_classify);
+TC_INDIRECT_FILTER_DECLARE(fl_classify);
+TC_INDIRECT_FILTER_DECLARE(flow_classify);
+TC_INDIRECT_FILTER_DECLARE(fw_classify);
+TC_INDIRECT_FILTER_DECLARE(mall_classify);
+TC_INDIRECT_FILTER_DECLARE(route4_classify);
+TC_INDIRECT_FILTER_DECLARE(rsvp_classify);
+TC_INDIRECT_FILTER_DECLARE(rsvp6_classify);
+TC_INDIRECT_FILTER_DECLARE(tcindex_classify);
+TC_INDIRECT_FILTER_DECLARE(u32_classify);
+
+static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
+				struct tcf_result *res)
+{
+	if (0) { /* noop */ }
+#if IS_BUILTIN(CONFIG_NET_CLS_BASIC)
+	else if (tp->classify == basic_classify)
+		return basic_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_BPF)
+	else if (tp->classify == cls_bpf_classify)
+		return cls_bpf_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_CGROUP)
+	else if (tp->classify == cls_cgroup_classify)
+		return cls_cgroup_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_FLOW)
+	else if (tp->classify == flow_classify)
+		return flow_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_FLOWER)
+	else if (tp->classify == fl_classify)
+		return fl_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_FW)
+	else if (tp->classify == fw_classify)
+		return fw_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_MATCHALL)
+	else if (tp->classify == mall_classify)
+		return mall_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_ROUTE4)
+	else if (tp->classify == route4_classify)
+		return route4_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_RSVP)
+	else if (tp->classify == rsvp_classify)
+		return rsvp_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_RSVP6)
+	else if (tp->classify == rsvp6_classify)
+		return rsvp_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_TCINDEX)
+	else if (tp->classify == tcindex_classify)
+		return tcindex_classify(skb, tp, res);
+#endif
+#if IS_BUILTIN(CONFIG_NET_CLS_U32)
+	else if (tp->classify == u32_classify)
+		return u32_classify(skb, tp, res);
+#endif
+	else
+		return tp->classify(skb, tp, res);
+}
+
+#endif /* CONFIG_NET_CLS */
+
+#else
+
+#define TC_INDIRECT_SCOPE static
+
+#ifdef CONFIG_NET_CLS_ACT
+static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
+			   struct tcf_result *res)
+{
+	return a->ops->act(skb, a, res);
+}
+#endif
+
+#ifdef CONFIG_NET_CLS
+static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
+				struct tcf_result *res)
+{
+	return tp->classify(skb, tp, res);
+}
+#endif
+
+#endif
+
+#endif /* __NET_TC_WRAPPER_H */
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index 1e8ab4749c6c..9bc055f8013e 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -1021,6 +1021,19 @@ config NET_TC_SKB_EXT
 
 	  Say N here if you won't be using tc<->ovs offload or tc chains offload.
 
+config NET_TC_INDIRECT_WRAPPER
+	bool "TC indirect call wrapper"
+	depends on NET_SCHED
+	depends on RETPOLINE
+
+	help
+	  Say Y here to skip indirect calls in the TC datapath for known
+	  builtin classifiers/actions under CONFIG_RETPOLINE kernels.
+
+	  TC may run slower on CPUs with hardware based mitigations.
+
+	  If unsure, say N.
+
 endif # NET_SCHED
 
 config NET_SCH_FIFO
-- 
2.34.1


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

* [PATCH net-next v2 2/3] net/sched: avoid indirect act functions on retpoline kernels
  2022-11-28 15:44 [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Pedro Tammela
  2022-11-28 15:44 ` [PATCH net-next v2 1/3] net/sched: add retpoline wrapper " Pedro Tammela
@ 2022-11-28 15:44 ` Pedro Tammela
  2022-11-28 15:44 ` [PATCH net-next v2 3/3] net/sched: avoid indirect classify " Pedro Tammela
  2022-12-01 11:05 ` [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Paolo Abeni
  3 siblings, 0 replies; 14+ messages in thread
From: Pedro Tammela @ 2022-11-28 15:44 UTC (permalink / raw)
  To: netdev
  Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri, kuniyu,
	Pedro Tammela

Expose the necessary tc act functions and wire up act_api to use
direct calls in retpoline kernels.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_api.c        | 3 ++-
 net/sched/act_bpf.c        | 6 ++++--
 net/sched/act_connmark.c   | 6 ++++--
 net/sched/act_csum.c       | 6 ++++--
 net/sched/act_ct.c         | 5 +++--
 net/sched/act_ctinfo.c     | 6 ++++--
 net/sched/act_gact.c       | 6 ++++--
 net/sched/act_gate.c       | 6 ++++--
 net/sched/act_ife.c        | 6 ++++--
 net/sched/act_ipt.c        | 6 ++++--
 net/sched/act_mirred.c     | 6 ++++--
 net/sched/act_mpls.c       | 6 ++++--
 net/sched/act_nat.c        | 7 ++++---
 net/sched/act_pedit.c      | 6 ++++--
 net/sched/act_police.c     | 6 ++++--
 net/sched/act_sample.c     | 6 ++++--
 net/sched/act_simple.c     | 6 ++++--
 net/sched/act_skbedit.c    | 6 ++++--
 net/sched/act_skbmod.c     | 6 ++++--
 net/sched/act_tunnel_key.c | 6 ++++--
 net/sched/act_vlan.c       | 6 ++++--
 21 files changed, 81 insertions(+), 42 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 9b31a10cc639..b3276720b80d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -23,6 +23,7 @@
 #include <net/act_api.h>
 #include <net/netlink.h>
 #include <net/flow_offload.h>
+#include <net/tc_wrapper.h>
 
 #ifdef CONFIG_INET
 DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
@@ -1080,7 +1081,7 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 
 		repeat_ttl = 32;
 repeat:
-		ret = a->ops->act(skb, a, res);
+		ret = __tc_act(skb, a, res);
 		if (unlikely(ret == TC_ACT_REPEAT)) {
 			if (--repeat_ttl != 0)
 				goto repeat;
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index b79eee44e24e..b0455fda7d0b 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -18,6 +18,7 @@
 
 #include <linux/tc_act/tc_bpf.h>
 #include <net/tc_act/tc_bpf.h>
+#include <net/tc_wrapper.h>
 
 #define ACT_BPF_NAME_LEN	256
 
@@ -31,8 +32,9 @@ struct tcf_bpf_cfg {
 
 static struct tc_action_ops act_bpf_ops;
 
-static int tcf_bpf_act(struct sk_buff *skb, const struct tc_action *act,
-		       struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_bpf_act(struct sk_buff *skb,
+				  const struct tc_action *act,
+				  struct tcf_result *res)
 {
 	bool at_ingress = skb_at_tc_ingress(skb);
 	struct tcf_bpf *prog = to_bpf(act);
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 66b143bb04ac..3e643aced3b3 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -20,6 +20,7 @@
 #include <net/pkt_cls.h>
 #include <uapi/linux/tc_act/tc_connmark.h>
 #include <net/tc_act/tc_connmark.h>
+#include <net/tc_wrapper.h>
 
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_core.h>
@@ -27,8 +28,9 @@
 
 static struct tc_action_ops act_connmark_ops;
 
-static int tcf_connmark_act(struct sk_buff *skb, const struct tc_action *a,
-			    struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
+				       const struct tc_action *a,
+				       struct tcf_result *res)
 {
 	const struct nf_conntrack_tuple_hash *thash;
 	struct nf_conntrack_tuple tuple;
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 1366adf9b909..95e9304024b7 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -32,6 +32,7 @@
 
 #include <linux/tc_act/tc_csum.h>
 #include <net/tc_act/tc_csum.h>
+#include <net/tc_wrapper.h>
 
 static const struct nla_policy csum_policy[TCA_CSUM_MAX + 1] = {
 	[TCA_CSUM_PARMS] = { .len = sizeof(struct tc_csum), },
@@ -563,8 +564,9 @@ static int tcf_csum_ipv6(struct sk_buff *skb, u32 update_flags)
 	return 0;
 }
 
-static int tcf_csum_act(struct sk_buff *skb, const struct tc_action *a,
-			struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_csum_act(struct sk_buff *skb,
+				   const struct tc_action *a,
+				   struct tcf_result *res)
 {
 	struct tcf_csum *p = to_tcf_csum(a);
 	bool orig_vlan_tag_present = false;
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index da0b7f665277..249c138376bb 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -24,6 +24,7 @@
 #include <net/ipv6_frag.h>
 #include <uapi/linux/tc_act/tc_ct.h>
 #include <net/tc_act/tc_ct.h>
+#include <net/tc_wrapper.h>
 
 #include <net/netfilter/nf_flow_table.h>
 #include <net/netfilter/nf_conntrack.h>
@@ -1038,8 +1039,8 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
 #endif
 }
 
-static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
-		      struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
+				 struct tcf_result *res)
 {
 	struct net *net = dev_net(skb->dev);
 	enum ip_conntrack_info ctinfo;
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index d4102f0a9abd..0064934a4eac 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -18,6 +18,7 @@
 #include <net/pkt_cls.h>
 #include <uapi/linux/tc_act/tc_ctinfo.h>
 #include <net/tc_act/tc_ctinfo.h>
+#include <net/tc_wrapper.h>
 
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_core.h>
@@ -75,8 +76,9 @@ static void tcf_ctinfo_cpmark_set(struct nf_conn *ct, struct tcf_ctinfo *ca,
 	skb->mark = ct->mark & cp->cpmarkmask;
 }
 
-static int tcf_ctinfo_act(struct sk_buff *skb, const struct tc_action *a,
-			  struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_ctinfo_act(struct sk_buff *skb,
+				     const struct tc_action *a,
+				     struct tcf_result *res)
 {
 	const struct nf_conntrack_tuple_hash *thash = NULL;
 	struct tcf_ctinfo *ca = to_ctinfo(a);
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 62d682b96b88..54f1b13b2360 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -18,6 +18,7 @@
 #include <net/pkt_cls.h>
 #include <linux/tc_act/tc_gact.h>
 #include <net/tc_act/tc_gact.h>
+#include <net/tc_wrapper.h>
 
 static struct tc_action_ops act_gact_ops;
 
@@ -145,8 +146,9 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	return err;
 }
 
-static int tcf_gact_act(struct sk_buff *skb, const struct tc_action *a,
-			struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_gact_act(struct sk_buff *skb,
+				   const struct tc_action *a,
+				   struct tcf_result *res)
 {
 	struct tcf_gact *gact = to_gact(a);
 	int action = READ_ONCE(gact->tcf_action);
diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 3049878e7315..9b8def0be41e 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -14,6 +14,7 @@
 #include <net/netlink.h>
 #include <net/pkt_cls.h>
 #include <net/tc_act/tc_gate.h>
+#include <net/tc_wrapper.h>
 
 static struct tc_action_ops act_gate_ops;
 
@@ -113,8 +114,9 @@ static enum hrtimer_restart gate_timer_func(struct hrtimer *timer)
 	return HRTIMER_RESTART;
 }
 
-static int tcf_gate_act(struct sk_buff *skb, const struct tc_action *a,
-			struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_gate_act(struct sk_buff *skb,
+				   const struct tc_action *a,
+				   struct tcf_result *res)
 {
 	struct tcf_gate *gact = to_gate(a);
 
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 41d63b33461d..bc7611b0744c 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -29,6 +29,7 @@
 #include <net/tc_act/tc_ife.h>
 #include <linux/etherdevice.h>
 #include <net/ife.h>
+#include <net/tc_wrapper.h>
 
 static int max_metacnt = IFE_META_MAX + 1;
 static struct tc_action_ops act_ife_ops;
@@ -861,8 +862,9 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
 	return action;
 }
 
-static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
-		       struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_ife_act(struct sk_buff *skb,
+				  const struct tc_action *a,
+				  struct tcf_result *res)
 {
 	struct tcf_ife_info *ife = to_ife(a);
 	struct tcf_ife_params *p;
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 1625e1037416..5d96ffebd40f 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -20,6 +20,7 @@
 #include <net/pkt_sched.h>
 #include <linux/tc_act/tc_ipt.h>
 #include <net/tc_act/tc_ipt.h>
+#include <net/tc_wrapper.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 
@@ -216,8 +217,9 @@ static int tcf_xt_init(struct net *net, struct nlattr *nla,
 			      a, &act_xt_ops, tp, flags);
 }
 
-static int tcf_ipt_act(struct sk_buff *skb, const struct tc_action *a,
-		       struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
+				  const struct tc_action *a,
+				  struct tcf_result *res)
 {
 	int ret = 0, result = 0;
 	struct tcf_ipt *ipt = to_ipt(a);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index b8ad6ae282c0..7284bcea7b0b 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -24,6 +24,7 @@
 #include <net/pkt_cls.h>
 #include <linux/tc_act/tc_mirred.h>
 #include <net/tc_act/tc_mirred.h>
+#include <net/tc_wrapper.h>
 
 static LIST_HEAD(mirred_list);
 static DEFINE_SPINLOCK(mirred_list_lock);
@@ -217,8 +218,9 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
 	return err;
 }
 
-static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
-			  struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
+				     const struct tc_action *a,
+				     struct tcf_result *res)
 {
 	struct tcf_mirred *m = to_mirred(a);
 	struct sk_buff *skb2 = skb;
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index 8ad25cc8ccd5..ff47ce4d3968 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -14,6 +14,7 @@
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
 #include <net/tc_act/tc_mpls.h>
+#include <net/tc_wrapper.h>
 
 static struct tc_action_ops act_mpls_ops;
 
@@ -49,8 +50,9 @@ static __be32 tcf_mpls_get_lse(struct mpls_shim_hdr *lse,
 	return cpu_to_be32(new_lse);
 }
 
-static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a,
-			struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_mpls_act(struct sk_buff *skb,
+				   const struct tc_action *a,
+				   struct tcf_result *res)
 {
 	struct tcf_mpls *m = to_mpls(a);
 	struct tcf_mpls_params *p;
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 9265145f1040..74c74be33048 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -24,7 +24,7 @@
 #include <net/tc_act/tc_nat.h>
 #include <net/tcp.h>
 #include <net/udp.h>
-
+#include <net/tc_wrapper.h>
 
 static struct tc_action_ops act_nat_ops;
 
@@ -98,8 +98,9 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	return err;
 }
 
-static int tcf_nat_act(struct sk_buff *skb, const struct tc_action *a,
-		       struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_nat_act(struct sk_buff *skb,
+				  const struct tc_action *a,
+				  struct tcf_result *res)
 {
 	struct tcf_nat *p = to_tcf_nat(a);
 	struct iphdr *iph;
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 94ed5857ce67..a0378e9f0121 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -20,6 +20,7 @@
 #include <net/tc_act/tc_pedit.h>
 #include <uapi/linux/tc_act/tc_pedit.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 static struct tc_action_ops act_pedit_ops;
 
@@ -319,8 +320,9 @@ static int pedit_skb_hdr_offset(struct sk_buff *skb,
 	return ret;
 }
 
-static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
-			 struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
+				    const struct tc_action *a,
+				    struct tcf_result *res)
 {
 	struct tcf_pedit *p = to_pedit(a);
 	u32 max_offset;
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 0adb26e366a7..227cba58ce9f 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -19,6 +19,7 @@
 #include <net/netlink.h>
 #include <net/pkt_cls.h>
 #include <net/tc_act/tc_police.h>
+#include <net/tc_wrapper.h>
 
 /* Each policer is serialized by its individual spinlock */
 
@@ -242,8 +243,9 @@ static bool tcf_police_mtu_check(struct sk_buff *skb, u32 limit)
 	return len <= limit;
 }
 
-static int tcf_police_act(struct sk_buff *skb, const struct tc_action *a,
-			  struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_police_act(struct sk_buff *skb,
+				     const struct tc_action *a,
+				     struct tcf_result *res)
 {
 	struct tcf_police *police = to_police(a);
 	s64 now, toks, ppstoks = 0, ptoks = 0;
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 7a25477f5d99..98dea08c1764 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -20,6 +20,7 @@
 #include <net/tc_act/tc_sample.h>
 #include <net/psample.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 #include <linux/if_arp.h>
 
@@ -153,8 +154,9 @@ static bool tcf_sample_dev_ok_push(struct net_device *dev)
 	}
 }
 
-static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
-			  struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
+				     const struct tc_action *a,
+				     struct tcf_result *res)
 {
 	struct tcf_sample *s = to_sample(a);
 	struct psample_group *psample_group;
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 18d376135461..4b84514534f3 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -14,6 +14,7 @@
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 #include <linux/tc_act/tc_defact.h>
 #include <net/tc_act/tc_defact.h>
@@ -21,8 +22,9 @@
 static struct tc_action_ops act_simp_ops;
 
 #define SIMP_MAX_DATA	32
-static int tcf_simp_act(struct sk_buff *skb, const struct tc_action *a,
-			struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_simp_act(struct sk_buff *skb,
+				   const struct tc_action *a,
+				   struct tcf_result *res)
 {
 	struct tcf_defact *d = to_defact(a);
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 1710780c908a..ce7008cf291c 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -16,6 +16,7 @@
 #include <net/ipv6.h>
 #include <net/dsfield.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 #include <linux/tc_act/tc_skbedit.h>
 #include <net/tc_act/tc_skbedit.h>
@@ -36,8 +37,9 @@ static u16 tcf_skbedit_hash(struct tcf_skbedit_params *params,
 	return netdev_cap_txqueue(skb->dev, queue_mapping);
 }
 
-static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
-			   struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_skbedit_act(struct sk_buff *skb,
+				      const struct tc_action *a,
+				      struct tcf_result *res)
 {
 	struct tcf_skbedit *d = to_skbedit(a);
 	struct tcf_skbedit_params *params;
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index d98758a63934..dffa990a9629 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -15,14 +15,16 @@
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 #include <linux/tc_act/tc_skbmod.h>
 #include <net/tc_act/tc_skbmod.h>
 
 static struct tc_action_ops act_skbmod_ops;
 
-static int tcf_skbmod_act(struct sk_buff *skb, const struct tc_action *a,
-			  struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_skbmod_act(struct sk_buff *skb,
+				     const struct tc_action *a,
+				     struct tcf_result *res)
 {
 	struct tcf_skbmod *d = to_skbmod(a);
 	int action, max_edit_len, err;
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 2691a3d8e451..2d12d2626415 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -16,14 +16,16 @@
 #include <net/pkt_sched.h>
 #include <net/dst.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 #include <linux/tc_act/tc_tunnel_key.h>
 #include <net/tc_act/tc_tunnel_key.h>
 
 static struct tc_action_ops act_tunnel_key_ops;
 
-static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
-			  struct tcf_result *res)
+TC_INDIRECT_SCOPE int tunnel_key_act(struct sk_buff *skb,
+				     const struct tc_action *a,
+				     struct tcf_result *res)
 {
 	struct tcf_tunnel_key *t = to_tunnel_key(a);
 	struct tcf_tunnel_key_params *params;
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 7b24e898a3e6..0251442f5f29 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -12,14 +12,16 @@
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 #include <linux/tc_act/tc_vlan.h>
 #include <net/tc_act/tc_vlan.h>
 
 static struct tc_action_ops act_vlan_ops;
 
-static int tcf_vlan_act(struct sk_buff *skb, const struct tc_action *a,
-			struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcf_vlan_act(struct sk_buff *skb,
+				   const struct tc_action *a,
+				   struct tcf_result *res)
 {
 	struct tcf_vlan *v = to_vlan(a);
 	struct tcf_vlan_params *p;
-- 
2.34.1


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

* [PATCH net-next v2 3/3] net/sched: avoid indirect classify functions on retpoline kernels
  2022-11-28 15:44 [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Pedro Tammela
  2022-11-28 15:44 ` [PATCH net-next v2 1/3] net/sched: add retpoline wrapper " Pedro Tammela
  2022-11-28 15:44 ` [PATCH net-next v2 2/3] net/sched: avoid indirect act functions on retpoline kernels Pedro Tammela
@ 2022-11-28 15:44 ` Pedro Tammela
  2022-12-01 11:05 ` [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Paolo Abeni
  3 siblings, 0 replies; 14+ messages in thread
From: Pedro Tammela @ 2022-11-28 15:44 UTC (permalink / raw)
  To: netdev
  Cc: davem, edumazet, kuba, pabeni, jhs, xiyou.wangcong, jiri, kuniyu,
	Pedro Tammela

Expose the necessary tc classifier functions and wire up cls_api to use
direct calls in retpoline kernels.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/cls_api.c      | 3 ++-
 net/sched/cls_basic.c    | 6 ++++--
 net/sched/cls_bpf.c      | 6 ++++--
 net/sched/cls_cgroup.c   | 6 ++++--
 net/sched/cls_flow.c     | 6 ++++--
 net/sched/cls_flower.c   | 6 ++++--
 net/sched/cls_fw.c       | 6 ++++--
 net/sched/cls_matchall.c | 6 ++++--
 net/sched/cls_route.c    | 6 ++++--
 net/sched/cls_rsvp.c     | 2 ++
 net/sched/cls_rsvp.h     | 6 +++---
 net/sched/cls_rsvp6.c    | 2 ++
 net/sched/cls_tcindex.c  | 7 ++++---
 net/sched/cls_u32.c      | 6 ++++--
 14 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 23d1cfa4f58c..3f86616f1abb 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -40,6 +40,7 @@
 #include <net/tc_act/tc_mpls.h>
 #include <net/tc_act/tc_gate.h>
 #include <net/flow_offload.h>
+#include <net/tc_wrapper.h>
 
 extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1];
 
@@ -1564,7 +1565,7 @@ static inline int __tcf_classify(struct sk_buff *skb,
 		    tp->protocol != htons(ETH_P_ALL))
 			continue;
 
-		err = tp->classify(skb, tp, res);
+		err = __tc_classify(skb, tp, res);
 #ifdef CONFIG_NET_CLS_ACT
 		if (unlikely(err == TC_ACT_RECLASSIFY && !compat_mode)) {
 			first_tp = orig_tp;
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index d229ce99e554..1b92c33b5f81 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -18,6 +18,7 @@
 #include <net/netlink.h>
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 struct basic_head {
 	struct list_head	flist;
@@ -36,8 +37,9 @@ struct basic_filter {
 	struct rcu_work		rwork;
 };
 
-static int basic_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			  struct tcf_result *res)
+TC_INDIRECT_SCOPE int basic_classify(struct sk_buff *skb,
+				     const struct tcf_proto *tp,
+				     struct tcf_result *res)
 {
 	int r;
 	struct basic_head *head = rcu_dereference_bh(tp->root);
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index bc317b3eac12..466c26df853a 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -19,6 +19,7 @@
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
 #include <net/sock.h>
+#include <net/tc_wrapper.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Daniel Borkmann <dborkman@redhat.com>");
@@ -77,8 +78,9 @@ static int cls_bpf_exec_opcode(int code)
 	}
 }
 
-static int cls_bpf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			    struct tcf_result *res)
+TC_INDIRECT_SCOPE int cls_bpf_classify(struct sk_buff *skb,
+				       const struct tcf_proto *tp,
+				       struct tcf_result *res)
 {
 	struct cls_bpf_head *head = rcu_dereference_bh(tp->root);
 	bool at_ingress = skb_at_tc_ingress(skb);
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index ed00001b528a..bd9322d71910 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -13,6 +13,7 @@
 #include <net/pkt_cls.h>
 #include <net/sock.h>
 #include <net/cls_cgroup.h>
+#include <net/tc_wrapper.h>
 
 struct cls_cgroup_head {
 	u32			handle;
@@ -22,8 +23,9 @@ struct cls_cgroup_head {
 	struct rcu_work		rwork;
 };
 
-static int cls_cgroup_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			       struct tcf_result *res)
+TC_INDIRECT_SCOPE int cls_cgroup_classify(struct sk_buff *skb,
+					  const struct tcf_proto *tp,
+					  struct tcf_result *res)
 {
 	struct cls_cgroup_head *head = rcu_dereference_bh(tp->root);
 	u32 classid = task_get_classid(skb);
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 014cd3de7b5d..535668e1f748 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -24,6 +24,7 @@
 #include <net/ip.h>
 #include <net/route.h>
 #include <net/flow_dissector.h>
+#include <net/tc_wrapper.h>
 
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 #include <net/netfilter/nf_conntrack.h>
@@ -292,8 +293,9 @@ static u32 flow_key_get(struct sk_buff *skb, int key, struct flow_keys *flow)
 			  (1 << FLOW_KEY_NFCT_PROTO_SRC) |	\
 			  (1 << FLOW_KEY_NFCT_PROTO_DST))
 
-static int flow_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			 struct tcf_result *res)
+TC_INDIRECT_SCOPE int flow_classify(struct sk_buff *skb,
+				    const struct tcf_proto *tp,
+				    struct tcf_result *res)
 {
 	struct flow_head *head = rcu_dereference_bh(tp->root);
 	struct flow_filter *f;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 25bc57ee6ea1..0b15698b3531 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -27,6 +27,7 @@
 #include <net/vxlan.h>
 #include <net/erspan.h>
 #include <net/gtp.h>
+#include <net/tc_wrapper.h>
 
 #include <net/dst.h>
 #include <net/dst_metadata.h>
@@ -305,8 +306,9 @@ static u16 fl_ct_info_to_flower_map[] = {
 					TCA_FLOWER_KEY_CT_FLAGS_NEW,
 };
 
-static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-		       struct tcf_result *res)
+TC_INDIRECT_SCOPE int fl_classify(struct sk_buff *skb,
+				  const struct tcf_proto *tp,
+				  struct tcf_result *res)
 {
 	struct cls_fl_head *head = rcu_dereference_bh(tp->root);
 	bool post_ct = tc_skb_cb(skb)->post_ct;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index a32351da968c..ae9439a6c56c 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -21,6 +21,7 @@
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
 #include <net/sch_generic.h>
+#include <net/tc_wrapper.h>
 
 #define HTSIZE 256
 
@@ -47,8 +48,9 @@ static u32 fw_hash(u32 handle)
 	return handle % HTSIZE;
 }
 
-static int fw_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-		       struct tcf_result *res)
+TC_INDIRECT_SCOPE int fw_classify(struct sk_buff *skb,
+				  const struct tcf_proto *tp,
+				  struct tcf_result *res)
 {
 	struct fw_head *head = rcu_dereference_bh(tp->root);
 	struct fw_filter *f;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 39a5d9c170de..705f63da2c21 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -12,6 +12,7 @@
 
 #include <net/sch_generic.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 struct cls_mall_head {
 	struct tcf_exts exts;
@@ -24,8 +25,9 @@ struct cls_mall_head {
 	bool deleting;
 };
 
-static int mall_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			 struct tcf_result *res)
+TC_INDIRECT_SCOPE int mall_classify(struct sk_buff *skb,
+				    const struct tcf_proto *tp,
+				    struct tcf_result *res)
 {
 	struct cls_mall_head *head = rcu_dereference_bh(tp->root);
 
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 9e43b929d4ca..d0c53724d3e8 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -17,6 +17,7 @@
 #include <net/netlink.h>
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 /*
  * 1. For now we assume that route tags < 256.
@@ -121,8 +122,9 @@ static inline int route4_hash_wild(void)
 	return 0;						\
 }
 
-static int route4_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			   struct tcf_result *res)
+TC_INDIRECT_SCOPE int route4_classify(struct sk_buff *skb,
+				      const struct tcf_proto *tp,
+				      struct tcf_result *res)
 {
 	struct route4_head *head = rcu_dereference_bh(tp->root);
 	struct dst_entry *dst;
diff --git a/net/sched/cls_rsvp.c b/net/sched/cls_rsvp.c
index de1c1d4da597..03d8619bd9c6 100644
--- a/net/sched/cls_rsvp.c
+++ b/net/sched/cls_rsvp.c
@@ -15,10 +15,12 @@
 #include <net/netlink.h>
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
+#include <net/tc_wrapper.h>
 
 #define RSVP_DST_LEN	1
 #define RSVP_ID		"rsvp"
 #define RSVP_OPS	cls_rsvp_ops
+#define RSVP_CLS	rsvp_classify
 
 #include "cls_rsvp.h"
 MODULE_LICENSE("GPL");
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index b00a7dbd0587..869efba9f834 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -124,8 +124,8 @@ static inline unsigned int hash_src(__be32 *src)
 		return r;				\
 }
 
-static int rsvp_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			 struct tcf_result *res)
+TC_INDIRECT_SCOPE int RSVP_CLS(struct sk_buff *skb, const struct tcf_proto *tp,
+			       struct tcf_result *res)
 {
 	struct rsvp_head *head = rcu_dereference_bh(tp->root);
 	struct rsvp_session *s;
@@ -738,7 +738,7 @@ static void rsvp_bind_class(void *fh, u32 classid, unsigned long cl, void *q,
 
 static struct tcf_proto_ops RSVP_OPS __read_mostly = {
 	.kind		=	RSVP_ID,
-	.classify	=	rsvp_classify,
+	.classify	=	RSVP_CLS,
 	.init		=	rsvp_init,
 	.destroy	=	rsvp_destroy,
 	.get		=	rsvp_get,
diff --git a/net/sched/cls_rsvp6.c b/net/sched/cls_rsvp6.c
index 64078846000e..e627cc32d633 100644
--- a/net/sched/cls_rsvp6.c
+++ b/net/sched/cls_rsvp6.c
@@ -15,10 +15,12 @@
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
 #include <net/netlink.h>
+#include <net/tc_wrapper.h>
 
 #define RSVP_DST_LEN	4
 #define RSVP_ID		"rsvp6"
 #define RSVP_OPS	cls_rsvp6_ops
+#define RSVP_CLS rsvp6_classify
 
 #include "cls_rsvp.h"
 MODULE_LICENSE("GPL");
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 1c9eeb98d826..eb0e9458e722 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -16,6 +16,7 @@
 #include <net/netlink.h>
 #include <net/pkt_cls.h>
 #include <net/sch_generic.h>
+#include <net/tc_wrapper.h>
 
 /*
  * Passing parameters to the root seems to be done more awkwardly than really
@@ -98,9 +99,9 @@ static struct tcindex_filter_result *tcindex_lookup(struct tcindex_data *p,
 	return NULL;
 }
 
-
-static int tcindex_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			    struct tcf_result *res)
+TC_INDIRECT_SCOPE int tcindex_classify(struct sk_buff *skb,
+				       const struct tcf_proto *tp,
+				       struct tcf_result *res)
 {
 	struct tcindex_data *p = rcu_dereference_bh(tp->root);
 	struct tcindex_filter_result *f;
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 34d25f7a0687..4e2e269f121f 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -39,6 +39,7 @@
 #include <net/act_api.h>
 #include <net/pkt_cls.h>
 #include <linux/idr.h>
+#include <net/tc_wrapper.h>
 
 struct tc_u_knode {
 	struct tc_u_knode __rcu	*next;
@@ -100,8 +101,9 @@ static inline unsigned int u32_hash_fold(__be32 key,
 	return h;
 }
 
-static int u32_classify(struct sk_buff *skb, const struct tcf_proto *tp,
-			struct tcf_result *res)
+TC_INDIRECT_SCOPE int u32_classify(struct sk_buff *skb,
+				   const struct tcf_proto *tp,
+				   struct tcf_result *res)
 {
 	struct {
 		struct tc_u_knode *knode;
-- 
2.34.1


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

* Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-11-28 15:44 ` [PATCH net-next v2 1/3] net/sched: add retpoline wrapper " Pedro Tammela
@ 2022-12-01  5:16   ` Jakub Kicinski
  2022-12-01 16:40     ` Pedro Tammela
  2022-12-01 11:10   ` Paolo Abeni
  2022-12-05  4:56   ` Eric Dumazet
  2 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2022-12-01  5:16 UTC (permalink / raw)
  To: Pedro Tammela
  Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri,
	kuniyu, Pedro Tammela

On Mon, 28 Nov 2022 12:44:54 -0300 Pedro Tammela wrote:
> On kernels compiled with CONFIG_RETPOLINE and CONFIG_NET_TC_INDIRECT_WRAPPER,
> optimize actions and filters that are compiled as built-ins into a direct call.
> The calls are ordered alphabetically, but new ones should be ideally
> added last.
> 
> On subsequent patches we expose the classifiers and actions functions
> and wire up the wrapper into tc.

> +#if IS_ENABLED(CONFIG_RETPOLINE) && IS_ENABLED(CONFIG_NET_TC_INDIRECT_WRAPPER)

The latter 'depends on' former, so just check the latter.

> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
> +			   struct tcf_result *res)
> +{
> +	if (0) { /* noop */ }
> +#if IS_BUILTIN(CONFIG_NET_ACT_BPF)
> +	else if (a->ops->act == tcf_bpf_act)
> +		return tcf_bpf_act(skb, a, res);
> +#endif

How does the 'else if' ladder compare to a switch statement?

> +#ifdef CONFIG_NET_CLS_ACT
> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
> +			   struct tcf_result *res)
> +{
> +	return a->ops->act(skb, a, res);
> +}
> +#endif
> +
> +#ifdef CONFIG_NET_CLS
> +static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
> +				struct tcf_result *res)
> +{
> +	return tp->classify(skb, tp, res);
> +}
> +#endif

please don't wrap the static inline helpers in #ifdefs unless it's
actually necessary for build to pass.

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

* Re: [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc
  2022-11-28 15:44 [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Pedro Tammela
                   ` (2 preceding siblings ...)
  2022-11-28 15:44 ` [PATCH net-next v2 3/3] net/sched: avoid indirect classify " Pedro Tammela
@ 2022-12-01 11:05 ` Paolo Abeni
  2022-12-01 12:34   ` Jamal Hadi Salim
  3 siblings, 1 reply; 14+ messages in thread
From: Paolo Abeni @ 2022-12-01 11:05 UTC (permalink / raw)
  To: Pedro Tammela, netdev
  Cc: davem, edumazet, kuba, jhs, xiyou.wangcong, jiri, kuniyu, Pedro Tammela

On Mon, 2022-11-28 at 12:44 -0300, Pedro Tammela wrote:
> In tc all qdics, classifiers and actions can be compiled as modules.
> This results today in indirect calls in all transitions in the tc hierarchy.
> Due to CONFIG_RETPOLINE, CPUs with mitigations=on might pay an extra cost on
> indirect calls. For newer Intel cpus with IBRS the extra cost is
> nonexistent, but AMD Zen cpus and older x86 cpus still go through the
> retpoline thunk.
> 
> Known built-in symbols can be optimized into direct calls, thus
> avoiding the retpoline thunk. So far, tc has not been leveraging this
> build information and leaving out a performance optimization for some
> CPUs. In this series we wire up 'tcf_classify()' and 'tcf_action_exec()'
> with direct calls when known modules are compiled as built-in as an
> opt-in optimization.
> 
> We measured these changes in one AMD Zen 3 cpu (Retpoline), one Intel 10th
> Gen CPU (IBRS), one Intel 3rd Gen cpu (Retpoline) and one Intel Xeon CPU (IBRS)
> using pktgen with 64b udp packets. Our test setup is a dummy device with
> clsact and matchall in a kernel compiled with every tc module as built-in.
> We observed a 3-6% speed up on the retpoline CPUs, when going through 1
> tc filter, 

Do yu have all the existing filters enabled at build time in your test
kernel? the reported figures are quite higher then expected considering
there are 7th new unlikely branch in between.

Also it would be nice to have some figure for the last filter in the if
chain. I fear we could have some regressions there even for 'retpoline'
CPUs - given the long if chain - and u32 is AFAIK (not much actually)
still quite used.

Finally, it looks like the filter order in patch 1/3 is quite relevant,
and it looks like you used the lexicographic order, I guess it should
be better to sort them by 'relevance', if someone could provide a
reasonable 'relevance' order. I personally would move ife, ipt and
simple towards the bottom.

Thanks,

Paolo



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

* Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-11-28 15:44 ` [PATCH net-next v2 1/3] net/sched: add retpoline wrapper " Pedro Tammela
  2022-12-01  5:16   ` Jakub Kicinski
@ 2022-12-01 11:10   ` Paolo Abeni
  2022-12-05  4:56   ` Eric Dumazet
  2 siblings, 0 replies; 14+ messages in thread
From: Paolo Abeni @ 2022-12-01 11:10 UTC (permalink / raw)
  To: Pedro Tammela, netdev
  Cc: davem, edumazet, kuba, jhs, xiyou.wangcong, jiri, kuniyu, Pedro Tammela

On Mon, 2022-11-28 at 12:44 -0300, Pedro Tammela wrote:
> On kernels compiled with CONFIG_RETPOLINE and CONFIG_NET_TC_INDIRECT_WRAPPER,
> optimize actions and filters that are compiled as built-ins into a direct call.
> The calls are ordered alphabetically, but new ones should be ideally
> added last.
> 
> On subsequent patches we expose the classifiers and actions functions
> and wire up the wrapper into tc.
> 
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

There is a mismatch between the submitter and the SoB tag. You either
need to add a 

From: Pedro Tammela <pctammela@mojatatu.com>

tag at the beginning of each patch or your mojatatu account to send the
patches (assuming you prefer to retain the mojatatu SoB)

Thanks!

Paolo


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

* Re: [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc
  2022-12-01 11:05 ` [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Paolo Abeni
@ 2022-12-01 12:34   ` Jamal Hadi Salim
  2022-12-04 23:13     ` Pedro Tammela
  0 siblings, 1 reply; 14+ messages in thread
From: Jamal Hadi Salim @ 2022-12-01 12:34 UTC (permalink / raw)
  To: Paolo Abeni
  Cc: Pedro Tammela, netdev, davem, edumazet, kuba, xiyou.wangcong,
	jiri, kuniyu, Pedro Tammela

On Thu, Dec 1, 2022 at 6:05 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2022-11-28 at 12:44 -0300, Pedro Tammela wrote:

[..]

> > We observed a 3-6% speed up on the retpoline CPUs, when going through 1
> > tc filter,
>
> Do yu have all the existing filters enabled at build time in your test
> kernel? the reported figures are quite higher then expected considering
> there are 7th new unlikely branch in between.
>

That can be validated with a test that compiles a kernel with a filter under
test listed first then another kernel with the same filter last.

Also given these tests were using 64B pkts to achieve the highest pps, perhaps
using MTU sized pkts with pktgen would give more realistic results?

In addition to the tests for 1 and 100 filters...

> Also it would be nice to have some figure for the last filter in the if
> chain. I fear we could have some regressions there even for 'retpoline'
> CPUs - given the long if chain - and u32 is AFAIK (not much actually)
> still quite used.
>

I would say flower and bpf + u32 are probably the highest used,
but given no available data on this usage beauty is in the eye of
the beholder. I hope it doesnt become a real estate battle like we
have in which subsystem gets to see packets first or last ;->

> Finally, it looks like the filter order in patch 1/3 is quite relevant,
> and it looks like you used the lexicographic order, I guess it should
> be better to sort them by 'relevance', if someone could provide a
> reasonable 'relevance' order. I personally would move ife, ipt and
> simple towards the bottom.

I think we can come up with some reasonable order.

cheers,
jamal

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

* Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-12-01  5:16   ` Jakub Kicinski
@ 2022-12-01 16:40     ` Pedro Tammela
  2022-12-01 22:38       ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Pedro Tammela @ 2022-12-01 16:40 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri, kuniyu

On 01/12/2022 02:16, Jakub Kicinski wrote:
> On Mon, 28 Nov 2022 12:44:54 -0300 Pedro Tammela wrote:
>> On kernels compiled with CONFIG_RETPOLINE and CONFIG_NET_TC_INDIRECT_WRAPPER,
>> optimize actions and filters that are compiled as built-ins into a direct call.
>> The calls are ordered alphabetically, but new ones should be ideally
>> added last.
>>
>> On subsequent patches we expose the classifiers and actions functions
>> and wire up the wrapper into tc.
> 
>> +#if IS_ENABLED(CONFIG_RETPOLINE) && IS_ENABLED(CONFIG_NET_TC_INDIRECT_WRAPPER)
> 
> The latter 'depends on' former, so just check the latter.
> 
>> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
>> +			   struct tcf_result *res)
>> +{
>> +	if (0) { /* noop */ }
>> +#if IS_BUILTIN(CONFIG_NET_ACT_BPF)
>> +	else if (a->ops->act == tcf_bpf_act)
>> +		return tcf_bpf_act(skb, a, res);
>> +#endif
> 
> How does the 'else if' ladder compare to a switch statement?

It's the semantically the same, we would just need to do some casts to 
unsigned long.
WDYT about the following?

   #define __TC_ACT_BUILTIN(builtin, fname) \
      if (builtin && a->ops->act == fname) return fname(skb, a, res)

   #define _TC_ACT_BUILTIN(builtin, fname) __TC_ACT_BUILTIN(builtin, fname)
   #define TC_ACT_BUILTIN(cfg, fname)  _TC_ACT_BUILTIN(IS_BUILTIN(cfg), 
fname)

   static inline int __tc_act(struct sk_buff *skb, const struct 
tc_action *a,
                              struct tcf_result *res)
   {
           TC_ACT_BUILTIN(CONFIG_NET_ACT_BPF, tcf_bpf_act);
   ...

It might be more pleasant to the reader.

> 
>> +#ifdef CONFIG_NET_CLS_ACT
>> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
>> +			   struct tcf_result *res)
>> +{
>> +	return a->ops->act(skb, a, res);
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_NET_CLS
>> +static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>> +				struct tcf_result *res)
>> +{
>> +	return tp->classify(skb, tp, res);
>> +}
>> +#endif
> 
> please don't wrap the static inline helpers in #ifdefs unless it's
> actually necessary for build to pass.

The only one really needed is CONFIG_NET_CLS_ACT because the struct 
tc_action definition is protected by it. Perhaps we should move it out 
of the #ifdef?

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

* Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-12-01 16:40     ` Pedro Tammela
@ 2022-12-01 22:38       ` Jakub Kicinski
  2022-12-02 12:48         ` Pedro Tammela
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2022-12-01 22:38 UTC (permalink / raw)
  To: Pedro Tammela
  Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri, kuniyu

On Thu, 1 Dec 2022 13:40:34 -0300 Pedro Tammela wrote:
> >> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
> >> +			   struct tcf_result *res)
> >> +{
> >> +	if (0) { /* noop */ }
> >> +#if IS_BUILTIN(CONFIG_NET_ACT_BPF)
> >> +	else if (a->ops->act == tcf_bpf_act)
> >> +		return tcf_bpf_act(skb, a, res);
> >> +#endif  
> > 
> > How does the 'else if' ladder compare to a switch statement?  
> 
> It's the semantically the same, we would just need to do some casts to 
> unsigned long.

Sorry, should've been clearer, I mean in terms of generated code.
Is the machine code identical / better / worse?

> WDYT about the following?
> 
>    #define __TC_ACT_BUILTIN(builtin, fname) \
>       if (builtin && a->ops->act == fname) return fname(skb, a, res)
> 
>    #define _TC_ACT_BUILTIN(builtin, fname) __TC_ACT_BUILTIN(builtin, fname)
>    #define TC_ACT_BUILTIN(cfg, fname)  _TC_ACT_BUILTIN(IS_BUILTIN(cfg), 
> fname)
> 
>    static inline int __tc_act(struct sk_buff *skb, const struct 
> tc_action *a,
>                               struct tcf_result *res)
>    {
>            TC_ACT_BUILTIN(CONFIG_NET_ACT_BPF, tcf_bpf_act);
>    ...
> 
> It might be more pleasant to the reader.

Most definitely not to this reader :) The less macro magic the better.
I'm primarily curious about whether the compiler treats this sort of
construct the same as a switch.

> >> +#ifdef CONFIG_NET_CLS_ACT
> >> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
> >> +			   struct tcf_result *res)
> >> +{
> >> +	return a->ops->act(skb, a, res);
> >> +}
> >> +#endif
> >> +
> >> +#ifdef CONFIG_NET_CLS
> >> +static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
> >> +				struct tcf_result *res)
> >> +{
> >> +	return tp->classify(skb, tp, res);
> >> +}
> >> +#endif  
> > 
> > please don't wrap the static inline helpers in #ifdefs unless it's
> > actually necessary for build to pass.  
> 
> The only one really needed is CONFIG_NET_CLS_ACT because the struct 
> tc_action definition is protected by it. Perhaps we should move it out 
> of the #ifdef?

Yes, I think that's a nice cleanup.

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

* Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-12-01 22:38       ` Jakub Kicinski
@ 2022-12-02 12:48         ` Pedro Tammela
  2022-12-02 18:13           ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Pedro Tammela @ 2022-12-02 12:48 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri, kuniyu

On 01/12/2022 19:38, Jakub Kicinski wrote:
> On Thu, 1 Dec 2022 13:40:34 -0300 Pedro Tammela wrote:
>>>> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
>>>> +			   struct tcf_result *res)
>>>> +{
>>>> +	if (0) { /* noop */ }
>>>> +#if IS_BUILTIN(CONFIG_NET_ACT_BPF)
>>>> +	else if (a->ops->act == tcf_bpf_act)
>>>> +		return tcf_bpf_act(skb, a, res);
>>>> +#endif
>>>
>>> How does the 'else if' ladder compare to a switch statement?
>>
>> It's the semantically the same, we would just need to do some casts to
>> unsigned long.
> 
> Sorry, should've been clearer, I mean in terms of generated code.
> Is the machine code identical / better / worse?
> 
>> WDYT about the following?
>>
>>     #define __TC_ACT_BUILTIN(builtin, fname) \
>>        if (builtin && a->ops->act == fname) return fname(skb, a, res)
>>
>>     #define _TC_ACT_BUILTIN(builtin, fname) __TC_ACT_BUILTIN(builtin, fname)
>>     #define TC_ACT_BUILTIN(cfg, fname)  _TC_ACT_BUILTIN(IS_BUILTIN(cfg),
>> fname)
>>
>>     static inline int __tc_act(struct sk_buff *skb, const struct
>> tc_action *a,
>>                                struct tcf_result *res)
>>     {
>>             TC_ACT_BUILTIN(CONFIG_NET_ACT_BPF, tcf_bpf_act);
>>     ...
>>
>> It might be more pleasant to the reader.
> 
> Most definitely not to this reader :) The less macro magic the better.
> I'm primarily curious about whether the compiler treats this sort of
> construct the same as a switch.

Apparently we can't do a switch case in pointer addresses because the 
case condition must be constant.

> 
>>>> +#ifdef CONFIG_NET_CLS_ACT
>>>> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
>>>> +			   struct tcf_result *res)
>>>> +{
>>>> +	return a->ops->act(skb, a, res);
>>>> +}
>>>> +#endif
>>>> +
>>>> +#ifdef CONFIG_NET_CLS
>>>> +static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>>>> +				struct tcf_result *res)
>>>> +{
>>>> +	return tp->classify(skb, tp, res);
>>>> +}
>>>> +#endif
>>>
>>> please don't wrap the static inline helpers in #ifdefs unless it's
>>> actually necessary for build to pass.
>>
>> The only one really needed is CONFIG_NET_CLS_ACT because the struct
>> tc_action definition is protected by it. Perhaps we should move it out
>> of the #ifdef?
> 
> Yes, I think that's a nice cleanup.


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

* Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-12-02 12:48         ` Pedro Tammela
@ 2022-12-02 18:13           ` Jakub Kicinski
  0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2022-12-02 18:13 UTC (permalink / raw)
  To: Pedro Tammela
  Cc: netdev, davem, edumazet, pabeni, jhs, xiyou.wangcong, jiri, kuniyu

On Fri, 2 Dec 2022 09:48:49 -0300 Pedro Tammela wrote:
> > Most definitely not to this reader :) The less macro magic the better.
> > I'm primarily curious about whether the compiler treats this sort of
> > construct the same as a switch.  
> 
> Apparently we can't do a switch case in pointer addresses because the 
> case condition must be constant.

Ack, thanks for checking!

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

* Re: [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc
  2022-12-01 12:34   ` Jamal Hadi Salim
@ 2022-12-04 23:13     ` Pedro Tammela
  0 siblings, 0 replies; 14+ messages in thread
From: Pedro Tammela @ 2022-12-04 23:13 UTC (permalink / raw)
  To: Jamal Hadi Salim, Paolo Abeni
  Cc: Pedro Tammela, netdev, davem, edumazet, kuba, xiyou.wangcong,
	jiri, kuniyu

On 01/12/2022 09:34, Jamal Hadi Salim wrote:
> On Thu, Dec 1, 2022 at 6:05 AM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>> On Mon, 2022-11-28 at 12:44 -0300, Pedro Tammela wrote:
> 
> [..]
> 
>>> We observed a 3-6% speed up on the retpoline CPUs, when going through 1
>>> tc filter,
>>
>> Do yu have all the existing filters enabled at build time in your test
>> kernel? the reported figures are quite higher then expected considering
>> there are 7th new unlikely branch in between.
>>
> 
> That can be validated with a test that compiles a kernel with a filter under
> test listed first then another kernel with the same filter last.
> 
> Also given these tests were using 64B pkts to achieve the highest pps, perhaps
> using MTU sized pkts with pktgen would give more realistic results?
> 
> In addition to the tests for 1 and 100 filters...
> 
>> Also it would be nice to have some figure for the last filter in the if
>> chain. I fear we could have some regressions there even for 'retpoline'
>> CPUs - given the long if chain - and u32 is AFAIK (not much actually)
>> still quite used.
>>
> 
> I would say flower and bpf + u32 are probably the highest used,
> but given no available data on this usage beauty is in the eye of
> the beholder. I hope it doesnt become a real estate battle like we
> have in which subsystem gets to see packets first or last ;->
> 
>> Finally, it looks like the filter order in patch 1/3 is quite relevant,
>> and it looks like you used the lexicographic order, I guess it should
>> be better to sort them by 'relevance', if someone could provide a
>> reasonable 'relevance' order. I personally would move ife, ipt and
>> simple towards the bottom.
> 
> I think we can come up with some reasonable order.
> 
> cheers,
> jamal

We got a new system with a 7950x and I had some free time today to test 
out the classifier order with v3, which I will post soon.

64b pps:
baseline - 5914980
first - 6397116 (+8.15%)
last - 6362476 (+7.5%)

1500b pps:
baseline - 6367965
first - 6754578 (+6.07%)
last - 6745576 (+5.9%)

The difference between first to last is minimal, but it exists.
DDR5 seems to give a nice boost on pps for this test, when compared to 
the 5950x. Which makes sense, since it's quite heavy on the memory 
allocator.

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

* Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc
  2022-11-28 15:44 ` [PATCH net-next v2 1/3] net/sched: add retpoline wrapper " Pedro Tammela
  2022-12-01  5:16   ` Jakub Kicinski
  2022-12-01 11:10   ` Paolo Abeni
@ 2022-12-05  4:56   ` Eric Dumazet
  2 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2022-12-05  4:56 UTC (permalink / raw)
  To: Pedro Tammela
  Cc: netdev, davem, kuba, pabeni, jhs, xiyou.wangcong, jiri, kuniyu,
	Pedro Tammela

On Mon, Nov 28, 2022 at 4:45 PM Pedro Tammela <pctammela@gmail.com> wrote:
>
> On kernels compiled with CONFIG_RETPOLINE and CONFIG_NET_TC_INDIRECT_WRAPPER,
> optimize actions and filters that are compiled as built-ins into a direct call.
> The calls are ordered alphabetically, but new ones should be ideally
> added last.
>
> On subsequent patches we expose the classifiers and actions functions
> and wire up the wrapper into tc.
>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> ---


> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
> +                          struct tcf_result *res)

Perhaps remove the __ prefix, since there is not yet an  __tc_act() and tc_act()

> +
> +static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
> +                               struct tcf_result *res)

Same remark, the __ prefix seems not necessary.

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

end of thread, other threads:[~2022-12-05  4:57 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-28 15:44 [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Pedro Tammela
2022-11-28 15:44 ` [PATCH net-next v2 1/3] net/sched: add retpoline wrapper " Pedro Tammela
2022-12-01  5:16   ` Jakub Kicinski
2022-12-01 16:40     ` Pedro Tammela
2022-12-01 22:38       ` Jakub Kicinski
2022-12-02 12:48         ` Pedro Tammela
2022-12-02 18:13           ` Jakub Kicinski
2022-12-01 11:10   ` Paolo Abeni
2022-12-05  4:56   ` Eric Dumazet
2022-11-28 15:44 ` [PATCH net-next v2 2/3] net/sched: avoid indirect act functions on retpoline kernels Pedro Tammela
2022-11-28 15:44 ` [PATCH net-next v2 3/3] net/sched: avoid indirect classify " Pedro Tammela
2022-12-01 11:05 ` [PATCH net-next v2 0/3] net/sched: retpoline wrappers for tc Paolo Abeni
2022-12-01 12:34   ` Jamal Hadi Salim
2022-12-04 23:13     ` Pedro Tammela

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).