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

[Gerd Bayer <gbayer@linux.ibm.com>]

On Thu, 2025-01-23 at 15:30 +0800, Dust Li wrote:
> 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.

If I may jump in with a suggestion here:
On my first glance, I'd expect SMC_OPS to offer OPS as a general API.
The description however suggest that this adds "contol points" or hooks
in the SMC code, that eBPF programs can use to tweak the protocol's
behavior. Exclusively eBPF programs, it seems.

So how about naming this SMC_EBPF_HOOKS or SMC_EBPF_SUPPORT?

Just my 2ct,
Gerd

> 
> 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
> 
>