Re: [PATCH bpf-next v7 3/6] net/smc: Introduce generic hook smc_ops

[Dust Li <dust.li@linux.alibaba.com>]

On 2025-01-23 09:59:39, D. Wythe wrote:
>The introduction of IPPROTO_SMC enables eBPF programs to determine
>whether to use SMC based on the context of socket creation, such as
>network namespaces, PID and comm name, etc.
>
>As a subsequent enhancement, to introduce a new generic hook that
>allows decisions on whether to use SMC or not at runtime, including
>but not limited to local/remote IP address or ports.
>
>Moreover, in the future, we can achieve more complex extensions to the
>protocol stack by extending this ops.
>
>Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>---
> include/net/netns/smc.h |  3 ++
> include/net/smc.h       | 53 ++++++++++++++++++++++++
> net/ipv4/tcp_output.c   | 18 +++++++--
> net/smc/Kconfig         | 12 ++++++
> net/smc/Makefile        |  1 +
> net/smc/smc_ops.c       | 53 ++++++++++++++++++++++++
> net/smc/smc_ops.h       | 28 +++++++++++++
> net/smc/smc_sysctl.c    | 90 +++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 254 insertions(+), 4 deletions(-)
> create mode 100644 net/smc/smc_ops.c
> create mode 100644 net/smc/smc_ops.h
>
>diff --git a/include/net/netns/smc.h b/include/net/netns/smc.h
>index fc752a50f91b..81b3fdb39cd2 100644
>--- a/include/net/netns/smc.h
>+++ b/include/net/netns/smc.h
>@@ -17,6 +17,9 @@ struct netns_smc {
> #ifdef CONFIG_SYSCTL
> 	struct ctl_table_header		*smc_hdr;
> #endif
>+#if IS_ENABLED(CONFIG_SMC_OPS)
>+	struct smc_ops __rcu		*ops;
>+#endif /* CONFIG_SMC_OPS */
> 	unsigned int			sysctl_autocorking_size;
> 	unsigned int			sysctl_smcr_buf_type;
> 	int				sysctl_smcr_testlink_time;
>diff --git a/include/net/smc.h b/include/net/smc.h
>index db84e4e35080..844f98a6296a 100644
>--- a/include/net/smc.h
>+++ b/include/net/smc.h
>@@ -18,6 +18,8 @@
> #include "linux/ism.h"
> 
> struct sock;
>+struct tcp_sock;
>+struct inet_request_sock;
> 
> #define SMC_MAX_PNETID_LEN	16	/* Max. length of PNET id */
> 
>@@ -97,4 +99,55 @@ struct smcd_dev {
> 	u8 going_away : 1;
> };
> 
>+#define  SMC_OPS_NAME_MAX 16
>+
>+enum {
>+	/* ops can be inherit from init_net */
>+	SMC_OPS_FLAG_INHERITABLE = 0x1,
>+
>+	SMC_OPS_ALL_FLAGS = SMC_OPS_FLAG_INHERITABLE,
>+};
>+
>+struct smc_ops {
>+	/* priavte */
>+
>+	struct list_head list;
>+	struct module *owner;
>+
>+	/* public */
>+
>+	/* unique name */
>+	char name[SMC_OPS_NAME_MAX];
>+	int flags;
>+
>+	/* Invoked before computing SMC option for SYN packets.
>+	 * We can control whether to set SMC options by returning varios value.
>+	 * Return 0 to disable SMC, or return any other value to enable it.
>+	 */
>+	int (*set_option)(struct tcp_sock *tp);
>+
>+	/* Invoked before Set up SMC options for SYN-ACK packets
>+	 * We can control whether to respond SMC options by returning varios
>+	 * value. Return 0 to disable SMC, or return any other value to enable
>+	 * it.
>+	 */
>+	int (*set_option_cond)(const struct tcp_sock *tp,
>+			       struct inet_request_sock *ireq);
>+};
>+
>+#if IS_ENABLED(CONFIG_SMC_OPS)
>+#define smc_call_retops(init_val, sk, func, ...) ({	\
>+	typeof(init_val) __ret = (init_val);		\
>+	struct smc_ops *ops;				\
>+	rcu_read_lock();				\
>+	ops = READ_ONCE(sock_net(sk)->smc.ops);		\
>+	if (ops && ops->func)				\
>+		__ret = ops->func(__VA_ARGS__);		\
>+	rcu_read_unlock();				\
>+	__ret;						\
>+})
>+#else
>+#define smc_call_retops(init_val, ...) (init_val)
>+#endif /* CONFIG_SMC_OPS */
>+
> #endif	/* _SMC_H */
>diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>index 0e5b9a654254..f62e30b4ffc8 100644
>--- a/net/ipv4/tcp_output.c
>+++ b/net/ipv4/tcp_output.c
>@@ -40,6 +40,7 @@
> #include <net/tcp.h>
> #include <net/mptcp.h>
> #include <net/proto_memory.h>
>+#include <net/smc.h>
> 
> #include <linux/compiler.h>
> #include <linux/gfp.h>
>@@ -759,14 +760,18 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> 	mptcp_options_write(th, ptr, tp, opts);
> }
> 
>-static void smc_set_option(const struct tcp_sock *tp,
>+static void smc_set_option(struct tcp_sock *tp,
> 			   struct tcp_out_options *opts,
> 			   unsigned int *remaining)
> {
> #if IS_ENABLED(CONFIG_SMC)
>+	struct sock *sk = &tp->inet_conn.icsk_inet.sk;
> 	if (static_branch_unlikely(&tcp_have_smc)) {
> 		if (tp->syn_smc) {
>-			if (*remaining >= TCPOLEN_EXP_SMC_BASE_ALIGNED) {
>+			tp->syn_smc = !!smc_call_retops(1, sk, set_option, tp);
>+			/* re-check syn_smc */
>+			if (tp->syn_smc &&
>+			    *remaining >= TCPOLEN_EXP_SMC_BASE_ALIGNED) {
> 				opts->options |= OPTION_SMC;
> 				*remaining -= TCPOLEN_EXP_SMC_BASE_ALIGNED;
> 			}
>@@ -776,14 +781,19 @@ static void smc_set_option(const struct tcp_sock *tp,
> }
> 
> static void smc_set_option_cond(const struct tcp_sock *tp,
>-				const struct inet_request_sock *ireq,
>+				struct inet_request_sock *ireq,
> 				struct tcp_out_options *opts,
> 				unsigned int *remaining)
> {
> #if IS_ENABLED(CONFIG_SMC)
>+	const struct sock *sk = &tp->inet_conn.icsk_inet.sk;
> 	if (static_branch_unlikely(&tcp_have_smc)) {
> 		if (tp->syn_smc && ireq->smc_ok) {
>-			if (*remaining >= TCPOLEN_EXP_SMC_BASE_ALIGNED) {
>+			ireq->smc_ok = !!smc_call_retops(1, sk, set_option_cond,
>+							 tp, ireq);
>+			/* re-check smc_ok */
>+			if (ireq->smc_ok &&
>+			    *remaining >= TCPOLEN_EXP_SMC_BASE_ALIGNED) {
> 				opts->options |= OPTION_SMC;
> 				*remaining -= TCPOLEN_EXP_SMC_BASE_ALIGNED;
> 			}
>diff --git a/net/smc/Kconfig b/net/smc/Kconfig
>index ba5e6a2dd2fd..27f35064d04c 100644
>--- a/net/smc/Kconfig
>+++ b/net/smc/Kconfig
>@@ -33,3 +33,15 @@ config SMC_LO
> 	  of architecture or hardware.
> 
> 	  if unsure, say N.
>+
>+config SMC_OPS
>+	bool "Generic hook for SMC subsystem"
>+	depends on SMC && BPF_SYSCALL
>+	default n
>+	help
>+	  SMC_OPS enables support to register generic hook via eBPF programs
>+	  for SMC subsystem. eBPF programs offer much greater flexibility
>+	  in modifying the behavior of the SMC protocol stack compared
>+	  to a complete kernel-based approach.
>+
>+	  if unsure, say N.

I'm still not completely satisfied with the name smc_ops. Since this
will be the API for our users, we need to be carefull on the name.

It seems like you're aiming to define a common set of operations, but
the implementation appears to be intertwined with BPF. If this is
intended to be a common interface, and if we are using another operation,
there shouldn’t be a need to hold a BPF reference.

As your 'help' sugguest, What about smc_hook ?

Best regards,
Dust