[PATCH bpf-next v1 0/5] net/smc: Introduce BPF injection capability

[PATCH bpf-next v1 0/5] net/smc: Introduce BPF injection capability

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

This patches attempt to introduce BPF injection capability for SMC,
and add selftest to ensure code stability.

As we all know that the SMC protocol is not suitable for all scenarios,
especially for short-lived. However, for most applications, they cannot
guarantee that there are no such scenarios at all. Therefore, apps
may need some specific strategies to decide shall we need to use SMC
or not, for example, apps can limit the scope of the SMC to a specific
IP address or port.

Based on the consideration of transparent replacement, we hope that apps
can remain transparent even if they need to formulate some specific
strategies for SMC using. That is, do not need to recompile their code.

On the other hand, we need to ensure the scalability of strategies
implementation. Although it is simple to use socket options or sysctl,
it will bring more complexity to subsequent expansion.

Fortunately, BPF can solve these concerns very well, users can write
thire own strategies in eBPF to choose whether to use SMC or not.
And it's quite easy for them to modify their strategies in the future.

This patches implement injection capability for SMC via struct_ops.
In that way, we can add new injection scenarios in the future.

v1:

1. split bpf_smc.c 
2. remove unnecessary symbol exports

D. Wythe (5):
  net/smc: move smc_sock related structure definition
  net/smc: allow smc to negotiate protocols on policies
  net/smc: allow set or get smc negotiator by sockopt
  bpf: add smc negotiator support in BPF struct_ops
  bpf/selftests: add selftest for SMC bpf capability

 include/net/smc.h                                | 257 ++++++++++++++++++++++
 include/uapi/linux/smc.h                         |   1 +
 kernel/bpf/bpf_struct_ops_types.h                |   4 +
 net/Makefile                                     |   1 +
 net/smc/Kconfig                                  |  11 +
 net/smc/af_smc.c                                 | 265 ++++++++++++++++++++---
 net/smc/bpf_smc.c                                | 171 +++++++++++++++
 net/smc/smc.h                                    | 224 -------------------
 net/smc/smc_negotiator.c                         | 119 ++++++++++
 net/smc/smc_negotiator.h                         | 116 ++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_smc.c | 107 +++++++++
 tools/testing/selftests/bpf/progs/bpf_smc.c      | 265 +++++++++++++++++++++++
 12 files changed, 1282 insertions(+), 259 deletions(-)
 create mode 100644 net/smc/bpf_smc.c
 create mode 100644 net/smc/smc_negotiator.c
 create mode 100644 net/smc/smc_negotiator.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_smc.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_smc.c

-- 
1.8.3.1

[PATCH bpf-next v1 1/5] net/smc: move smc_sock related structure definition

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

This patch only try to move the definition of smc_sock and its
related structure, from net/smc/smc.h to include/net/smc/smc.h.
In that way can ebpf generate the BTF ID corresponding to our
structure.

Of course, we can also choose to hide the structure and only to
expose an intermediate structure, but it requires an additional
transformation. If we need to obtain some information frequently, this
may cause some performance problems.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 include/net/smc.h | 225 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/smc/smc.h     | 224 -----------------------------------------------------
 2 files changed, 225 insertions(+), 224 deletions(-)

diff --git a/include/net/smc.h b/include/net/smc.h
index a002552..6d076f5 100644
--- a/include/net/smc.h
+++ b/include/net/smc.h
@@ -11,12 +11,17 @@
 #ifndef _SMC_H
 #define _SMC_H
 
+#include <net/inet_connection_sock.h>
 #include <linux/device.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/wait.h>
 #include "linux/ism.h"
 
+#ifdef ATOMIC64_INIT
+#define KERNEL_HAS_ATOMIC64
+#endif
+
 struct sock;
 
 #define SMC_MAX_PNETID_LEN	16	/* Max. length of PNET id */
@@ -91,4 +96,224 @@ struct smcd_dev {
 	u8 going_away : 1;
 };
 
+struct smc_wr_rx_hdr {	/* common prefix part of LLC and CDC to demultiplex */
+	union {
+		u8 type;
+#if defined(__BIG_ENDIAN_BITFIELD)
+		struct {
+			u8 llc_version:4,
+			   llc_type:4;
+		};
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+		struct {
+			u8 llc_type:4,
+			   llc_version:4;
+		};
+#endif
+	};
+} __aligned(1);
+
+struct smc_cdc_conn_state_flags {
+#if defined(__BIG_ENDIAN_BITFIELD)
+	u8	peer_done_writing : 1;	/* Sending done indicator */
+	u8	peer_conn_closed : 1;	/* Peer connection closed indicator */
+	u8	peer_conn_abort : 1;	/* Abnormal close indicator */
+	u8	reserved : 5;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+	u8	reserved : 5;
+	u8	peer_conn_abort : 1;
+	u8	peer_conn_closed : 1;
+	u8	peer_done_writing : 1;
+#endif
+};
+
+struct smc_cdc_producer_flags {
+#if defined(__BIG_ENDIAN_BITFIELD)
+	u8	write_blocked : 1;	/* Writing Blocked, no rx buf space */
+	u8	urg_data_pending : 1;	/* Urgent Data Pending */
+	u8	urg_data_present : 1;	/* Urgent Data Present */
+	u8	cons_curs_upd_req : 1;	/* cursor update requested */
+	u8	failover_validation : 1;/* message replay due to failover */
+	u8	reserved : 3;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+	u8	reserved : 3;
+	u8	failover_validation : 1;
+	u8	cons_curs_upd_req : 1;
+	u8	urg_data_present : 1;
+	u8	urg_data_pending : 1;
+	u8	write_blocked : 1;
+#endif
+};
+
+/* in host byte order */
+union smc_host_cursor {	/* SMC cursor - an offset in an RMBE */
+	struct {
+		u16	reserved;
+		u16	wrap;		/* window wrap sequence number */
+		u32	count;		/* cursor (= offset) part */
+	};
+#ifdef KERNEL_HAS_ATOMIC64
+	atomic64_t		acurs;	/* for atomic processing */
+#else
+	u64			acurs;	/* for atomic processing */
+#endif
+} __aligned(8);
+
+/* in host byte order, except for flag bitfields in network byte order */
+struct smc_host_cdc_msg {		/* Connection Data Control message */
+	struct smc_wr_rx_hdr		common; /* .type = 0xFE */
+	u8				len;	/* length = 44 */
+	u16				seqno;	/* connection seq # */
+	u32				token;	/* alert_token */
+	union smc_host_cursor		prod;		/* producer cursor */
+	union smc_host_cursor		cons;		/* consumer cursor,
+							 * piggy backed "ack"
+							 */
+	struct smc_cdc_producer_flags	prod_flags;	/* conn. tx/rx status */
+	struct smc_cdc_conn_state_flags	conn_state_flags; /* peer conn. status*/
+	u8				reserved[18];
+} __aligned(8);
+
+enum smc_urg_state {
+	SMC_URG_VALID	= 1,			/* data present */
+	SMC_URG_NOTYET	= 2,			/* data pending */
+	SMC_URG_READ	= 3,			/* data was already read */
+};
+
+struct smc_connection {
+	struct rb_node		alert_node;
+	struct smc_link_group	*lgr;		/* link group of connection */
+	struct smc_link		*lnk;		/* assigned SMC-R link */
+	u32			alert_token_local; /* unique conn. id */
+	u8			peer_rmbe_idx;	/* from tcp handshake */
+	int			peer_rmbe_size;	/* size of peer rx buffer */
+	atomic_t		peer_rmbe_space;/* remaining free bytes in peer
+						 * rmbe
+						 */
+	int			rtoken_idx;	/* idx to peer RMB rkey/addr */
+
+	struct smc_buf_desc	*sndbuf_desc;	/* send buffer descriptor */
+	struct smc_buf_desc	*rmb_desc;	/* RMBE descriptor */
+	int			rmbe_size_short;/* compressed notation */
+	int			rmbe_update_limit;
+						/* lower limit for consumer
+						 * cursor update
+						 */
+
+	struct smc_host_cdc_msg	local_tx_ctrl;	/* host byte order staging
+						 * buffer for CDC msg send
+						 * .prod cf. TCP snd_nxt
+						 * .cons cf. TCP sends ack
+						 */
+	union smc_host_cursor	local_tx_ctrl_fin;
+						/* prod crsr - confirmed by peer
+						 */
+	union smc_host_cursor	tx_curs_prep;	/* tx - prepared data
+						 * snd_max..wmem_alloc
+						 */
+	union smc_host_cursor	tx_curs_sent;	/* tx - sent data
+						 * snd_nxt ?
+						 */
+	union smc_host_cursor	tx_curs_fin;	/* tx - confirmed by peer
+						 * snd-wnd-begin ?
+						 */
+	atomic_t		sndbuf_space;	/* remaining space in sndbuf */
+	u16			tx_cdc_seq;	/* sequence # for CDC send */
+	u16			tx_cdc_seq_fin;	/* sequence # - tx completed */
+	spinlock_t		send_lock;	/* protect wr_sends */
+	atomic_t		cdc_pend_tx_wr; /* number of pending tx CDC wqe
+						 * - inc when post wqe,
+						 * - dec on polled tx cqe
+						 */
+	wait_queue_head_t	cdc_pend_tx_wq; /* wakeup on no cdc_pend_tx_wr*/
+	atomic_t		tx_pushing;     /* nr_threads trying tx push */
+	struct delayed_work	tx_work;	/* retry of smc_cdc_msg_send */
+	u32			tx_off;		/* base offset in peer rmb */
+
+	struct smc_host_cdc_msg	local_rx_ctrl;	/* filled during event_handl.
+						 * .prod cf. TCP rcv_nxt
+						 * .cons cf. TCP snd_una
+						 */
+	union smc_host_cursor	rx_curs_confirmed; /* confirmed to peer
+						    * source of snd_una ?
+						    */
+	union smc_host_cursor	urg_curs;	/* points at urgent byte */
+	enum smc_urg_state	urg_state;
+	bool			urg_tx_pend;	/* urgent data staged */
+	bool			urg_rx_skip_pend;
+						/* indicate urgent oob data
+						 * read, but previous regular
+						 * data still pending
+						 */
+	char			urg_rx_byte;	/* urgent byte */
+	bool			tx_in_release_sock;
+						/* flush pending tx data in
+						 * sock release_cb()
+						 */
+	atomic_t		bytes_to_rcv;	/* arrived data,
+						 * not yet received
+						 */
+	atomic_t		splice_pending;	/* number of spliced bytes
+						 * pending processing
+						 */
+#ifndef KERNEL_HAS_ATOMIC64
+	spinlock_t		acurs_lock;	/* protect cursors */
+#endif
+	struct work_struct	close_work;	/* peer sent some closing */
+	struct work_struct	abort_work;	/* abort the connection */
+	struct tasklet_struct	rx_tsklet;	/* Receiver tasklet for SMC-D */
+	u8			rx_off;		/* receive offset:
+						 * 0 for SMC-R, 32 for SMC-D
+						 */
+	u64			peer_token;	/* SMC-D token of peer */
+	u8			killed : 1;	/* abnormal termination */
+	u8			freed : 1;	/* normal termiation */
+	u8			out_of_sync : 1; /* out of sync with peer */
+};
+
+struct smc_sock {				/* smc sock container */
+	struct sock		sk;
+	struct socket		*clcsock;	/* internal tcp socket */
+	void			(*clcsk_state_change)(struct sock *sk);
+						/* original stat_change fct. */
+	void			(*clcsk_data_ready)(struct sock *sk);
+						/* original data_ready fct. */
+	void			(*clcsk_write_space)(struct sock *sk);
+						/* original write_space fct. */
+	void			(*clcsk_error_report)(struct sock *sk);
+						/* original error_report fct. */
+	struct smc_connection	conn;		/* smc connection */
+	struct smc_sock		*listen_smc;	/* listen parent */
+	struct work_struct	connect_work;	/* handle non-blocking connect*/
+	struct work_struct	tcp_listen_work;/* handle tcp socket accepts */
+	struct work_struct	smc_listen_work;/* prepare new accept socket */
+	struct list_head	accept_q;	/* sockets to be accepted */
+	spinlock_t		accept_q_lock;	/* protects accept_q */
+	bool			limit_smc_hs;	/* put constraint on handshake */
+	bool			use_fallback;	/* fallback to tcp */
+	int			fallback_rsn;	/* reason for fallback */
+	u32			peer_diagnosis; /* decline reason from peer */
+	atomic_t                queued_smc_hs;  /* queued smc handshakes */
+	struct inet_connection_sock_af_ops		af_ops;
+	const struct inet_connection_sock_af_ops	*ori_af_ops;
+						/* original af ops */
+	int			sockopt_defer_accept;
+						/* sockopt TCP_DEFER_ACCEPT
+						 * value
+						 */
+	u8			wait_close_tx_prepared : 1;
+						/* shutdown wr or close
+						 * started, waiting for unsent
+						 * data to be sent
+						 */
+	u8			connect_nonblock : 1;
+						/* non-blocking connect in
+						 * flight
+						 */
+	struct mutex            clcsock_release_lock;
+						/* protects clcsock of a listen
+						 * socket
+						 */
+};
+
 #endif	/* _SMC_H */
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 2eeea4c..55ae8883 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -34,10 +34,6 @@
 extern struct proto smc_proto;
 extern struct proto smc_proto6;
 
-#ifdef ATOMIC64_INIT
-#define KERNEL_HAS_ATOMIC64
-#endif
-
 enum smc_state {		/* possible states of an SMC socket */
 	SMC_ACTIVE	= 1,
 	SMC_INIT	= 2,
@@ -57,232 +53,12 @@ enum smc_state {		/* possible states of an SMC socket */
 
 struct smc_link_group;
 
-struct smc_wr_rx_hdr {	/* common prefix part of LLC and CDC to demultiplex */
-	union {
-		u8 type;
-#if defined(__BIG_ENDIAN_BITFIELD)
-		struct {
-			u8 llc_version:4,
-			   llc_type:4;
-		};
-#elif defined(__LITTLE_ENDIAN_BITFIELD)
-		struct {
-			u8 llc_type:4,
-			   llc_version:4;
-		};
-#endif
-	};
-} __aligned(1);
-
-struct smc_cdc_conn_state_flags {
-#if defined(__BIG_ENDIAN_BITFIELD)
-	u8	peer_done_writing : 1;	/* Sending done indicator */
-	u8	peer_conn_closed : 1;	/* Peer connection closed indicator */
-	u8	peer_conn_abort : 1;	/* Abnormal close indicator */
-	u8	reserved : 5;
-#elif defined(__LITTLE_ENDIAN_BITFIELD)
-	u8	reserved : 5;
-	u8	peer_conn_abort : 1;
-	u8	peer_conn_closed : 1;
-	u8	peer_done_writing : 1;
-#endif
-};
-
-struct smc_cdc_producer_flags {
-#if defined(__BIG_ENDIAN_BITFIELD)
-	u8	write_blocked : 1;	/* Writing Blocked, no rx buf space */
-	u8	urg_data_pending : 1;	/* Urgent Data Pending */
-	u8	urg_data_present : 1;	/* Urgent Data Present */
-	u8	cons_curs_upd_req : 1;	/* cursor update requested */
-	u8	failover_validation : 1;/* message replay due to failover */
-	u8	reserved : 3;
-#elif defined(__LITTLE_ENDIAN_BITFIELD)
-	u8	reserved : 3;
-	u8	failover_validation : 1;
-	u8	cons_curs_upd_req : 1;
-	u8	urg_data_present : 1;
-	u8	urg_data_pending : 1;
-	u8	write_blocked : 1;
-#endif
-};
-
-/* in host byte order */
-union smc_host_cursor {	/* SMC cursor - an offset in an RMBE */
-	struct {
-		u16	reserved;
-		u16	wrap;		/* window wrap sequence number */
-		u32	count;		/* cursor (= offset) part */
-	};
-#ifdef KERNEL_HAS_ATOMIC64
-	atomic64_t		acurs;	/* for atomic processing */
-#else
-	u64			acurs;	/* for atomic processing */
-#endif
-} __aligned(8);
-
-/* in host byte order, except for flag bitfields in network byte order */
-struct smc_host_cdc_msg {		/* Connection Data Control message */
-	struct smc_wr_rx_hdr		common; /* .type = 0xFE */
-	u8				len;	/* length = 44 */
-	u16				seqno;	/* connection seq # */
-	u32				token;	/* alert_token */
-	union smc_host_cursor		prod;		/* producer cursor */
-	union smc_host_cursor		cons;		/* consumer cursor,
-							 * piggy backed "ack"
-							 */
-	struct smc_cdc_producer_flags	prod_flags;	/* conn. tx/rx status */
-	struct smc_cdc_conn_state_flags	conn_state_flags; /* peer conn. status*/
-	u8				reserved[18];
-} __aligned(8);
-
-enum smc_urg_state {
-	SMC_URG_VALID	= 1,			/* data present */
-	SMC_URG_NOTYET	= 2,			/* data pending */
-	SMC_URG_READ	= 3,			/* data was already read */
-};
-
 struct smc_mark_woken {
 	bool woken;
 	void *key;
 	wait_queue_entry_t wait_entry;
 };
 
-struct smc_connection {
-	struct rb_node		alert_node;
-	struct smc_link_group	*lgr;		/* link group of connection */
-	struct smc_link		*lnk;		/* assigned SMC-R link */
-	u32			alert_token_local; /* unique conn. id */
-	u8			peer_rmbe_idx;	/* from tcp handshake */
-	int			peer_rmbe_size;	/* size of peer rx buffer */
-	atomic_t		peer_rmbe_space;/* remaining free bytes in peer
-						 * rmbe
-						 */
-	int			rtoken_idx;	/* idx to peer RMB rkey/addr */
-
-	struct smc_buf_desc	*sndbuf_desc;	/* send buffer descriptor */
-	struct smc_buf_desc	*rmb_desc;	/* RMBE descriptor */
-	int			rmbe_size_short;/* compressed notation */
-	int			rmbe_update_limit;
-						/* lower limit for consumer
-						 * cursor update
-						 */
-
-	struct smc_host_cdc_msg	local_tx_ctrl;	/* host byte order staging
-						 * buffer for CDC msg send
-						 * .prod cf. TCP snd_nxt
-						 * .cons cf. TCP sends ack
-						 */
-	union smc_host_cursor	local_tx_ctrl_fin;
-						/* prod crsr - confirmed by peer
-						 */
-	union smc_host_cursor	tx_curs_prep;	/* tx - prepared data
-						 * snd_max..wmem_alloc
-						 */
-	union smc_host_cursor	tx_curs_sent;	/* tx - sent data
-						 * snd_nxt ?
-						 */
-	union smc_host_cursor	tx_curs_fin;	/* tx - confirmed by peer
-						 * snd-wnd-begin ?
-						 */
-	atomic_t		sndbuf_space;	/* remaining space in sndbuf */
-	u16			tx_cdc_seq;	/* sequence # for CDC send */
-	u16			tx_cdc_seq_fin;	/* sequence # - tx completed */
-	spinlock_t		send_lock;	/* protect wr_sends */
-	atomic_t		cdc_pend_tx_wr; /* number of pending tx CDC wqe
-						 * - inc when post wqe,
-						 * - dec on polled tx cqe
-						 */
-	wait_queue_head_t	cdc_pend_tx_wq; /* wakeup on no cdc_pend_tx_wr*/
-	atomic_t		tx_pushing;     /* nr_threads trying tx push */
-	struct delayed_work	tx_work;	/* retry of smc_cdc_msg_send */
-	u32			tx_off;		/* base offset in peer rmb */
-
-	struct smc_host_cdc_msg	local_rx_ctrl;	/* filled during event_handl.
-						 * .prod cf. TCP rcv_nxt
-						 * .cons cf. TCP snd_una
-						 */
-	union smc_host_cursor	rx_curs_confirmed; /* confirmed to peer
-						    * source of snd_una ?
-						    */
-	union smc_host_cursor	urg_curs;	/* points at urgent byte */
-	enum smc_urg_state	urg_state;
-	bool			urg_tx_pend;	/* urgent data staged */
-	bool			urg_rx_skip_pend;
-						/* indicate urgent oob data
-						 * read, but previous regular
-						 * data still pending
-						 */
-	char			urg_rx_byte;	/* urgent byte */
-	bool			tx_in_release_sock;
-						/* flush pending tx data in
-						 * sock release_cb()
-						 */
-	atomic_t		bytes_to_rcv;	/* arrived data,
-						 * not yet received
-						 */
-	atomic_t		splice_pending;	/* number of spliced bytes
-						 * pending processing
-						 */
-#ifndef KERNEL_HAS_ATOMIC64
-	spinlock_t		acurs_lock;	/* protect cursors */
-#endif
-	struct work_struct	close_work;	/* peer sent some closing */
-	struct work_struct	abort_work;	/* abort the connection */
-	struct tasklet_struct	rx_tsklet;	/* Receiver tasklet for SMC-D */
-	u8			rx_off;		/* receive offset:
-						 * 0 for SMC-R, 32 for SMC-D
-						 */
-	u64			peer_token;	/* SMC-D token of peer */
-	u8			killed : 1;	/* abnormal termination */
-	u8			freed : 1;	/* normal termiation */
-	u8			out_of_sync : 1; /* out of sync with peer */
-};
-
-struct smc_sock {				/* smc sock container */
-	struct sock		sk;
-	struct socket		*clcsock;	/* internal tcp socket */
-	void			(*clcsk_state_change)(struct sock *sk);
-						/* original stat_change fct. */
-	void			(*clcsk_data_ready)(struct sock *sk);
-						/* original data_ready fct. */
-	void			(*clcsk_write_space)(struct sock *sk);
-						/* original write_space fct. */
-	void			(*clcsk_error_report)(struct sock *sk);
-						/* original error_report fct. */
-	struct smc_connection	conn;		/* smc connection */
-	struct smc_sock		*listen_smc;	/* listen parent */
-	struct work_struct	connect_work;	/* handle non-blocking connect*/
-	struct work_struct	tcp_listen_work;/* handle tcp socket accepts */
-	struct work_struct	smc_listen_work;/* prepare new accept socket */
-	struct list_head	accept_q;	/* sockets to be accepted */
-	spinlock_t		accept_q_lock;	/* protects accept_q */
-	bool			limit_smc_hs;	/* put constraint on handshake */
-	bool			use_fallback;	/* fallback to tcp */
-	int			fallback_rsn;	/* reason for fallback */
-	u32			peer_diagnosis; /* decline reason from peer */
-	atomic_t                queued_smc_hs;  /* queued smc handshakes */
-	struct inet_connection_sock_af_ops		af_ops;
-	const struct inet_connection_sock_af_ops	*ori_af_ops;
-						/* original af ops */
-	int			sockopt_defer_accept;
-						/* sockopt TCP_DEFER_ACCEPT
-						 * value
-						 */
-	u8			wait_close_tx_prepared : 1;
-						/* shutdown wr or close
-						 * started, waiting for unsent
-						 * data to be sent
-						 */
-	u8			connect_nonblock : 1;
-						/* non-blocking connect in
-						 * flight
-						 */
-	struct mutex            clcsock_release_lock;
-						/* protects clcsock of a listen
-						 * socket
-						 * */
-};
-
 #define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
 
 static inline void smc_init_saved_callbacks(struct smc_sock *smc)
-- 
1.8.3.1

[PATCH bpf-next v1 2/5] net/smc: allow smc to negotiate protocols on policies

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

As we all know, the SMC protocol is not suitable for all scenarios,
especially for short-lived. However, for most applications, they cannot
guarantee that there are no such scenarios at all. Therefore, apps
may need some specific strategies to decide shall we need to use SMC
or not.

Just like the congestion control implementation in TCP, this patch
provides a generic negotiator implementation. If necessary,
we can provide different protocol negotiation strategies for
apps based on this implementation.

But most importantly, this patch provides the possibility of
eBPF injection, allowing users to implement their own protocol
negotiation policy in userspace.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 include/net/smc.h        |  32 +++++++++++
 net/Makefile             |   1 +
 net/smc/Kconfig          |  11 ++++
 net/smc/af_smc.c         | 134 ++++++++++++++++++++++++++++++++++++++++++++++-
 net/smc/smc_negotiator.c | 119 +++++++++++++++++++++++++++++++++++++++++
 net/smc/smc_negotiator.h | 116 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 412 insertions(+), 1 deletion(-)
 create mode 100644 net/smc/smc_negotiator.c
 create mode 100644 net/smc/smc_negotiator.h

diff --git a/include/net/smc.h b/include/net/smc.h
index 6d076f5..191061c 100644
--- a/include/net/smc.h
+++ b/include/net/smc.h
@@ -296,6 +296,8 @@ struct smc_sock {				/* smc sock container */
 	atomic_t                queued_smc_hs;  /* queued smc handshakes */
 	struct inet_connection_sock_af_ops		af_ops;
 	const struct inet_connection_sock_af_ops	*ori_af_ops;
+	/* protocol negotiator ops */
+	const struct smc_sock_negotiator_ops *negotiator_ops;
 						/* original af ops */
 	int			sockopt_defer_accept;
 						/* sockopt TCP_DEFER_ACCEPT
@@ -316,4 +318,34 @@ struct smc_sock {				/* smc sock container */
 						 */
 };
 
+#ifdef CONFIG_SMC_BPF
+/* BPF struct ops for smc protocol negotiator */
+struct smc_sock_negotiator_ops {
+
+	struct list_head	list;
+
+	/* ops name */
+	char		name[16];
+	/* key for name */
+	u32			key;
+
+	/* init with sk */
+	void (*init)(struct sock *sk);
+
+	/* release with sk */
+	void (*release)(struct sock *sk);
+
+	/* advice for negotiate */
+	int (*negotiate)(struct sock *sk);
+
+	/* info gathering timing */
+	void (*collect_info)(struct sock *sk, int timing);
+
+	/* module owner */
+	struct module *owner;
+};
+#else
+struct smc_sock_negotiator_ops {};
+#endif
+
 #endif	/* _SMC_H */
diff --git a/net/Makefile b/net/Makefile
index 4c4dc53..222916a 100644
--- a/net/Makefile
+++ b/net/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_TIPC)		+= tipc/
 obj-$(CONFIG_NETLABEL)		+= netlabel/
 obj-$(CONFIG_IUCV)		+= iucv/
 obj-$(CONFIG_SMC)		+= smc/
+obj-$(CONFIG_SMC_BPF)		+= smc/smc_negotiator.o
 obj-$(CONFIG_RFKILL)		+= rfkill/
 obj-$(CONFIG_NET_9P)		+= 9p/
 obj-$(CONFIG_CAIF)		+= caif/
diff --git a/net/smc/Kconfig b/net/smc/Kconfig
index 1ab3c5a..bdcc9f1 100644
--- a/net/smc/Kconfig
+++ b/net/smc/Kconfig
@@ -19,3 +19,14 @@ config SMC_DIAG
 	  smcss.
 
 	  if unsure, say Y.
+
+config SMC_BPF
+	bool "SMC: support eBPF" if SMC
+	depends on BPF_SYSCALL
+	default n
+	help
+	  Supports eBPF to allows user mode participation in SMC's protocol process
+	  via ebpf programs. Alternatively, obtain information about the SMC socks
+	  through the ebpf program.
+
+	  If unsure, say N.
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 50c38b6..7406fd4 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -52,6 +52,7 @@
 #include "smc_close.h"
 #include "smc_stats.h"
 #include "smc_tracepoint.h"
+#include "smc_negotiator.h"
 #include "smc_sysctl.h"
 
 static DEFINE_MUTEX(smc_server_lgr_pending);	/* serialize link group
@@ -68,6 +69,119 @@
 static void smc_tcp_listen_work(struct work_struct *);
 static void smc_connect_work(struct work_struct *);
 
+#ifdef CONFIG_SMC_BPF
+
+/* Check if sock should use smc */
+int smc_sock_should_select_smc(const struct smc_sock *smc)
+{
+	const struct smc_sock_negotiator_ops *ops;
+	int ret;
+
+	rcu_read_lock();
+	ops = READ_ONCE(smc->negotiator_ops);
+
+	/* No negotiator_ops supply or no negotiate func set,
+	 * always pass it.
+	 */
+	if (!ops || !ops->negotiate) {
+		rcu_read_unlock();
+		return SK_PASS;
+	}
+
+	ret = ops->negotiate((struct sock *)&smc->sk);
+	rcu_read_unlock();
+	return ret;
+}
+
+void smc_sock_perform_collecting_info(const struct smc_sock *smc, int timing)
+{
+	const struct smc_sock_negotiator_ops *ops;
+
+	rcu_read_lock();
+	ops = READ_ONCE(smc->negotiator_ops);
+
+	if (!ops || !ops->collect_info) {
+		rcu_read_unlock();
+		return;
+	}
+
+	ops->collect_info((struct sock *)&smc->sk, timing);
+	rcu_read_unlock();
+}
+
+int smc_sock_assign_negotiator_ops(struct smc_sock *smc, const char *name)
+{
+	struct smc_sock_negotiator_ops *ops;
+	int ret = -EINVAL;
+
+	/* already set */
+	if (READ_ONCE(smc->negotiator_ops))
+		smc_sock_cleanup_negotiator_ops(smc, /* might be still referenced */ false);
+
+	/* Just for clear negotiator_ops */
+	if (!name || !strlen(name))
+		return 0;
+
+	rcu_read_lock();
+	ops = smc_negotiator_ops_get_by_name(name);
+	if (likely(ops)) {
+		if (unlikely(!bpf_try_module_get(ops, ops->owner))) {
+			ret = -EACCES;
+		} else {
+			WRITE_ONCE(smc->negotiator_ops, ops);
+			/* make sure ops can be seen */
+			smp_wmb();
+			if (ops->init)
+				ops->init(&smc->sk);
+			ret = 0;
+		}
+	}
+	rcu_read_unlock();
+	return ret;
+}
+
+void smc_sock_cleanup_negotiator_ops(struct smc_sock *smc, bool no_more)
+{
+	const struct smc_sock_negotiator_ops *ops;
+
+	ops = READ_ONCE(smc->negotiator_ops);
+
+	/* not all smc sock has negotiator_ops */
+	if (!ops)
+		return;
+
+	might_sleep();
+
+	/* Just ensure data integrity */
+	WRITE_ONCE(smc->negotiator_ops, NULL);
+	/* make sure NULL can be seen */
+	smp_wmb();
+	/* if the socks may have references to the negotiator ops to be removed.
+	 * it means that we might need to wait for the readers of ops
+	 * to complete. It's slow though.
+	 */
+	if (unlikely(!no_more))
+		synchronize_rcu();
+	if (ops->release)
+		ops->release(&smc->sk);
+	bpf_module_put(ops, ops->owner);
+}
+
+void smc_sock_clone_negotiator_ops(struct sock *parent, struct sock *child)
+{
+	const struct smc_sock_negotiator_ops *ops;
+
+	rcu_read_lock();
+	ops = READ_ONCE(smc_sk(parent)->negotiator_ops);
+	if (ops && bpf_try_module_get(ops, ops->owner)) {
+		smc_sk(child)->negotiator_ops = ops;
+		if (ops->init)
+			ops->init(child);
+	}
+	rcu_read_unlock();
+}
+#endif
+
 int smc_nl_dump_hs_limitation(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct smc_nl_dmp_ctx *cb_ctx = smc_nl_dmp_ctx(cb);
@@ -166,6 +280,9 @@ static bool smc_hs_congested(const struct sock *sk)
 	if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
 		return true;
 
+	if (!smc_sock_should_select_smc(smc))
+		return true;
+
 	return false;
 }
 
@@ -320,6 +437,9 @@ static int smc_release(struct socket *sock)
 	sock_hold(sk); /* sock_put below */
 	smc = smc_sk(sk);
 
+	/* trigger info gathering if needed.*/
+	smc_sock_perform_collecting_info(smc, SMC_SOCK_CLOSED_TIMING);
+
 	old_state = sk->sk_state;
 
 	/* cleanup for a dangling non-blocking connect */
@@ -356,6 +476,9 @@ static int smc_release(struct socket *sock)
 
 static void smc_destruct(struct sock *sk)
 {
+	/* cleanup negotiator_ops if set */
+	smc_sock_cleanup_negotiator_ops(smc_sk(sk), /* no longer used */ true);
+
 	if (sk->sk_state != SMC_CLOSED)
 		return;
 	if (!sock_flag(sk, SOCK_DEAD))
@@ -1627,7 +1750,14 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
 	}
 
 	smc_copy_sock_settings_to_clc(smc);
-	tcp_sk(smc->clcsock->sk)->syn_smc = 1;
+	/* accept out connection as SMC connection */
+	if (smc_sock_should_select_smc(smc) == SK_PASS) {
+		tcp_sk(smc->clcsock->sk)->syn_smc = 1;
+	} else {
+		tcp_sk(smc->clcsock->sk)->syn_smc = 0;
+		smc_switch_to_fallback(smc, /* active fallback */ 0);
+	}
+
 	if (smc->connect_nonblock) {
 		rc = -EALREADY;
 		goto out;
@@ -1679,6 +1809,8 @@ static int smc_clcsock_accept(struct smc_sock *lsmc, struct smc_sock **new_smc)
 	}
 	*new_smc = smc_sk(new_sk);
 
+	smc_sock_clone_negotiator_ops(lsk, new_sk);
+
 	mutex_lock(&lsmc->clcsock_release_lock);
 	if (lsmc->clcsock)
 		rc = kernel_accept(lsmc->clcsock, &new_clcsock, SOCK_NONBLOCK);
diff --git a/net/smc/smc_negotiator.c b/net/smc/smc_negotiator.c
new file mode 100644
index 0000000..a93a19e
--- /dev/null
+++ b/net/smc/smc_negotiator.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Support eBPF for Shared Memory Communications over RDMA (SMC-R) and RoCE
+ *
+ *  Author(s):  D. Wythe <alibuda@linux.alibaba.com>
+ */
+#include <linux/kernel.h>
+#include <linux/bpf.h>
+#include <linux/smc.h>
+#include <net/sock.h>
+
+#include "smc_negotiator.h"
+#include "smc.h"
+
+static DEFINE_SPINLOCK(smc_sock_negotiator_list_lock);
+static LIST_HEAD(smc_sock_negotiator_list);
+
+/* required smc_sock_negotiator_list_lock locked */
+static inline struct smc_sock_negotiator_ops *smc_negotiator_ops_get_by_key(u32 key)
+{
+	struct smc_sock_negotiator_ops *ops;
+
+	list_for_each_entry_rcu(ops, &smc_sock_negotiator_list, list) {
+		if (ops->key == key)
+			return ops;
+	}
+
+	return NULL;
+}
+
+struct smc_sock_negotiator_ops *smc_negotiator_ops_get_by_name(const char *name)
+{
+	struct smc_sock_negotiator_ops *ops = NULL;
+
+	spin_lock(&smc_sock_negotiator_list_lock);
+	list_for_each_entry_rcu(ops, &smc_sock_negotiator_list, list) {
+		if (strcmp(ops->name, name) == 0)
+			break;
+	}
+	spin_unlock(&smc_sock_negotiator_list_lock);
+	return ops;
+}
+EXPORT_SYMBOL_GPL(smc_negotiator_ops_get_by_name);
+
+int smc_sock_validate_negotiator_ops(struct smc_sock_negotiator_ops *ops)
+{
+	/* not required yet */
+	return 0;
+}
+
+/* register ops */
+int smc_sock_register_negotiator_ops(struct smc_sock_negotiator_ops *ops)
+{
+	int ret;
+
+	ret = smc_sock_validate_negotiator_ops(ops);
+	if (ret)
+		return ret;
+
+	/* calt key by name hash */
+	ops->key = jhash(ops->name, sizeof(ops->name), strlen(ops->name));
+
+	spin_lock(&smc_sock_negotiator_list_lock);
+	if (smc_negotiator_ops_get_by_key(ops->key)) {
+		pr_notice("smc: %s negotiator already registered\n", ops->name);
+		ret = -EEXIST;
+	} else {
+		list_add_tail_rcu(&ops->list, &smc_sock_negotiator_list);
+	}
+	spin_unlock(&smc_sock_negotiator_list_lock);
+	return ret;
+}
+
+/* unregister ops */
+void smc_sock_unregister_negotiator_ops(struct smc_sock_negotiator_ops *ops)
+{
+	spin_lock(&smc_sock_negotiator_list_lock);
+	list_del_rcu(&ops->list);
+	spin_unlock(&smc_sock_negotiator_list_lock);
+
+	/* Wait for outstanding readers to complete before the
+	 * ops gets removed entirely.
+	 */
+	synchronize_rcu();
+}
+
+int smc_sock_update_negotiator_ops(struct smc_sock_negotiator_ops *ops,
+				   struct smc_sock_negotiator_ops *old_ops)
+{
+	struct smc_sock_negotiator_ops *existing;
+	int ret;
+
+	ret = smc_sock_validate_negotiator_ops(ops);
+	if (ret)
+		return ret;
+
+	ops->key = jhash(ops->name, sizeof(ops->name), strlen(ops->name));
+	if (unlikely(!ops->key))
+		return -EINVAL;
+
+	spin_lock(&smc_sock_negotiator_list_lock);
+	existing = smc_negotiator_ops_get_by_key(old_ops->key);
+	if (!existing || strcmp(existing->name, ops->name)) {
+		ret = -EINVAL;
+	} else if (existing != old_ops) {
+		pr_notice("invalid old negotiator to replace\n");
+		ret = -EINVAL;
+	} else {
+		list_add_tail_rcu(&ops->list, &smc_sock_negotiator_list);
+		list_del_rcu(&existing->list);
+	}
+
+	spin_unlock(&smc_sock_negotiator_list_lock);
+	if (ret)
+		return ret;
+
+	synchronize_rcu();
+	return 0;
+}
diff --git a/net/smc/smc_negotiator.h b/net/smc/smc_negotiator.h
new file mode 100644
index 0000000..b294ede
--- /dev/null
+++ b/net/smc/smc_negotiator.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ *  Support eBPF for Shared Memory Communications over RDMA (SMC-R) and RoCE
+ *
+ *  Author(s):  D. Wythe <alibuda@linux.alibaba.com>
+ */
+
+#include <linux/types.h>
+#include <net/smc.h>
+
+/* Max length of negotiator name */
+#define SMC_NEGOTIATOR_NAME_MAX	(16)
+
+/* closing time */
+#define SMC_SOCK_CLOSED_TIMING	(0)
+
+#ifdef CONFIG_SMC_BPF
+
+/* Register a new SMC socket negotiator ops
+ * The registered ops can then be assigned to SMC sockets using
+ * smc_sock_assign_negotiator_ops() via name
+ * Return: 0 on success, negative error code on failure
+ */
+int smc_sock_register_negotiator_ops(struct smc_sock_negotiator_ops *ops);
+
+/* Update an existing SMC socket negotiator ops
+ * This function is used to update an existing SMC socket negotiator ops. The new ops will
+ * replace the old ops who has the same name.
+ * Return: 0 on success, negative error code on failure.
+ */
+int smc_sock_update_negotiator_ops(struct smc_sock_negotiator_ops *ops,
+				   struct smc_sock_negotiator_ops *old_ops);
+
+/* Validate SMC negotiator operations
+ * This function is called to validate an SMC negotiator operations structure
+ * before it is assigned to a socket. It checks that all necessary function
+ * pointers are defined and not null.
+ * Returns 0 if the @ops argument is valid, or a negative error code otherwise.
+ */
+int smc_sock_validate_negotiator_ops(struct smc_sock_negotiator_ops *ops);
+
+/* Unregister an SMC socket negotiator ops
+ * This function is used to unregister an existing SMC socket negotiator ops.
+ * The ops will no longer be available for assignment to SMC sockets immediately.
+ */
+void smc_sock_unregister_negotiator_ops(struct smc_sock_negotiator_ops *ops);
+
+/* Get registered negotiator ops via name, caller should invoke it
+ * with RCU protected.
+ */
+struct smc_sock_negotiator_ops *smc_negotiator_ops_get_by_name(const char *name);
+
+/* Assign a negotiator ops to an SMC socket
+ * This function is used to assign a negotiator ops to an SMC socket.
+ * The ops must have been previously registered with
+ * smc_sock_register_negotiator_ops().
+ * Return: 0 on success, negative error code on failure.
+ */
+int smc_sock_assign_negotiator_ops(struct smc_sock *smc, const char *name);
+
+/* Remove negotiator ops who had assigned to @smc.
+ * @no_more implies that the caller explicitly states that the @smc have no references
+ * to the negotiator ops to be removed. This is not a mandatory option.
+ * When it sets to false, we will use RCU to protect ops, but in this case we have to
+ * always call synchronize_rcu(), which has a significant performance impact.
+ */
+void smc_sock_cleanup_negotiator_ops(struct smc_sock *smc, bool no_more);
+
+/* Clone negotiator ops of parnet sock to
+ * child sock.
+ */
+void smc_sock_clone_negotiator_ops(struct sock *parent, struct sock *child);
+
+/* Check if sock should use smc */
+int smc_sock_should_select_smc(const struct smc_sock *smc);
+
+/* Collect information to assigned ops */
+void smc_sock_perform_collecting_info(const struct smc_sock *smc, int timing);
+
+#else
+static inline int smc_sock_register_negotiator_ops(struct smc_sock_negotiator_ops *ops)
+{
+	return 0;
+}
+
+static inline int smc_sock_update_negotiator_ops(struct smc_sock_negotiator_ops *ops,
+						 struct smc_sock_negotiator_ops *old_ops)
+{
+	return 0;
+}
+
+static inline int smc_sock_validate_negotiator_ops(struct smc_sock_negotiator_ops *ops)
+{
+	return 0;
+}
+
+static inline void smc_sock_unregister_negotiator_ops(struct smc_sock_negotiator_ops *ops) {}
+
+static inline struct smc_sock_negotiator_ops *smc_negotiator_ops_get_by_name(const char *name)
+{
+	return NULL;
+}
+
+static inline int smc_sock_assign_negotiator_ops(struct smc_sock *smc, const char *name)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void smc_sock_cleanup_negotiator_ops(struct smc_sock *smc, bool no_more) {}
+
+static inline void smc_sock_clone_negotiator_ops(struct sock *parent, struct sock *child) {}
+
+static inline int smc_sock_should_select_smc(const struct smc_sock *smc) { return SK_PASS; }
+
+static inline void smc_sock_perform_collecting_info(const struct smc_sock *smc, int timing) {}
+#endif
-- 
1.8.3.1

[PATCH bpf-next v1 3/5] net/smc: allow set or get smc negotiator by sockopt

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

Allow applications to set specific protocol negotiation rules for SMC
Socks. Typically, applications need to know the name of the negotiator
and then set it through the syscall setsockopt, for examples:

const char name[] = "apps";
setsockopt(fd, SOL_SMC, SMC_NEGOTIATOR, name, sizeof(name) - 1);

Noted that there is no default negotiator in SMC implementation,
the application needs to inject the specific implementation through
eBPF before setting it up. Although no default negotiator implementation
is provided,

Note that SMC does not provide a default negotiator in SMC
implementation,
and the application needs to inject the specific implementation through
eBPF before setting it up. Although no default negotiator implementation
is provided, logically it can be seen as an implementation that
always return SK_PASS.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 include/uapi/linux/smc.h |   1 +
 net/smc/af_smc.c         | 131 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 98 insertions(+), 34 deletions(-)

diff --git a/include/uapi/linux/smc.h b/include/uapi/linux/smc.h
index bb4dacc..1887ed5 100644
--- a/include/uapi/linux/smc.h
+++ b/include/uapi/linux/smc.h
@@ -299,5 +299,6 @@ enum {
 
 /* SMC socket options */
 #define SMC_LIMIT_HS 1	/* constraint on smc handshake */
+#define SMC_NEGOTIATOR 2 /* SMC protocol negotiator */
 
 #endif /* _UAPI_LINUX_SMC_H */
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 7406fd4..a433c74 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -3001,48 +3001,37 @@ static int smc_shutdown(struct socket *sock, int how)
 	return rc ? rc : rc1;
 }
 
-static int __smc_getsockopt(struct socket *sock, int level, int optname,
-			    char __user *optval, int __user *optlen)
+/* set smc negotoiatior by name */
+static int smc_setsockopt_negotiator(struct sock *sk, sockptr_t optval,
+				     unsigned int optlen)
 {
-	struct smc_sock *smc;
-	int val, len;
-
-	smc = smc_sk(sock->sk);
-
-	if (get_user(len, optlen))
-		return -EFAULT;
-
-	len = min_t(int, len, sizeof(int));
+	char name[SMC_NEGOTIATOR_NAME_MAX];
+	struct smc_sock *smc = smc_sk(sk);
+	int val, rc;
 
-	if (len < 0)
+	if (optlen < 1)
 		return -EINVAL;
 
-	switch (optname) {
-	case SMC_LIMIT_HS:
-		val = smc->limit_smc_hs;
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-
-	if (put_user(len, optlen))
-		return -EFAULT;
-	if (copy_to_user(optval, &val, len))
+	val = strncpy_from_sockptr(name, optval,
+				   min_t(long, SMC_NEGOTIATOR_NAME_MAX - 1, optlen));
+	if (val < 0)
 		return -EFAULT;
 
-	return 0;
+	/* typical c str */
+	name[val] = 0;
+
+	sockopt_lock_sock(sk);
+	rc = smc_sock_assign_negotiator_ops(smc, name);
+	sockopt_release_sock(sk);
+	return rc;
 }
 
-static int __smc_setsockopt(struct socket *sock, int level, int optname,
+static int __smc_setsockopt(struct sock *sk, int level, int optname,
 			    sockptr_t optval, unsigned int optlen)
 {
-	struct sock *sk = sock->sk;
-	struct smc_sock *smc;
+	struct smc_sock *smc = smc_sk(sk);
 	int val, rc;
 
-	smc = smc_sk(sk);
-
-	lock_sock(sk);
 	switch (optname) {
 	case SMC_LIMIT_HS:
 		if (optlen < sizeof(int)) {
@@ -3053,15 +3042,17 @@ static int __smc_setsockopt(struct socket *sock, int level, int optname,
 			rc = -EFAULT;
 			break;
 		}
-
+		sockopt_lock_sock(sk);
 		smc->limit_smc_hs = !!val;
+		sockopt_release_sock(sk);
 		rc = 0;
 		break;
+	case SMC_NEGOTIATOR:
+		return smc_setsockopt_negotiator(sk, optval, optlen);
 	default:
 		rc = -EOPNOTSUPP;
 		break;
 	}
-	release_sock(sk);
 
 	return rc;
 }
@@ -3076,7 +3067,7 @@ static int smc_setsockopt(struct socket *sock, int level, int optname,
 	if (level == SOL_TCP && optname == TCP_ULP)
 		return -EOPNOTSUPP;
 	else if (level == SOL_SMC)
-		return __smc_setsockopt(sock, level, optname, optval, optlen);
+		return __smc_setsockopt(sk, level, optname, optval, optlen);
 
 	smc = smc_sk(sk);
 
@@ -3153,6 +3144,77 @@ static int smc_setsockopt(struct socket *sock, int level, int optname,
 	return rc;
 }
 
+/* get current negotoiatior sock used */
+static int smc_getsockopt_negotiator(struct sock *sk, sockptr_t optval,
+				     sockptr_t optlen)
+{
+#ifdef CONFIG_SMC_BPF
+	const struct smc_sock_negotiator_ops *ops;
+	struct smc_sock *smc = smc_sk(sk);
+	int len;
+
+	if (copy_from_sockptr(&len, optlen, sizeof(int)))
+		return -EFAULT;
+
+	len = min_t(unsigned int, len, sizeof(int));
+
+	if (len < 0)
+		return -EINVAL;
+
+	rcu_read_lock();
+	ops = READ_ONCE(smc->negotiator_ops);
+	if (ops) {
+		len = min_t(unsigned int, len, SMC_NEGOTIATOR_NAME_MAX);
+		if (copy_to_sockptr(optval, ops->name, len)) {
+			rcu_read_unlock();
+			return -EFAULT;
+		}
+	} else {
+		len = 0;
+	}
+	rcu_read_unlock();
+
+	if (copy_to_sockptr(optlen, &len, sizeof(int)))
+		return -EFAULT;
+
+	return 0;
+#else
+	return -EOPNOTSUPP;
+#endif
+}
+
+static int __smc_getsockopt(struct sock *sk, int level, int optname,
+			    sockptr_t optval, sockptr_t optlen)
+{
+	struct smc_sock *smc = smc_sk(sk);
+	int val, len;
+
+	if (copy_from_sockptr(&len, optlen, sizeof(int)))
+		return -EFAULT;
+
+	len = min_t(unsigned int, len, sizeof(int));
+
+	if (len < 0)
+		return -EINVAL;
+
+	switch (optname) {
+	case SMC_LIMIT_HS:
+		val = smc->limit_smc_hs;
+		break;
+	case SMC_NEGOTIATOR:
+		return smc_getsockopt_negotiator(sk, optval, optlen);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	if (copy_to_sockptr(optval, &val, len))
+		return -EFAULT;
+	if (copy_to_sockptr(optlen, &len, sizeof(int)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int smc_getsockopt(struct socket *sock, int level, int optname,
 			  char __user *optval, int __user *optlen)
 {
@@ -3160,7 +3222,8 @@ static int smc_getsockopt(struct socket *sock, int level, int optname,
 	int rc;
 
 	if (level == SOL_SMC)
-		return __smc_getsockopt(sock, level, optname, optval, optlen);
+		return __smc_getsockopt(sock->sk, level, optname,
+					USER_SOCKPTR(optval), USER_SOCKPTR(optlen));
 
 	smc = smc_sk(sock->sk);
 	mutex_lock(&smc->clcsock_release_lock);
-- 
1.8.3.1

[PATCH bpf-next v1 4/5] bpf: add smc negotiator support in BPF struct_ops

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

This PATCH attempts to introduce BPF injection capability for SMC.
Considering that the SMC protocol is not suitable for all scenarios,
especially for short-lived. However, for most applications, they cannot
guarantee that there are no such scenarios at all. Therefore, apps
may need some specific strategies to decide shall we need to use SMC
or not, for example, apps can limit the scope of the SMC to a specific
IP address or port.

Based on the consideration of transparent replacement, we hope that apps
can remain transparent even if they need to formulate some specific
strategies for SMC using. That is, do not need to recompile their code.

On the other hand, we need to ensure the scalability of strategies
implementation. Although it is simple to use socket options or sysctl,
it will bring more complexity to subsequent expansion.

Fortunately, BPF can solve these concerns very well, users can write
thire own strategies in eBPF to choose whether to use SMC or not.
And it's quite easy for them to modify their strategies in the future.

This PATCH implement injection capability for SMC via struct_ops.
In that way, we can add new injection scenarios in the future.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 kernel/bpf/bpf_struct_ops_types.h |   4 +
 net/Makefile                      |   2 +-
 net/smc/bpf_smc.c                 | 171 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100644 net/smc/bpf_smc.c

diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
index 5678a9d..d952b85 100644
--- a/kernel/bpf/bpf_struct_ops_types.h
+++ b/kernel/bpf/bpf_struct_ops_types.h
@@ -9,4 +9,8 @@
 #include <net/tcp.h>
 BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
 #endif
+#if IS_ENABLED(CONFIG_SMC_BPF)
+#include <net/smc.h>
+BPF_STRUCT_OPS_TYPE(smc_sock_negotiator_ops)
+#endif
 #endif
diff --git a/net/Makefile b/net/Makefile
index 222916a..2139fa4 100644
--- a/net/Makefile
+++ b/net/Makefile
@@ -52,7 +52,7 @@ obj-$(CONFIG_TIPC)		+= tipc/
 obj-$(CONFIG_NETLABEL)		+= netlabel/
 obj-$(CONFIG_IUCV)		+= iucv/
 obj-$(CONFIG_SMC)		+= smc/
-obj-$(CONFIG_SMC_BPF)		+= smc/smc_negotiator.o
+obj-$(CONFIG_SMC_BPF)		+= smc/smc_negotiator.o smc/bpf_smc.o
 obj-$(CONFIG_RFKILL)		+= rfkill/
 obj-$(CONFIG_NET_9P)		+= 9p/
 obj-$(CONFIG_CAIF)		+= caif/
diff --git a/net/smc/bpf_smc.c b/net/smc/bpf_smc.c
new file mode 100644
index 0000000..ac9a9ae91
--- /dev/null
+++ b/net/smc/bpf_smc.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Support eBPF for Shared Memory Communications over RDMA (SMC-R) and RoCE
+ *
+ *  Copyright IBM Corp. 2016, 2018
+ *
+ *  Author(s):  D. Wythe <alibuda@linux.alibaba.com>
+ */
+
+#include <linux/bpf_verifier.h>
+#include <linux/btf_ids.h>
+#include <linux/kernel.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
+#include "smc_negotiator.h"
+
+extern struct bpf_struct_ops bpf_smc_sock_negotiator_ops;
+static u32 smc_sock_id, sock_id;
+
+static int bpf_smc_negotiator_init(struct btf *btf)
+{
+	s32 type_id;
+
+	type_id = btf_find_by_name_kind(btf, "sock", BTF_KIND_STRUCT);
+	if (type_id < 0)
+		return -EINVAL;
+	sock_id = type_id;
+
+	type_id = btf_find_by_name_kind(btf, "smc_sock", BTF_KIND_STRUCT);
+	if (type_id < 0)
+		return -EINVAL;
+	smc_sock_id = type_id;
+
+	return 0;
+}
+
+/* register ops */
+static int bpf_smc_negotiator_reg(void *kdata)
+{
+	return smc_sock_register_negotiator_ops(kdata);
+}
+
+/* unregister ops */
+static void bpf_smc_negotiator_unreg(void *kdata)
+{
+	smc_sock_unregister_negotiator_ops(kdata);
+}
+
+/* unregister ops */
+static int bpf_smc_negotiator_update(void *kdata, void *old_kdata)
+{
+	return smc_sock_update_negotiator_ops(kdata, old_kdata);
+}
+
+static int bpf_smc_negotiator_validate(void *kdata)
+{
+	return smc_sock_validate_negotiator_ops(kdata);
+}
+
+static int bpf_smc_negotiator_check_member(const struct btf_type *t,
+					   const struct btf_member *member,
+					   const struct bpf_prog *prog)
+{
+	return 0;
+}
+
+static int bpf_smc_negotiator_init_member(const struct btf_type *t,
+					  const struct btf_member *member,
+					  void *kdata, const void *udata)
+{
+	const struct smc_sock_negotiator_ops *uops;
+	struct smc_sock_negotiator_ops *ops;
+	u32 moff;
+
+	uops = (const struct smc_sock_negotiator_ops *)udata;
+	ops = (struct smc_sock_negotiator_ops *)kdata;
+
+	moff = __btf_member_bit_offset(t, member) / 8;
+
+	/* init name */
+	if (moff ==  offsetof(struct smc_sock_negotiator_ops, name)) {
+		if (bpf_obj_name_cpy(ops->name, uops->name,
+				     sizeof(uops->name)) <= 0)
+			return -EINVAL;
+		return 1;
+	}
+
+	return 0;
+}
+
+BPF_CALL_1(bpf_smc_skc_to_tcp_sock, struct sock *, sk)
+{
+	if (sk && sk_fullsock(sk) && sk->sk_family == AF_SMC)
+		return (unsigned long)((struct smc_sock *)(sk))->clcsock->sk;
+
+	return (unsigned long)NULL;
+}
+
+static const struct bpf_func_proto bpf_smc_skc_to_tcp_sock_proto = {
+	.func			= bpf_smc_skc_to_tcp_sock,
+	.gpl_only		= false,
+	.ret_type		= RET_PTR_TO_BTF_ID_OR_NULL,
+	.arg1_type		= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
+	.ret_btf_id		= &btf_sock_ids[BTF_SOCK_TYPE_TCP],
+};
+
+static const struct bpf_func_proto *
+smc_negotiator_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+	const struct btf_member *m;
+	const struct btf_type *t;
+	u32 midx, moff;
+
+	midx = prog->expected_attach_type;
+	t = bpf_smc_sock_negotiator_ops.type;
+	m = &btf_type_member(t)[midx];
+
+	moff = __btf_member_bit_offset(t, m) / 8;
+
+	switch (func_id) {
+	case BPF_FUNC_setsockopt:
+		switch (moff) {
+		/* Avoid potential deadloop risk */
+		case offsetof(struct smc_sock_negotiator_ops, init):
+			fallthrough;
+		/* Avoid potential leak risk */
+		case offsetof(struct smc_sock_negotiator_ops, release):
+			return NULL;
+		}
+		return &bpf_sk_setsockopt_proto;
+	case BPF_FUNC_getsockopt:
+		return &bpf_sk_getsockopt_proto;
+	case BPF_FUNC_skc_to_tcp_sock:
+		return &bpf_smc_skc_to_tcp_sock_proto;
+	default:
+		return bpf_base_func_proto(func_id);
+	}
+}
+
+static bool smc_negotiator_prog_is_valid_access(int off, int size, enum bpf_access_type type,
+						const struct bpf_prog *prog,
+						struct bpf_insn_access_aux *info)
+{
+	if (!bpf_tracing_btf_ctx_access(off, size, type, prog, info))
+		return false;
+
+	/* promote it to smc_sock */
+	if (base_type(info->reg_type) == PTR_TO_BTF_ID &&
+	    !bpf_type_has_unsafe_modifiers(info->reg_type) &&
+	    info->btf_id == sock_id)
+		info->btf_id = smc_sock_id;
+
+	return true;
+}
+
+static const struct bpf_verifier_ops bpf_smc_negotiator_verifier_ops = {
+	.get_func_proto  = smc_negotiator_prog_func_proto,
+	.is_valid_access = smc_negotiator_prog_is_valid_access,
+};
+
+struct bpf_struct_ops bpf_smc_sock_negotiator_ops = {
+	.verifier_ops = &bpf_smc_negotiator_verifier_ops,
+	.init = bpf_smc_negotiator_init,
+	.check_member = bpf_smc_negotiator_check_member,
+	.init_member = bpf_smc_negotiator_init_member,
+	.reg = bpf_smc_negotiator_reg,
+	.update = bpf_smc_negotiator_update,
+	.unreg = bpf_smc_negotiator_unreg,
+	.validate = bpf_smc_negotiator_validate,
+	.name = "smc_sock_negotiator_ops",
+};
\ No newline at end of file
-- 
1.8.3.1

[PATCH bpf-next v1 5/5] bpf/selftests: add selftest for SMC bpf capability

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

This PATCH adds a tiny selftest for SMC bpf capability,
making decisions on whether to use SMC by collecting
certain information from kernel smc sock.

Follow the steps below to run this test.

make -C tools/testing/selftests/bpf
cd tools/testing/selftests/bpf
sudo ./test_progs -t smc

Results shows:
18/1    bpf_smc/load:OK
18/2    bpf_smc/update:OK
18/3    bpf_smc/ref:OK
18      bpf_smc:OK
Summary: 1/3 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 tools/testing/selftests/bpf/prog_tests/bpf_smc.c | 107 +++++++++
 tools/testing/selftests/bpf/progs/bpf_smc.c      | 265 +++++++++++++++++++++++
 2 files changed, 372 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_smc.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_smc.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_smc.c b/tools/testing/selftests/bpf/prog_tests/bpf_smc.c
new file mode 100644
index 0000000..e668857
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_smc.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/err.h>
+#include <netinet/tcp.h>
+#include <test_progs.h>
+#include "network_helpers.h"
+#include "bpf_smc.skel.h"
+
+#define SOL_SMC 286
+#define SMC_NEGOTIATOR 2
+static const char name[] = "apps";
+
+void run_smc(void)
+{
+	int fd, err;
+
+	fd = socket(AF_SMC, SOCK_STREAM, 0);
+	ASSERT_GT(fd, 0, "create smc socket");
+
+	err = setsockopt(fd, SOL_SMC, SMC_NEGOTIATOR, name, sizeof(name) / sizeof(char));
+	ASSERT_EQ(err, 0, "setsockopt");
+
+	close(fd);
+}
+
+void test_load(void)
+{
+	struct bpf_smc *smc_skel;
+	struct bpf_link *link;
+
+	smc_skel = bpf_smc__open_and_load();
+	if (!ASSERT_OK_PTR(smc_skel, "skel_open"))
+		return;
+
+	link = bpf_map__attach_struct_ops(smc_skel->maps.ops);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto error;
+
+	bpf_link__destroy(link);
+error:
+	bpf_smc__destroy(smc_skel);
+}
+
+void test_update(void)
+{
+	struct bpf_smc *smc_skel;
+	struct bpf_link *link;
+	int err;
+
+	smc_skel = bpf_smc__open_and_load();
+	if (!ASSERT_OK_PTR(smc_skel, "skel_open"))
+		return;
+
+	link = bpf_map__attach_struct_ops(smc_skel->maps.accept);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto error;
+
+	run_smc();
+	ASSERT_EQ(smc_skel->bss->accept_cnt, 1, "accept_cnt");
+
+	err = bpf_link__update_map(link, smc_skel->maps.drop);
+	ASSERT_OK(err, "update_map");
+
+	run_smc();
+	ASSERT_EQ(smc_skel->bss->accept_cnt, 1, "accept_cnt");
+	ASSERT_EQ(smc_skel->bss->drop_cnt, 1, "drop_cnt");
+
+	bpf_link__destroy(link);
+error:
+	bpf_smc__destroy(smc_skel);
+}
+
+void test_ref(void)
+{
+	struct bpf_smc *smc_skel;
+	struct bpf_link *link;
+	int fd = 0, err;
+
+	smc_skel = bpf_smc__open_and_load();
+	if (!ASSERT_OK_PTR(smc_skel, "skel_open"))
+		return;
+
+	link = bpf_map__attach_struct_ops(smc_skel->maps.accept);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops"))
+		goto error;
+
+	fd = socket(AF_SMC, SOCK_STREAM, 0);
+	ASSERT_GT(fd, 0, "create smc socket");
+	err = setsockopt(fd, SOL_SMC, SMC_NEGOTIATOR, name, sizeof(name) / sizeof(char));
+	ASSERT_EQ(err, 0, "setsockopt");
+	bpf_link__destroy(link);
+	if (fd > 0)
+		close(fd);
+	ASSERT_EQ(smc_skel->bss->accept_release_cnt, 1, "accept_release_cnt");
+error:
+	bpf_smc__destroy(smc_skel);
+}
+
+void test_bpf_smc(void)
+{
+	if (test__start_subtest("load"))
+		test_load();
+	if (test__start_subtest("update"))
+		test_update();
+	if (test__start_subtest("ref"))
+		test_ref();
+}
diff --git a/tools/testing/selftests/bpf/progs/bpf_smc.c b/tools/testing/selftests/bpf/progs/bpf_smc.c
new file mode 100644
index 0000000..8ff70af
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_smc.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+#include <bpf/bpf_tracing.h>
+
+#define AF_SMC			(43)
+#define SMC_LISTEN		(10)
+#define SMC_SOCK_CLOSED_TIMING	(0)
+extern unsigned long CONFIG_HZ __kconfig;
+#define HZ CONFIG_HZ
+
+char _license[] SEC("license") = "GPL";
+#define max(a, b) ((a) > (b) ? (a) : (b))
+
+static __always_inline struct smc_sock *smc_sk(struct sock *sk)
+{
+	return (struct smc_sock *)sk;
+}
+
+struct smc_prediction {
+	/* protection for smc_prediction */
+	struct bpf_spin_lock lock;
+	/* start of time slice */
+	__u64	start_tstamp;
+	/* delta of pacing */
+	__u64	pacing_delta;
+	/* N of closed connections determined as long connections
+	 * in current time slice
+	 */
+	__u32	closed_long_cc;
+	/* N of closed connections in this time slice */
+	__u32	closed_total_cc;
+	/* N of incoming connections determined as long connections
+	 * in current time slice
+	 */
+	__u32	incoming_long_cc;
+	/* last splice rate of long cc */
+	__u32	last_rate_of_lcc;
+};
+
+#define SMC_PREDICTION_MIN_PACING_DELTA                (1llu)
+#define SMC_PREDICTION_MAX_PACING_DELTA                (HZ << 3)
+#define SMC_PREDICTION_MAX_LONGCC_PER_SPLICE           (8)
+#define SMC_PREDICTION_MAX_PORT                        (64)
+#define SMC_PREDICTION_MAX_SPLICE_GAP                  (1)
+#define SMC_PREDICTION_LONGCC_RATE_THRESHOLD           (13189)
+#define SMC_PREDICTION_LONGCC_PACKETS_THRESHOLD        (100)
+#define SMC_PREDICTION_LONGCC_BYTES_THRESHOLD	\
+		(SMC_PREDICTION_LONGCC_PACKETS_THRESHOLD * 1024)
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(max_entries, SMC_PREDICTION_MAX_PORT);
+	__type(key, __u16);
+	__type(value, struct smc_prediction);
+} negotiator_map SEC(".maps");
+
+
+static inline __u32 smc_prediction_calt_rate(struct smc_prediction *smc_predictor)
+{
+	if (!smc_predictor->closed_total_cc)
+		return smc_predictor->last_rate_of_lcc;
+
+	return (smc_predictor->closed_long_cc << 14) / smc_predictor->closed_total_cc;
+}
+
+static inline struct smc_prediction *smc_prediction_get(__u16 key, __u64 tstamp)
+{
+	struct smc_prediction zero = {}, *smc_predictor;
+	__u32 gap;
+	int err;
+
+	smc_predictor = bpf_map_lookup_elem(&negotiator_map, &key);
+	if (!smc_predictor) {
+		zero.start_tstamp = bpf_jiffies64();
+		zero.pacing_delta = SMC_PREDICTION_MIN_PACING_DELTA;
+		err = bpf_map_update_elem(&negotiator_map, &key, &zero, 0);
+		if (err)
+			return NULL;
+		smc_predictor =  bpf_map_lookup_elem(&negotiator_map, &key);
+		if (!smc_predictor)
+			return NULL;
+	}
+
+	if (tstamp) {
+		bpf_spin_lock(&smc_predictor->lock);
+		gap = (tstamp - smc_predictor->start_tstamp) / smc_predictor->pacing_delta;
+		/* new splice */
+		if (gap > 0) {
+			smc_predictor->start_tstamp = tstamp;
+			smc_predictor->last_rate_of_lcc =
+				(smc_prediction_calt_rate(smc_predictor) * 7) >> (2 + gap);
+			smc_predictor->closed_long_cc = 0;
+			smc_predictor->closed_total_cc = 0;
+			smc_predictor->incoming_long_cc = 0;
+		}
+		bpf_spin_unlock(&smc_predictor->lock);
+	}
+	return smc_predictor;
+}
+
+int SEC("struct_ops/bpf_smc_negotiate")
+BPF_PROG(bpf_smc_negotiate, struct sock *sk)
+{
+	struct smc_prediction *smc_predictor;
+	struct smc_sock *smc = smc_sk(sk);
+	struct tcp_sock *tp;
+	__u32 rate = 0;
+	__u16 key;
+
+	/* client side */
+	if (smc == NULL || smc->sk.__sk_common.skc_state != SMC_LISTEN) {
+		/* use Global smc_predictor */
+		key = 0;
+	} else {	/* server side */
+		tp = bpf_skc_to_tcp_sock(sk);
+		if (!tp)
+			goto error;
+		key = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num;
+	}
+
+	smc_predictor = smc_prediction_get(key, bpf_jiffies64());
+	if (!smc_predictor)
+		return SK_PASS;
+
+	bpf_spin_lock(&smc_predictor->lock);
+
+	if (smc_predictor->incoming_long_cc == 0)
+		goto out_locked_pass;
+
+	if (smc_predictor->incoming_long_cc > SMC_PREDICTION_MAX_LONGCC_PER_SPLICE)
+		goto out_locked_drop;
+
+	rate = smc_prediction_calt_rate(smc_predictor);
+	if (rate < SMC_PREDICTION_LONGCC_RATE_THRESHOLD)
+		goto out_locked_drop;
+
+out_locked_pass:
+	smc_predictor->incoming_long_cc++;
+	bpf_spin_unlock(&smc_predictor->lock);
+	return SK_PASS;
+out_locked_drop:
+	bpf_spin_unlock(&smc_predictor->lock);
+error:
+	return SK_DROP;
+}
+
+void SEC("struct_ops/bpf_smc_collect_info")
+BPF_PROG(bpf_smc_collect_info, struct sock *sk, int timing)
+{
+	struct smc_prediction *smc_predictor;
+	int use_fallback, sndbuf;
+	struct smc_sock *smc;
+	struct tcp_sock *tp;
+	bool match = false;
+	__u16 wrap, count;
+	__u16 key;
+
+	/* no info can collect */
+	if (sk == NULL)
+		return;
+
+	/* only fouces on closed */
+	if (timing != SMC_SOCK_CLOSED_TIMING)
+		return;
+
+	/* every full smc sock should contains a tcp sock */
+	tp = bpf_skc_to_tcp_sock(sk);
+	if (!tp)
+		return;
+
+	smc = smc_sk(sk);
+	if (smc->use_fallback) {
+		use_fallback = 1;
+		match = tp->delivered > SMC_PREDICTION_LONGCC_PACKETS_THRESHOLD;
+	} else {
+		wrap = smc->conn.tx_curs_sent.wrap;
+		count = smc->conn.tx_curs_sent.count;
+		sndbuf = tp->inet_conn.icsk_inet.sk.sk_sndbuf;
+		match = (count + wrap * sndbuf) > SMC_PREDICTION_LONGCC_BYTES_THRESHOLD;
+	}
+
+	key = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num;
+
+	smc_predictor = smc_prediction_get(key, 0);
+	if (!smc_predictor)
+		goto error;
+
+	bpf_spin_lock(&smc_predictor->lock);
+	smc_predictor->closed_total_cc++;
+	if (match) {
+		/* increase stats */
+		smc_predictor->closed_long_cc++;
+		/* try more aggressive */
+		if (smc_predictor->pacing_delta > SMC_PREDICTION_MIN_PACING_DELTA) {
+			if (use_fallback) {
+				smc_predictor->pacing_delta = max(SMC_PREDICTION_MIN_PACING_DELTA,
+						(smc_predictor->pacing_delta * 3) >> 2);
+			}
+		}
+	} else if (!use_fallback) {
+		smc_predictor->pacing_delta <<= 1;
+	}
+	bpf_spin_unlock(&smc_predictor->lock);
+error:
+	return;
+}
+
+SEC(".struct_ops.link")
+struct smc_sock_negotiator_ops ops = {
+	.name = "apps",
+	.negotiate	= (void *)bpf_smc_negotiate,
+	.collect_info	= (void *)bpf_smc_collect_info,
+};
+
+int accept_cnt = 0;
+int drop_cnt = 0;
+int accept_release_cnt = 0;
+
+int SEC("struct_ops/bpf_smc_accept")
+BPF_PROG(bpf_smc_accept, struct sock *sk)
+{
+	return SK_PASS;
+}
+
+void SEC("struct_ops/bpf_smc_accept_init")
+BPF_PROG(bpf_smc_accept_init, struct sock *sk)
+{
+	accept_cnt++;
+}
+
+void SEC("struct_ops/bpf_smc_accept_release")
+BPF_PROG(bpf_smc_accept_release, struct sock *sk)
+{
+	accept_release_cnt++;
+}
+
+int SEC("struct_ops/bpf_smc_drop")
+BPF_PROG(bpf_smc_drop, struct sock *sk)
+{
+	return SK_DROP;
+}
+
+void SEC("struct_ops/bpf_smc_drop_init")
+BPF_PROG(bpf_smc_drop_init, struct sock *sk)
+{
+	drop_cnt++;
+}
+
+SEC(".struct_ops.link")
+struct smc_sock_negotiator_ops accept = {
+	.name = "apps",
+	.init = (void *) bpf_smc_accept_init,
+	.release = (void *) bpf_smc_accept_release,
+	.negotiate = (void *) bpf_smc_accept,
+};
+
+SEC(".struct_ops.link")
+struct smc_sock_negotiator_ops drop = {
+	.name = "apps",
+	.init = (void *) bpf_smc_drop_init,
+	.negotiate = (void *) bpf_smc_drop,
+};
-- 
1.8.3.1

Re: [PATCH bpf-next v1 2/5] net/smc: allow smc to negotiate protocols on policies

[kernel test robot]

Hi Wythe,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/D-Wythe/net-smc-move-smc_sock-related-structure-definition/20230512-142700
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/1683872684-64872-3-git-send-email-alibuda%40linux.alibaba.com
patch subject: [PATCH bpf-next v1 2/5] net/smc: allow smc to negotiate protocols on policies
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20230512/202305122104.msaKEOV1-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/db8daea84b78121c3612ad5e5ba1d1eaac2f4171
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review D-Wythe/net-smc-move-smc_sock-related-structure-definition/20230512-142700
        git checkout db8daea84b78121c3612ad5e5ba1d1eaac2f4171
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305122104.msaKEOV1-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "bpf_struct_ops_get" [net/smc/smc.ko] undefined!
>> ERROR: modpost: "bpf_struct_ops_put" [net/smc/smc.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH bpf-next v1 4/5] bpf: add smc negotiator support in BPF struct_ops

[Yonghong Song]

On 5/11/23 11:24 PM, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> This PATCH attempts to introduce BPF injection capability for SMC.
> Considering that the SMC protocol is not suitable for all scenarios,
> especially for short-lived. However, for most applications, they cannot
> guarantee that there are no such scenarios at all. Therefore, apps
> may need some specific strategies to decide shall we need to use SMC
> or not, for example, apps can limit the scope of the SMC to a specific
> IP address or port.
> 
> Based on the consideration of transparent replacement, we hope that apps
> can remain transparent even if they need to formulate some specific
> strategies for SMC using. That is, do not need to recompile their code.
> 
> On the other hand, we need to ensure the scalability of strategies
> implementation. Although it is simple to use socket options or sysctl,
> it will bring more complexity to subsequent expansion.
> 
> Fortunately, BPF can solve these concerns very well, users can write
> thire own strategies in eBPF to choose whether to use SMC or not.
> And it's quite easy for them to modify their strategies in the future.
> 
> This PATCH implement injection capability for SMC via struct_ops.
> In that way, we can add new injection scenarios in the future.
> 
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> ---
>   kernel/bpf/bpf_struct_ops_types.h |   4 +
>   net/Makefile                      |   2 +-
>   net/smc/bpf_smc.c                 | 171 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 176 insertions(+), 1 deletion(-)
>   create mode 100644 net/smc/bpf_smc.c
> 
> diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
> index 5678a9d..d952b85 100644
> --- a/kernel/bpf/bpf_struct_ops_types.h
> +++ b/kernel/bpf/bpf_struct_ops_types.h
> @@ -9,4 +9,8 @@
>   #include <net/tcp.h>
>   BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
>   #endif
> +#if IS_ENABLED(CONFIG_SMC_BPF)
> +#include <net/smc.h>
> +BPF_STRUCT_OPS_TYPE(smc_sock_negotiator_ops)
> +#endif
>   #endif
> diff --git a/net/Makefile b/net/Makefile
> index 222916a..2139fa4 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -52,7 +52,7 @@ obj-$(CONFIG_TIPC)		+= tipc/
>   obj-$(CONFIG_NETLABEL)		+= netlabel/
>   obj-$(CONFIG_IUCV)		+= iucv/
>   obj-$(CONFIG_SMC)		+= smc/
> -obj-$(CONFIG_SMC_BPF)		+= smc/smc_negotiator.o
> +obj-$(CONFIG_SMC_BPF)		+= smc/smc_negotiator.o smc/bpf_smc.o
>   obj-$(CONFIG_RFKILL)		+= rfkill/
>   obj-$(CONFIG_NET_9P)		+= 9p/
>   obj-$(CONFIG_CAIF)		+= caif/
> diff --git a/net/smc/bpf_smc.c b/net/smc/bpf_smc.c
> new file mode 100644
> index 0000000..ac9a9ae91
> --- /dev/null
> +++ b/net/smc/bpf_smc.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  Support eBPF for Shared Memory Communications over RDMA (SMC-R) and RoCE
> + *
> + *  Copyright IBM Corp. 2016, 2018

The above description and copyright sound very wierd.

> + *
> + *  Author(s):  D. Wythe <alibuda@linux.alibaba.com>

One author, so just "Author: ...".
> + */
> +
> +#include <linux/bpf_verifier.h>
> +#include <linux/btf_ids.h>
> +#include <linux/kernel.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> +#include "smc_negotiator.h"
> +
> +extern struct bpf_struct_ops bpf_smc_sock_negotiator_ops;
> +static u32 smc_sock_id, sock_id;
> +
> +static int bpf_smc_negotiator_init(struct btf *btf)
> +{
> +	s32 type_id;
> +
> +	type_id = btf_find_by_name_kind(btf, "sock", BTF_KIND_STRUCT);
> +	if (type_id < 0)
> +		return -EINVAL;
> +	sock_id = type_id;
> +
> +	type_id = btf_find_by_name_kind(btf, "smc_sock", BTF_KIND_STRUCT);
> +	if (type_id < 0)
> +		return -EINVAL;
> +	smc_sock_id = type_id;
> +
> +	return 0;
> +}
> +
> +/* register ops */
> +static int bpf_smc_negotiator_reg(void *kdata)
> +{
> +	return smc_sock_register_negotiator_ops(kdata);
> +}
> +
> +/* unregister ops */
> +static void bpf_smc_negotiator_unreg(void *kdata)
> +{
> +	smc_sock_unregister_negotiator_ops(kdata);
> +}
> +
> +/* unregister ops */

update ops?
Also I think the above comments like
'register ops', 'unregister ops' and 'update ops' are not
necessary. The code itself is self-explanary.

> +static int bpf_smc_negotiator_update(void *kdata, void *old_kdata)
> +{
> +	return smc_sock_update_negotiator_ops(kdata, old_kdata);
> +}
> +
> +static int bpf_smc_negotiator_validate(void *kdata)
> +{
> +	return smc_sock_validate_negotiator_ops(kdata);
> +}
> +
> +static int bpf_smc_negotiator_check_member(const struct btf_type *t,
> +					   const struct btf_member *member,
> +					   const struct bpf_prog *prog)
> +{
> +	return 0;
> +}
> +
> +static int bpf_smc_negotiator_init_member(const struct btf_type *t,
> +					  const struct btf_member *member,
> +					  void *kdata, const void *udata)
> +{
> +	const struct smc_sock_negotiator_ops *uops;
> +	struct smc_sock_negotiator_ops *ops;
> +	u32 moff;
> +
> +	uops = (const struct smc_sock_negotiator_ops *)udata;
> +	ops = (struct smc_sock_negotiator_ops *)kdata;
> +
> +	moff = __btf_member_bit_offset(t, member) / 8;
> +
> +	/* init name */
> +	if (moff ==  offsetof(struct smc_sock_negotiator_ops, name)) {
> +		if (bpf_obj_name_cpy(ops->name, uops->name,
> +				     sizeof(uops->name)) <= 0)
> +			return -EINVAL;
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +BPF_CALL_1(bpf_smc_skc_to_tcp_sock, struct sock *, sk)
> +{
> +	if (sk && sk_fullsock(sk) && sk->sk_family == AF_SMC)
> +		return (unsigned long)((struct smc_sock *)(sk))->clcsock->sk;
> +
> +	return (unsigned long)NULL;
> +}
> +
> +static const struct bpf_func_proto bpf_smc_skc_to_tcp_sock_proto = {
> +	.func			= bpf_smc_skc_to_tcp_sock,
> +	.gpl_only		= false,
> +	.ret_type		= RET_PTR_TO_BTF_ID_OR_NULL,
> +	.arg1_type		= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> +	.ret_btf_id		= &btf_sock_ids[BTF_SOCK_TYPE_TCP],
> +};
> +
> +static const struct bpf_func_proto *
> +smc_negotiator_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> +{
> +	const struct btf_member *m;
> +	const struct btf_type *t;
> +	u32 midx, moff;
> +
> +	midx = prog->expected_attach_type;
> +	t = bpf_smc_sock_negotiator_ops.type;
> +	m = &btf_type_member(t)[midx];
> +
> +	moff = __btf_member_bit_offset(t, m) / 8;
> +
> +	switch (func_id) {
> +	case BPF_FUNC_setsockopt:
> +		switch (moff) {
> +		/* Avoid potential deadloop risk */
> +		case offsetof(struct smc_sock_negotiator_ops, init):
> +			fallthrough;

I am not sure whether a 'fallthrough' is needed here or since the case
itself does not have any code. Any warning will show up if
'fallthrough;' is removed?

> +		/* Avoid potential leak risk */

I think more detailed explanation about 'deadloop risk' and 'leak risk'
is necessary.

> +		case offsetof(struct smc_sock_negotiator_ops, release):
> +			return NULL;
> +		}
> +		return &bpf_sk_setsockopt_proto;
> +	case BPF_FUNC_getsockopt:
> +		return &bpf_sk_getsockopt_proto;
> +	case BPF_FUNC_skc_to_tcp_sock:
> +		return &bpf_smc_skc_to_tcp_sock_proto;
> +	default:
> +		return bpf_base_func_proto(func_id);
> +	}
> +}
> +
> +static bool smc_negotiator_prog_is_valid_access(int off, int size, enum bpf_access_type type,
> +						const struct bpf_prog *prog,
> +						struct bpf_insn_access_aux *info)
> +{
> +	if (!bpf_tracing_btf_ctx_access(off, size, type, prog, info))
> +		return false;
> +
> +	/* promote it to smc_sock */
> +	if (base_type(info->reg_type) == PTR_TO_BTF_ID &&
> +	    !bpf_type_has_unsafe_modifiers(info->reg_type) &&
> +	    info->btf_id == sock_id)
> +		info->btf_id = smc_sock_id;
> +
> +	return true;
> +}
> +
> +static const struct bpf_verifier_ops bpf_smc_negotiator_verifier_ops = {
> +	.get_func_proto  = smc_negotiator_prog_func_proto,
> +	.is_valid_access = smc_negotiator_prog_is_valid_access,
> +};
> +
> +struct bpf_struct_ops bpf_smc_sock_negotiator_ops = {
> +	.verifier_ops = &bpf_smc_negotiator_verifier_ops,
> +	.init = bpf_smc_negotiator_init,
> +	.check_member = bpf_smc_negotiator_check_member,
> +	.init_member = bpf_smc_negotiator_init_member,
> +	.reg = bpf_smc_negotiator_reg,
> +	.update = bpf_smc_negotiator_update,
> +	.unreg = bpf_smc_negotiator_unreg,
> +	.validate = bpf_smc_negotiator_validate,
> +	.name = "smc_sock_negotiator_ops",
> +};
> \ No newline at end of file

Empty line at the end?

Re: [PATCH bpf-next v1 4/5] bpf: add smc negotiator support in BPF struct_ops

[D. Wythe]

On 5/13/23 10:36 AM, Yonghong Song wrote:
>
>
> On 5/11/23 11:24 PM, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> This PATCH attempts to introduce BPF injection capability for SMC.
>> Considering that the SMC protocol is not suitable for all scenarios,
>> especially for short-lived. However, for most applications, they cannot
>> guarantee that there are no such scenarios at all. Therefore, apps
>> may need some specific strategies to decide shall we need to use SMC
>> or not, for example, apps can limit the scope of the SMC to a specific
>> IP address or port.
>>
>> Based on the consideration of transparent replacement, we hope that apps
>> can remain transparent even if they need to formulate some specific
>> strategies for SMC using. That is, do not need to recompile their code.
>>
>> On the other hand, we need to ensure the scalability of strategies
>> implementation. Although it is simple to use socket options or sysctl,
>> it will bring more complexity to subsequent expansion.
>>
>> Fortunately, BPF can solve these concerns very well, users can write
>> thire own strategies in eBPF to choose whether to use SMC or not.
>> And it's quite easy for them to modify their strategies in the future.
>>
>> This PATCH implement injection capability for SMC via struct_ops.
>> In that way, we can add new injection scenarios in the future.
>>
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>>   kernel/bpf/bpf_struct_ops_types.h |   4 +
>>   net/Makefile                      |   2 +-
>>   net/smc/bpf_smc.c                 | 171 
>> ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 176 insertions(+), 1 deletion(-)
>>   create mode 100644 net/smc/bpf_smc.c
>>
>> diff --git a/kernel/bpf/bpf_struct_ops_types.h 
>> b/kernel/bpf/bpf_struct_ops_types.h
>> index 5678a9d..d952b85 100644
>> --- a/kernel/bpf/bpf_struct_ops_types.h
>> +++ b/kernel/bpf/bpf_struct_ops_types.h
>> @@ -9,4 +9,8 @@
>>   #include <net/tcp.h>
>>   BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
>>   #endif
>> +#if IS_ENABLED(CONFIG_SMC_BPF)
>> +#include <net/smc.h>
>> +BPF_STRUCT_OPS_TYPE(smc_sock_negotiator_ops)
>> +#endif
>>   #endif
>> diff --git a/net/Makefile b/net/Makefile
>> index 222916a..2139fa4 100644
>> --- a/net/Makefile
>> +++ b/net/Makefile
>> @@ -52,7 +52,7 @@ obj-$(CONFIG_TIPC)        += tipc/
>>   obj-$(CONFIG_NETLABEL)        += netlabel/
>>   obj-$(CONFIG_IUCV)        += iucv/
>>   obj-$(CONFIG_SMC)        += smc/
>> -obj-$(CONFIG_SMC_BPF)        += smc/smc_negotiator.o
>> +obj-$(CONFIG_SMC_BPF)        += smc/smc_negotiator.o smc/bpf_smc.o
>>   obj-$(CONFIG_RFKILL)        += rfkill/
>>   obj-$(CONFIG_NET_9P)        += 9p/
>>   obj-$(CONFIG_CAIF)        += caif/
>> diff --git a/net/smc/bpf_smc.c b/net/smc/bpf_smc.c
>> new file mode 100644
>> index 0000000..ac9a9ae91
>> --- /dev/null
>> +++ b/net/smc/bpf_smc.c
>> @@ -0,0 +1,171 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + *  Support eBPF for Shared Memory Communications over RDMA (SMC-R) 
>> and RoCE
>> + *
>> + *  Copyright IBM Corp. 2016, 2018
>
> The above description and copyright sound very wierd.

Received, let me see how to modify it.

>
>> + *
>> + *  Author(s):  D. Wythe <alibuda@linux.alibaba.com>
>
> One author, so just "Author: ...".

Got it. I will fix that.

>> + */
>> +
>> +#include <linux/bpf_verifier.h>
>> +#include <linux/btf_ids.h>
>> +#include <linux/kernel.h>
>> +#include <linux/bpf.h>
>> +#include <linux/btf.h>
>> +#include "smc_negotiator.h"
>> +
>> +extern struct bpf_struct_ops bpf_smc_sock_negotiator_ops;
>> +static u32 smc_sock_id, sock_id;
>> +
>> +static int bpf_smc_negotiator_init(struct btf *btf)
>> +{
>> +    s32 type_id;
>> +
>> +    type_id = btf_find_by_name_kind(btf, "sock", BTF_KIND_STRUCT);
>> +    if (type_id < 0)
>> +        return -EINVAL;
>> +    sock_id = type_id;
>> +
>> +    type_id = btf_find_by_name_kind(btf, "smc_sock", BTF_KIND_STRUCT);
>> +    if (type_id < 0)
>> +        return -EINVAL;
>> +    smc_sock_id = type_id;
>> +
>> +    return 0;
>> +}
>> +
>> +/* register ops */
>> +static int bpf_smc_negotiator_reg(void *kdata)
>> +{
>> +    return smc_sock_register_negotiator_ops(kdata);
>> +}
>> +
>> +/* unregister ops */
>> +static void bpf_smc_negotiator_unreg(void *kdata)
>> +{
>> +    smc_sock_unregister_negotiator_ops(kdata);
>> +}
>> +
>> +/* unregister ops */
>
> update ops?
> Also I think the above comments like
> 'register ops', 'unregister ops' and 'update ops' are not
> necessary. The code itself is self-explanary.
My mistake, thank you very much for your suggestion. The annotations here
are unnecessary indeed.
>
>> +static int bpf_smc_negotiator_update(void *kdata, void *old_kdata)
>> +{
>> +    return smc_sock_update_negotiator_ops(kdata, old_kdata);
>> +}
>> +
>> +static int bpf_smc_negotiator_validate(void *kdata)
>> +{
>> +    return smc_sock_validate_negotiator_ops(kdata);
>> +}
>> +
>> +static int bpf_smc_negotiator_check_member(const struct btf_type *t,
>> +                       const struct btf_member *member,
>> +                       const struct bpf_prog *prog)
>> +{
>> +    return 0;
>> +}
>> +
>> +static int bpf_smc_negotiator_init_member(const struct btf_type *t,
>> +                      const struct btf_member *member,
>> +                      void *kdata, const void *udata)
>> +{
>> +    const struct smc_sock_negotiator_ops *uops;
>> +    struct smc_sock_negotiator_ops *ops;
>> +    u32 moff;
>> +
>> +    uops = (const struct smc_sock_negotiator_ops *)udata;
>> +    ops = (struct smc_sock_negotiator_ops *)kdata;
>> +
>> +    moff = __btf_member_bit_offset(t, member) / 8;
>> +
>> +    /* init name */
>> +    if (moff ==  offsetof(struct smc_sock_negotiator_ops, name)) {
>> +        if (bpf_obj_name_cpy(ops->name, uops->name,
>> +                     sizeof(uops->name)) <= 0)
>> +            return -EINVAL;
>> +        return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +BPF_CALL_1(bpf_smc_skc_to_tcp_sock, struct sock *, sk)
>> +{
>> +    if (sk && sk_fullsock(sk) && sk->sk_family == AF_SMC)
>> +        return (unsigned long)((struct smc_sock *)(sk))->clcsock->sk;
>> +
>> +    return (unsigned long)NULL;
>> +}
>> +
>> +static const struct bpf_func_proto bpf_smc_skc_to_tcp_sock_proto = {
>> +    .func            = bpf_smc_skc_to_tcp_sock,
>> +    .gpl_only        = false,
>> +    .ret_type        = RET_PTR_TO_BTF_ID_OR_NULL,
>> +    .arg1_type        = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
>> +    .ret_btf_id        = &btf_sock_ids[BTF_SOCK_TYPE_TCP],
>> +};
>> +
>> +static const struct bpf_func_proto *
>> +smc_negotiator_prog_func_proto(enum bpf_func_id func_id, const 
>> struct bpf_prog *prog)
>> +{
>> +    const struct btf_member *m;
>> +    const struct btf_type *t;
>> +    u32 midx, moff;
>> +
>> +    midx = prog->expected_attach_type;
>> +    t = bpf_smc_sock_negotiator_ops.type;
>> +    m = &btf_type_member(t)[midx];
>> +
>> +    moff = __btf_member_bit_offset(t, m) / 8;
>> +
>> +    switch (func_id) {
>> +    case BPF_FUNC_setsockopt:
>> +        switch (moff) {
>> +        /* Avoid potential deadloop risk */
>> +        case offsetof(struct smc_sock_negotiator_ops, init):
>> +            fallthrough;
>
> I am not sure whether a 'fallthrough' is needed here or since the case
> itself does not have any code. Any warning will show up if
> 'fallthrough;' is removed?

Yes, if there is no code, fallthrough is unnecessary, I will fix it in 
the next version.

>
>> +        /* Avoid potential leak risk */
>
> I think more detailed explanation about 'deadloop risk' and 'leak risk'
> is necessary.

Got it, i will add more detailed explanation.
>
>> +        case offsetof(struct smc_sock_negotiator_ops, release):
>> +            return NULL;
>> +        }
>> +        return &bpf_sk_setsockopt_proto;
>> +    case BPF_FUNC_getsockopt:
>> +        return &bpf_sk_getsockopt_proto;
>> +    case BPF_FUNC_skc_to_tcp_sock:
>> +        return &bpf_smc_skc_to_tcp_sock_proto;
>> +    default:
>> +        return bpf_base_func_proto(func_id);
>> +    }
>> +}
>> +
>> +static bool smc_negotiator_prog_is_valid_access(int off, int size, 
>> enum bpf_access_type type,
>> +                        const struct bpf_prog *prog,
>> +                        struct bpf_insn_access_aux *info)
>> +{
>> +    if (!bpf_tracing_btf_ctx_access(off, size, type, prog, info))
>> +        return false;
>> +
>> +    /* promote it to smc_sock */
>> +    if (base_type(info->reg_type) == PTR_TO_BTF_ID &&
>> +        !bpf_type_has_unsafe_modifiers(info->reg_type) &&
>> +        info->btf_id == sock_id)
>> +        info->btf_id = smc_sock_id;
>> +
>> +    return true;
>> +}
>> +
>> +static const struct bpf_verifier_ops bpf_smc_negotiator_verifier_ops 
>> = {
>> +    .get_func_proto  = smc_negotiator_prog_func_proto,
>> +    .is_valid_access = smc_negotiator_prog_is_valid_access,
>> +};
>> +
>> +struct bpf_struct_ops bpf_smc_sock_negotiator_ops = {
>> +    .verifier_ops = &bpf_smc_negotiator_verifier_ops,
>> +    .init = bpf_smc_negotiator_init,
>> +    .check_member = bpf_smc_negotiator_check_member,
>> +    .init_member = bpf_smc_negotiator_init_member,
>> +    .reg = bpf_smc_negotiator_reg,
>> +    .update = bpf_smc_negotiator_update,
>> +    .unreg = bpf_smc_negotiator_unreg,
>> +    .validate = bpf_smc_negotiator_validate,
>> +    .name = "smc_sock_negotiator_ops",
>> +};
>> \ No newline at end of file
>
> Empty line at the end?

Will fix that, thanks.

Re: [PATCH bpf-next v1 2/5] net/smc: allow smc to negotiate protocols on policies

[Martin KaFai Lau]

On 5/11/23 11:24 PM, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> As we all know, the SMC protocol is not suitable for all scenarios,
> especially for short-lived. However, for most applications, they cannot
> guarantee that there are no such scenarios at all. Therefore, apps
> may need some specific strategies to decide shall we need to use SMC
> or not.
> 
> Just like the congestion control implementation in TCP, this patch
> provides a generic negotiator implementation. If necessary,
> we can provide different protocol negotiation strategies for
> apps based on this implementation.
> 
> But most importantly, this patch provides the possibility of
> eBPF injection, allowing users to implement their own protocol
> negotiation policy in userspace.
> 
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> ---
>   include/net/smc.h        |  32 +++++++++++
>   net/Makefile             |   1 +
>   net/smc/Kconfig          |  11 ++++
>   net/smc/af_smc.c         | 134 ++++++++++++++++++++++++++++++++++++++++++++++-
>   net/smc/smc_negotiator.c | 119 +++++++++++++++++++++++++++++++++++++++++
>   net/smc/smc_negotiator.h | 116 ++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 412 insertions(+), 1 deletion(-)
>   create mode 100644 net/smc/smc_negotiator.c
>   create mode 100644 net/smc/smc_negotiator.h
> 
> diff --git a/include/net/smc.h b/include/net/smc.h
> index 6d076f5..191061c 100644
> --- a/include/net/smc.h
> +++ b/include/net/smc.h
> @@ -296,6 +296,8 @@ struct smc_sock {				/* smc sock container */
>   	atomic_t                queued_smc_hs;  /* queued smc handshakes */
>   	struct inet_connection_sock_af_ops		af_ops;
>   	const struct inet_connection_sock_af_ops	*ori_af_ops;
> +	/* protocol negotiator ops */
> +	const struct smc_sock_negotiator_ops *negotiator_ops;
>   						/* original af ops */
>   	int			sockopt_defer_accept;
>   						/* sockopt TCP_DEFER_ACCEPT
> @@ -316,4 +318,34 @@ struct smc_sock {				/* smc sock container */
>   						 */
>   };
>   
> +#ifdef CONFIG_SMC_BPF
> +/* BPF struct ops for smc protocol negotiator */
> +struct smc_sock_negotiator_ops {
> +
> +	struct list_head	list;
> +
> +	/* ops name */
> +	char		name[16];
> +	/* key for name */
> +	u32			key;
> +
> +	/* init with sk */
> +	void (*init)(struct sock *sk);
> +
> +	/* release with sk */
> +	void (*release)(struct sock *sk);
> +
> +	/* advice for negotiate */
> +	int (*negotiate)(struct sock *sk);
> +
> +	/* info gathering timing */
> +	void (*collect_info)(struct sock *sk, int timing);
> +
> +	/* module owner */
> +	struct module *owner;
> +};
> +#else
> +struct smc_sock_negotiator_ops {};
> +#endif
> +
>   #endif	/* _SMC_H */
> diff --git a/net/Makefile b/net/Makefile
> index 4c4dc53..222916a 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_TIPC)		+= tipc/
>   obj-$(CONFIG_NETLABEL)		+= netlabel/
>   obj-$(CONFIG_IUCV)		+= iucv/
>   obj-$(CONFIG_SMC)		+= smc/
> +obj-$(CONFIG_SMC_BPF)		+= smc/smc_negotiator.o >   obj-$(CONFIG_RFKILL)		+= rfkill/
>   obj-$(CONFIG_NET_9P)		+= 9p/
>   obj-$(CONFIG_CAIF)		+= caif/
> diff --git a/net/smc/Kconfig b/net/smc/Kconfig
> index 1ab3c5a..bdcc9f1 100644
> --- a/net/smc/Kconfig
> +++ b/net/smc/Kconfig
> @@ -19,3 +19,14 @@ config SMC_DIAG
>   	  smcss.
>   
>   	  if unsure, say Y.
> +
> +config SMC_BPF
> +	bool "SMC: support eBPF" if SMC


so smc_negotiator will always be in the kernel image even af_smc is compiled as 
a module? If the SMC_BPF needs to support af_smc as a module, proper 
implementation needs to be added to bpf_struct_ops to support module first. It 
is work-in-progress.

> +	depends on BPF_SYSCALL
> +	default n
> +	help
> +	  Supports eBPF to allows user mode participation in SMC's protocol process
> +	  via ebpf programs. Alternatively, obtain information about the SMC socks
> +	  through the ebpf program.
> +
> +	  If unsure, say N.
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 50c38b6..7406fd4 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -52,6 +52,7 @@
>   #include "smc_close.h"
>   #include "smc_stats.h"
>   #include "smc_tracepoint.h"
> +#include "smc_negotiator.h"
>   #include "smc_sysctl.h"
>   
>   static DEFINE_MUTEX(smc_server_lgr_pending);	/* serialize link group
> @@ -68,6 +69,119 @@
>   static void smc_tcp_listen_work(struct work_struct *);
>   static void smc_connect_work(struct work_struct *);
>   
> +#ifdef CONFIG_SMC_BPF
> +
> +/* Check if sock should use smc */
> +int smc_sock_should_select_smc(const struct smc_sock *smc)
> +{
> +	const struct smc_sock_negotiator_ops *ops;
> +	int ret;
> +
> +	rcu_read_lock();
> +	ops = READ_ONCE(smc->negotiator_ops);
> +
> +	/* No negotiator_ops supply or no negotiate func set,
> +	 * always pass it.
> +	 */
> +	if (!ops || !ops->negotiate) {

A smc_sock_negotiator_ops without ->negotiate? Is it useful at all to allow the 
register in the first place?

> +		rcu_read_unlock();
> +		return SK_PASS;
> +	}
> +
> +	ret = ops->negotiate((struct sock *)&smc->sk);
> +	rcu_read_unlock();
> +	return ret;
> +}
> +
> +void smc_sock_perform_collecting_info(const struct smc_sock *smc, int timing)
> +{
> +	const struct smc_sock_negotiator_ops *ops;
> +
> +	rcu_read_lock();
> +	ops = READ_ONCE(smc->negotiator_ops);
> +
> +	if (!ops || !ops->collect_info) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +
> +	ops->collect_info((struct sock *)&smc->sk, timing);
> +	rcu_read_unlock();
> +}
> +
> +int smc_sock_assign_negotiator_ops(struct smc_sock *smc, const char *name)
> +{
> +	struct smc_sock_negotiator_ops *ops;
> +	int ret = -EINVAL;
> +
> +	/* already set */
> +	if (READ_ONCE(smc->negotiator_ops))
> +		smc_sock_cleanup_negotiator_ops(smc, /* might be still referenced */ false);
> +
> +	/* Just for clear negotiator_ops */
> +	if (!name || !strlen(name))
> +		return 0;
> +
> +	rcu_read_lock();
> +	ops = smc_negotiator_ops_get_by_name(name);
> +	if (likely(ops)) {
> +		if (unlikely(!bpf_try_module_get(ops, ops->owner))) {
> +			ret = -EACCES;
> +		} else {
> +			WRITE_ONCE(smc->negotiator_ops, ops);
> +			/* make sure ops can be seen */
> +			smp_wmb();

This rcu_read_lock(), WRITE_ONCE, and smp_wmb() combo looks very suspicious. 
smc->negotiator_ops is protected by rcu (+refcnt) or lock_sock()?

I am going to stop reviewing here.

> +			if (ops->init)
> +				ops->init(&smc->sk);
> +			ret = 0;
> +		}
> +	}
> +	rcu_read_unlock();
> +	return ret;
> +}
> +
> +void smc_sock_cleanup_negotiator_ops(struct smc_sock *smc, bool no_more)
> +{
> +	const struct smc_sock_negotiator_ops *ops;
> +
> +	ops = READ_ONCE(smc->negotiator_ops);
> +
> +	/* not all smc sock has negotiator_ops */
> +	if (!ops)
> +		return;
> +
> +	might_sleep();
> +
> +	/* Just ensure data integrity */
> +	WRITE_ONCE(smc->negotiator_ops, NULL);
> +	/* make sure NULL can be seen */
> +	smp_wmb();
> +	/* if the socks may have references to the negotiator ops to be removed.
> +	 * it means that we might need to wait for the readers of ops
> +	 * to complete. It's slow though.
> +	 */
> +	if (unlikely(!no_more))
> +		synchronize_rcu();
> +	if (ops->release)
> +		ops->release(&smc->sk);
> +	bpf_module_put(ops, ops->owner);
> +}
> +
> +void smc_sock_clone_negotiator_ops(struct sock *parent, struct sock *child)
> +{
> +	const struct smc_sock_negotiator_ops *ops;
> +
> +	rcu_read_lock();
> +	ops = READ_ONCE(smc_sk(parent)->negotiator_ops);
> +	if (ops && bpf_try_module_get(ops, ops->owner)) {
> +		smc_sk(child)->negotiator_ops = ops;
> +		if (ops->init)
> +			ops->init(child);
> +	}
> +	rcu_read_unlock();
> +}
> +#endif
> +
>   int smc_nl_dump_hs_limitation(struct sk_buff *skb, struct netlink_callback *cb)
>   {
>   	struct smc_nl_dmp_ctx *cb_ctx = smc_nl_dmp_ctx(cb);
> @@ -166,6 +280,9 @@ static bool smc_hs_congested(const struct sock *sk)
>   	if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
>   		return true;
>   
> +	if (!smc_sock_should_select_smc(smc))
> +		return true;
> +
>   	return false;
>   }
>   
> @@ -320,6 +437,9 @@ static int smc_release(struct socket *sock)
>   	sock_hold(sk); /* sock_put below */
>   	smc = smc_sk(sk);
>   
> +	/* trigger info gathering if needed.*/
> +	smc_sock_perform_collecting_info(smc, SMC_SOCK_CLOSED_TIMING);
> +
>   	old_state = sk->sk_state;
>   
>   	/* cleanup for a dangling non-blocking connect */
> @@ -356,6 +476,9 @@ static int smc_release(struct socket *sock)
>   
>   static void smc_destruct(struct sock *sk)
>   {
> +	/* cleanup negotiator_ops if set */
> +	smc_sock_cleanup_negotiator_ops(smc_sk(sk), /* no longer used */ true);
> +
>   	if (sk->sk_state != SMC_CLOSED)
>   		return;
>   	if (!sock_flag(sk, SOCK_DEAD))
> @@ -1627,7 +1750,14 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
>   	}
>   
>   	smc_copy_sock_settings_to_clc(smc);
> -	tcp_sk(smc->clcsock->sk)->syn_smc = 1;
> +	/* accept out connection as SMC connection */
> +	if (smc_sock_should_select_smc(smc) == SK_PASS) {
> +		tcp_sk(smc->clcsock->sk)->syn_smc = 1;
> +	} else {
> +		tcp_sk(smc->clcsock->sk)->syn_smc = 0;
> +		smc_switch_to_fallback(smc, /* active fallback */ 0);
> +	}
> +
>   	if (smc->connect_nonblock) {
>   		rc = -EALREADY;
>   		goto out;
> @@ -1679,6 +1809,8 @@ static int smc_clcsock_accept(struct smc_sock *lsmc, struct smc_sock **new_smc)
>   	}
>   	*new_smc = smc_sk(new_sk);
>   
> +	smc_sock_clone_negotiator_ops(lsk, new_sk);
> +
>   	mutex_lock(&lsmc->clcsock_release_lock);
>   	if (lsmc->clcsock)
>   		rc = kernel_accept(lsmc->clcsock, &new_clcsock, SOCK_NONBLOCK);

Re: [PATCH bpf-next v1 2/5] net/smc: allow smc to negotiate protocols on policies

[D. Wythe]

On 5/16/23 6:52 AM, Martin KaFai Lau wrote:
> On 5/11/23 11:24 PM, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> As we all know, the SMC protocol is not suitable for all scenarios,
>> especially for short-lived. However, for most applications, they cannot
>> guarantee that there are no such scenarios at all. Therefore, apps
>> may need some specific strategies to decide shall we need to use SMC
>> or not.
>>
>> Just like the congestion control implementation in TCP, this patch
>> provides a generic negotiator implementation. If necessary,
>> we can provide different protocol negotiation strategies for
>> apps based on this implementation.
>>
>> But most importantly, this patch provides the possibility of
>> eBPF injection, allowing users to implement their own protocol
>> negotiation policy in userspace.
>>
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>>   include/net/smc.h        |  32 +++++++++++
>>   net/Makefile             |   1 +
>>   net/smc/Kconfig          |  11 ++++
>>   net/smc/af_smc.c         | 134 
>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>   net/smc/smc_negotiator.c | 119 
>> +++++++++++++++++++++++++++++++++++++++++
>>   net/smc/smc_negotiator.h | 116 
>> ++++++++++++++++++++++++++++++++++++++++
>>   6 files changed, 412 insertions(+), 1 deletion(-)
>>   create mode 100644 net/smc/smc_negotiator.c
>>   create mode 100644 net/smc/smc_negotiator.h
>>
>> diff --git a/include/net/smc.h b/include/net/smc.h
>> index 6d076f5..191061c 100644
>> --- a/include/net/smc.h
>> +++ b/include/net/smc.h
>> @@ -296,6 +296,8 @@ struct smc_sock {                /* smc sock 
>> container */
>>       atomic_t                queued_smc_hs;  /* queued smc 
>> handshakes */
>>       struct inet_connection_sock_af_ops        af_ops;
>>       const struct inet_connection_sock_af_ops    *ori_af_ops;
>> +    /* protocol negotiator ops */
>> +    const struct smc_sock_negotiator_ops *negotiator_ops;
>>                           /* original af ops */
>>       int            sockopt_defer_accept;
>>                           /* sockopt TCP_DEFER_ACCEPT
>> @@ -316,4 +318,34 @@ struct smc_sock {                /* smc sock 
>> container */
>>                            */
>>   };
>>   +#ifdef CONFIG_SMC_BPF
>> +/* BPF struct ops for smc protocol negotiator */
>> +struct smc_sock_negotiator_ops {
>> +
>> +    struct list_head    list;
>> +
>> +    /* ops name */
>> +    char        name[16];
>> +    /* key for name */
>> +    u32            key;
>> +
>> +    /* init with sk */
>> +    void (*init)(struct sock *sk);
>> +
>> +    /* release with sk */
>> +    void (*release)(struct sock *sk);
>> +
>> +    /* advice for negotiate */
>> +    int (*negotiate)(struct sock *sk);
>> +
>> +    /* info gathering timing */
>> +    void (*collect_info)(struct sock *sk, int timing);
>> +
>> +    /* module owner */
>> +    struct module *owner;
>> +};
>> +#else
>> +struct smc_sock_negotiator_ops {};
>> +#endif
>> +
>>   #endif    /* _SMC_H */
>> diff --git a/net/Makefile b/net/Makefile
>> index 4c4dc53..222916a 100644
>> --- a/net/Makefile
>> +++ b/net/Makefile
>> @@ -52,6 +52,7 @@ obj-$(CONFIG_TIPC)        += tipc/
>>   obj-$(CONFIG_NETLABEL)        += netlabel/
>>   obj-$(CONFIG_IUCV)        += iucv/
>>   obj-$(CONFIG_SMC)        += smc/
>> +obj-$(CONFIG_SMC_BPF)        += smc/smc_negotiator.o > 
>> obj-$(CONFIG_RFKILL)        += rfkill/
>>   obj-$(CONFIG_NET_9P)        += 9p/
>>   obj-$(CONFIG_CAIF)        += caif/
>> diff --git a/net/smc/Kconfig b/net/smc/Kconfig
>> index 1ab3c5a..bdcc9f1 100644
>> --- a/net/smc/Kconfig
>> +++ b/net/smc/Kconfig
>> @@ -19,3 +19,14 @@ config SMC_DIAG
>>         smcss.
>>           if unsure, say Y.
>> +
>> +config SMC_BPF
>> +    bool "SMC: support eBPF" if SMC
>
>
> so smc_negotiator will always be in the kernel image even af_smc is 
> compiled as a module? If the SMC_BPF needs to support af_smc as a 
> module, proper implementation needs to be added to bpf_struct_ops to 
> support module first. It is work-in-progress.
>

smc_negotiator will not no in the kernel image when af_smc is compiled 
as a module,
it's requires config SMC_BPF also sets to be Y,  while it's default to 
be N. That's is,
even if af_smc is compiled as a module but with no SMC_BPF set, 
smc_negotiator
doesn't exist anywhere.

>> +    depends on BPF_SYSCALL
>> +    default n
>> +    help
>> +      Supports eBPF to allows user mode participation in SMC's 
>> protocol process
>> +      via ebpf programs. Alternatively, obtain information about the 
>> SMC socks
>> +      through the ebpf program.
>> +
>> +      If unsure, say N.
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 50c38b6..7406fd4 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -52,6 +52,7 @@
>>   #include "smc_close.h"
>>   #include "smc_stats.h"
>>   #include "smc_tracepoint.h"
>> +#include "smc_negotiator.h"
>>   #include "smc_sysctl.h"
>>     static DEFINE_MUTEX(smc_server_lgr_pending);    /* serialize link 
>> group
>> @@ -68,6 +69,119 @@
>>   static void smc_tcp_listen_work(struct work_struct *);
>>   static void smc_connect_work(struct work_struct *);
>>   +#ifdef CONFIG_SMC_BPF
>> +
>> +/* Check if sock should use smc */
>> +int smc_sock_should_select_smc(const struct smc_sock *smc)
>> +{
>> +    const struct smc_sock_negotiator_ops *ops;
>> +    int ret;
>> +
>> +    rcu_read_lock();
>> +    ops = READ_ONCE(smc->negotiator_ops);
>> +
>> +    /* No negotiator_ops supply or no negotiate func set,
>> +     * always pass it.
>> +     */
>> +    if (!ops || !ops->negotiate) {
>
> A smc_sock_negotiator_ops without ->negotiate? Is it useful at all to 
> allow the register in the first place?
>

You are right, this can be avoid before registration. I'll fix it.

>> +        rcu_read_unlock();
>> +        return SK_PASS;
>> +    }
>> +
>> +    ret = ops->negotiate((struct sock *)&smc->sk);
>> +    rcu_read_unlock();
>> +    return ret;
>> +}
>> +
>> +void smc_sock_perform_collecting_info(const struct smc_sock *smc, 
>> int timing)
>> +{
>> +    const struct smc_sock_negotiator_ops *ops;
>> +
>> +    rcu_read_lock();
>> +    ops = READ_ONCE(smc->negotiator_ops);
>> +
>> +    if (!ops || !ops->collect_info) {
>> +        rcu_read_unlock();
>> +        return;
>> +    }
>> +
>> +    ops->collect_info((struct sock *)&smc->sk, timing);
>> +    rcu_read_unlock();
>> +}
>> +
>> +int smc_sock_assign_negotiator_ops(struct smc_sock *smc, const char 
>> *name)
>> +{
>> +    struct smc_sock_negotiator_ops *ops;
>> +    int ret = -EINVAL;
>> +
>> +    /* already set */
>> +    if (READ_ONCE(smc->negotiator_ops))
>> +        smc_sock_cleanup_negotiator_ops(smc, /* might be still 
>> referenced */ false);
>> +
>> +    /* Just for clear negotiator_ops */
>> +    if (!name || !strlen(name))
>> +        return 0;
>> +
>> +    rcu_read_lock();
>> +    ops = smc_negotiator_ops_get_by_name(name);
>> +    if (likely(ops)) {
>> +        if (unlikely(!bpf_try_module_get(ops, ops->owner))) {
>> +            ret = -EACCES;
>> +        } else {
>> +            WRITE_ONCE(smc->negotiator_ops, ops);
>> +            /* make sure ops can be seen */
>> +            smp_wmb();
>
> This rcu_read_lock(), WRITE_ONCE, and smp_wmb() combo looks very 
> suspicious. smc->negotiator_ops is protected by rcu (+refcnt) or 
> lock_sock()?
>

All access to ops is protected by RCU, and there are no lock_sock. 
WRITE_ONCE() and smp_wmb() do
not participate in any guarantee of the availability of ops,  The 
purpose to using them is just wish the latest values
can be read as soon as possible , In fact, even if old value is read, 
there will be no problem in logic because all updates
will do synchronize_rcu() and all access to ops is under in rcu_read_lock().

> I am going to stop reviewing here.
>

Hoping my explanation can answer your questions and still looking forward to
your more feedback 😁.

Best wishes.
D. Wythe

>> +            if (ops->init)
>> +                ops->init(&smc->sk);
>> +            ret = 0;
>> +        }
>> +    }
>> +    rcu_read_unlock();
>> +    return ret;
>> +}
>> +
>> +void smc_sock_cleanup_negotiator_ops(struct smc_sock *smc, bool 
>> no_more)
>> +{
>> +    const struct smc_sock_negotiator_ops *ops;
>> +
>> +    ops = READ_ONCE(smc->negotiator_ops);
>> +
>> +    /* not all smc sock has negotiator_ops */
>> +    if (!ops)
>> +        return;
>> +
>> +    might_sleep();
>> +
>> +    /* Just ensure data integrity */
>> +    WRITE_ONCE(smc->negotiator_ops, NULL);
>> +    /* make sure NULL can be seen */
>> +    smp_wmb();
>> +    /* if the socks may have references to the negotiator ops to be 
>> removed.
>> +     * it means that we might need to wait for the readers of ops
>> +     * to complete. It's slow though.
>> +     */
>> +    if (unlikely(!no_more))
>> +        synchronize_rcu();
>> +    if (ops->release)
>> +        ops->release(&smc->sk);
>> +    bpf_module_put(ops, ops->owner);
>> +}
>> +
>> +void smc_sock_clone_negotiator_ops(struct sock *parent, struct sock 
>> *child)
>> +{
>> +    const struct smc_sock_negotiator_ops *ops;
>> +
>> +    rcu_read_lock();
>> +    ops = READ_ONCE(smc_sk(parent)->negotiator_ops);
>> +    if (ops && bpf_try_module_get(ops, ops->owner)) {
>> +        smc_sk(child)->negotiator_ops = ops;
>> +        if (ops->init)
>> +            ops->init(child);
>> +    }
>> +    rcu_read_unlock();
>> +}
>> +#endif
>> +
>>   int smc_nl_dump_hs_limitation(struct sk_buff *skb, struct 
>> netlink_callback *cb)
>>   {
>>       struct smc_nl_dmp_ctx *cb_ctx = smc_nl_dmp_ctx(cb);
>> @@ -166,6 +280,9 @@ static bool smc_hs_congested(const struct sock *sk)
>>       if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
>>           return true;
>>   +    if (!smc_sock_should_select_smc(smc))
>> +        return true;
>> +
>>       return false;
>>   }
>>   @@ -320,6 +437,9 @@ static int smc_release(struct socket *sock)
>>       sock_hold(sk); /* sock_put below */
>>       smc = smc_sk(sk);
>>   +    /* trigger info gathering if needed.*/
>> +    smc_sock_perform_collecting_info(smc, SMC_SOCK_CLOSED_TIMING);
>> +
>>       old_state = sk->sk_state;
>>         /* cleanup for a dangling non-blocking connect */
>> @@ -356,6 +476,9 @@ static int smc_release(struct socket *sock)
>>     static void smc_destruct(struct sock *sk)
>>   {
>> +    /* cleanup negotiator_ops if set */
>> +    smc_sock_cleanup_negotiator_ops(smc_sk(sk), /* no longer used */ 
>> true);
>> +
>>       if (sk->sk_state != SMC_CLOSED)
>>           return;
>>       if (!sock_flag(sk, SOCK_DEAD))
>> @@ -1627,7 +1750,14 @@ static int smc_connect(struct socket *sock, 
>> struct sockaddr *addr,
>>       }
>>         smc_copy_sock_settings_to_clc(smc);
>> -    tcp_sk(smc->clcsock->sk)->syn_smc = 1;
>> +    /* accept out connection as SMC connection */
>> +    if (smc_sock_should_select_smc(smc) == SK_PASS) {
>> +        tcp_sk(smc->clcsock->sk)->syn_smc = 1;
>> +    } else {
>> +        tcp_sk(smc->clcsock->sk)->syn_smc = 0;
>> +        smc_switch_to_fallback(smc, /* active fallback */ 0);
>> +    }
>> +
>>       if (smc->connect_nonblock) {
>>           rc = -EALREADY;
>>           goto out;
>> @@ -1679,6 +1809,8 @@ static int smc_clcsock_accept(struct smc_sock 
>> *lsmc, struct smc_sock **new_smc)
>>       }
>>       *new_smc = smc_sk(new_sk);
>>   +    smc_sock_clone_negotiator_ops(lsk, new_sk);
>> +
>>       mutex_lock(&lsmc->clcsock_release_lock);
>>       if (lsmc->clcsock)
>>           rc = kernel_accept(lsmc->clcsock, &new_clcsock, 
>> SOCK_NONBLOCK);
>

Re: [PATCH bpf-next v1 2/5] net/smc: allow smc to negotiate protocols on policies

[Martin KaFai Lau]

On 5/17/23 12:08 AM, D. Wythe wrote:
> 
> 
> On 5/16/23 6:52 AM, Martin KaFai Lau wrote:
>> On 5/11/23 11:24 PM, D. Wythe wrote:
>>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>>
>>> As we all know, the SMC protocol is not suitable for all scenarios,
>>> especially for short-lived. However, for most applications, they cannot
>>> guarantee that there are no such scenarios at all. Therefore, apps
>>> may need some specific strategies to decide shall we need to use SMC
>>> or not.
>>>
>>> Just like the congestion control implementation in TCP, this patch
>>> provides a generic negotiator implementation. If necessary,
>>> we can provide different protocol negotiation strategies for
>>> apps based on this implementation.
>>>
>>> But most importantly, this patch provides the possibility of
>>> eBPF injection, allowing users to implement their own protocol
>>> negotiation policy in userspace.
>>>
>>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>>> ---
>>>   include/net/smc.h        |  32 +++++++++++
>>>   net/Makefile             |   1 +
>>>   net/smc/Kconfig          |  11 ++++
>>>   net/smc/af_smc.c         | 134 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>   net/smc/smc_negotiator.c | 119 +++++++++++++++++++++++++++++++++++++++++
>>>   net/smc/smc_negotiator.h | 116 ++++++++++++++++++++++++++++++++++++++++
>>>   6 files changed, 412 insertions(+), 1 deletion(-)
>>>   create mode 100644 net/smc/smc_negotiator.c
>>>   create mode 100644 net/smc/smc_negotiator.h
>>>
>>> diff --git a/include/net/smc.h b/include/net/smc.h
>>> index 6d076f5..191061c 100644
>>> --- a/include/net/smc.h
>>> +++ b/include/net/smc.h
>>> @@ -296,6 +296,8 @@ struct smc_sock {                /* smc sock container */
>>>       atomic_t                queued_smc_hs;  /* queued smc handshakes */
>>>       struct inet_connection_sock_af_ops        af_ops;
>>>       const struct inet_connection_sock_af_ops    *ori_af_ops;
>>> +    /* protocol negotiator ops */
>>> +    const struct smc_sock_negotiator_ops *negotiator_ops;
>>>                           /* original af ops */
>>>       int            sockopt_defer_accept;
>>>                           /* sockopt TCP_DEFER_ACCEPT
>>> @@ -316,4 +318,34 @@ struct smc_sock {                /* smc sock container */
>>>                            */
>>>   };
>>>   +#ifdef CONFIG_SMC_BPF
>>> +/* BPF struct ops for smc protocol negotiator */
>>> +struct smc_sock_negotiator_ops {
>>> +
>>> +    struct list_head    list;
>>> +
>>> +    /* ops name */
>>> +    char        name[16];
>>> +    /* key for name */
>>> +    u32            key;
>>> +
>>> +    /* init with sk */
>>> +    void (*init)(struct sock *sk);
>>> +
>>> +    /* release with sk */
>>> +    void (*release)(struct sock *sk);
>>> +
>>> +    /* advice for negotiate */
>>> +    int (*negotiate)(struct sock *sk);
>>> +
>>> +    /* info gathering timing */
>>> +    void (*collect_info)(struct sock *sk, int timing);
>>> +
>>> +    /* module owner */
>>> +    struct module *owner;
>>> +};
>>> +#else
>>> +struct smc_sock_negotiator_ops {};
>>> +#endif
>>> +
>>>   #endif    /* _SMC_H */
>>> diff --git a/net/Makefile b/net/Makefile
>>> index 4c4dc53..222916a 100644
>>> --- a/net/Makefile
>>> +++ b/net/Makefile
>>> @@ -52,6 +52,7 @@ obj-$(CONFIG_TIPC)        += tipc/
>>>   obj-$(CONFIG_NETLABEL)        += netlabel/
>>>   obj-$(CONFIG_IUCV)        += iucv/
>>>   obj-$(CONFIG_SMC)        += smc/
>>> +obj-$(CONFIG_SMC_BPF)        += smc/smc_negotiator.o > 
>>> obj-$(CONFIG_RFKILL)        += rfkill/
>>>   obj-$(CONFIG_NET_9P)        += 9p/
>>>   obj-$(CONFIG_CAIF)        += caif/
>>> diff --git a/net/smc/Kconfig b/net/smc/Kconfig
>>> index 1ab3c5a..bdcc9f1 100644
>>> --- a/net/smc/Kconfig
>>> +++ b/net/smc/Kconfig
>>> @@ -19,3 +19,14 @@ config SMC_DIAG
>>>         smcss.
>>>           if unsure, say Y.
>>> +
>>> +config SMC_BPF
>>> +    bool "SMC: support eBPF" if SMC
>>
>>
>> so smc_negotiator will always be in the kernel image even af_smc is compiled 
>> as a module? If the SMC_BPF needs to support af_smc as a module, proper 
>> implementation needs to be added to bpf_struct_ops to support module first. It 
>> is work-in-progress.
>>
> 
> smc_negotiator will not no in the kernel image when af_smc is compiled as a module,
> it's requires config SMC_BPF also sets to be Y,  while it's default to be N. 
> That's is,
> even if af_smc is compiled as a module but with no SMC_BPF set, smc_negotiator
> doesn't exist anywhere.

CONFIG_SMC_BPF could be "y" while CONFIG_SMC is "m", no?

Anyway, there is a build error when CONFIG_SMC is "m" :(

> 
>>> +    depends on BPF_SYSCALL
>>> +    default n
>>> +    help
>>> +      Supports eBPF to allows user mode participation in SMC's protocol process
>>> +      via ebpf programs. Alternatively, obtain information about the SMC socks
>>> +      through the ebpf program.
>>> +
>>> +      If unsure, say N.
>>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>>> index 50c38b6..7406fd4 100644
>>> --- a/net/smc/af_smc.c
>>> +++ b/net/smc/af_smc.c
>>> @@ -52,6 +52,7 @@
>>>   #include "smc_close.h"
>>>   #include "smc_stats.h"
>>>   #include "smc_tracepoint.h"
>>> +#include "smc_negotiator.h"
>>>   #include "smc_sysctl.h"
>>>     static DEFINE_MUTEX(smc_server_lgr_pending);    /* serialize link group
>>> @@ -68,6 +69,119 @@
>>>   static void smc_tcp_listen_work(struct work_struct *);
>>>   static void smc_connect_work(struct work_struct *);
>>>   +#ifdef CONFIG_SMC_BPF
>>> +
>>> +/* Check if sock should use smc */
>>> +int smc_sock_should_select_smc(const struct smc_sock *smc)
>>> +{
>>> +    const struct smc_sock_negotiator_ops *ops;
>>> +    int ret;
>>> +
>>> +    rcu_read_lock();
>>> +    ops = READ_ONCE(smc->negotiator_ops);
>>> +
>>> +    /* No negotiator_ops supply or no negotiate func set,
>>> +     * always pass it.
>>> +     */
>>> +    if (!ops || !ops->negotiate) {
>>
>> A smc_sock_negotiator_ops without ->negotiate? Is it useful at all to allow 
>> the register in the first place?
>>
> 
> You are right, this can be avoid before registration. I'll fix it.
> 
>>> +        rcu_read_unlock();
>>> +        return SK_PASS;
>>> +    }
>>> +
>>> +    ret = ops->negotiate((struct sock *)&smc->sk);
>>> +    rcu_read_unlock();
>>> +    return ret;
>>> +}
>>> +
>>> +void smc_sock_perform_collecting_info(const struct smc_sock *smc, int timing)
>>> +{
>>> +    const struct smc_sock_negotiator_ops *ops;
>>> +
>>> +    rcu_read_lock();
>>> +    ops = READ_ONCE(smc->negotiator_ops);
>>> +
>>> +    if (!ops || !ops->collect_info) {
>>> +        rcu_read_unlock();
>>> +        return;
>>> +    }
>>> +
>>> +    ops->collect_info((struct sock *)&smc->sk, timing);
>>> +    rcu_read_unlock();
>>> +}
>>> +
>>> +int smc_sock_assign_negotiator_ops(struct smc_sock *smc, const char *name)
>>> +{
>>> +    struct smc_sock_negotiator_ops *ops;
>>> +    int ret = -EINVAL;
>>> +
>>> +    /* already set */
>>> +    if (READ_ONCE(smc->negotiator_ops))
>>> +        smc_sock_cleanup_negotiator_ops(smc, /* might be still referenced */ 
>>> false);
>>> +
>>> +    /* Just for clear negotiator_ops */
>>> +    if (!name || !strlen(name))
>>> +        return 0;
>>> +
>>> +    rcu_read_lock();
>>> +    ops = smc_negotiator_ops_get_by_name(name);
>>> +    if (likely(ops)) {
>>> +        if (unlikely(!bpf_try_module_get(ops, ops->owner))) {
>>> +            ret = -EACCES;
>>> +        } else {
>>> +            WRITE_ONCE(smc->negotiator_ops, ops);
>>> +            /* make sure ops can be seen */
>>> +            smp_wmb();
>>
>> This rcu_read_lock(), WRITE_ONCE, and smp_wmb() combo looks very suspicious. 
>> smc->negotiator_ops is protected by rcu (+refcnt) or lock_sock()?
>>
> 
> All access to ops is protected by RCU, and there are no lock_sock. WRITE_ONCE() 
> and smp_wmb() do
> not participate in any guarantee of the availability of ops,  The purpose to 
> using them is just wish the latest values
> can be read as soon as possible , In fact, even if old value is read, there will 
> be no problem in logic because all updates
> will do synchronize_rcu() and all access to ops is under in rcu_read_lock().

The explanation is not encouraging. No clear benefit while having this kind of 
complexity here. Switching tcp congestion ops also does not require this. Some 
of the new codes is in af_smc but bpf is the primary user. It is not something 
that I would like to maintain and then need to reason about this unusual pattern 
a year later. Beside, this negotiator_ops assignment must be done under a 
lock_sock(). The same probably is true for calling ops->negotiate() where the 
bpf prog may be looking at the sk and calling bpf_setsockopt.

> 
>> I am going to stop reviewing here.
>>
> 
> Hoping my explanation can answer your questions and still looking forward to
> your more feedback 😁.

Sorry, based on the review so far (there was some RFC before), it is not 
something that I want to continue to review and maintain a bpf hook for it. You 
have to solicit other known community members for review and sponsor this set 
from now on.

Re: [PATCH bpf-next v1 2/5] net/smc: allow smc to negotiate protocols on policies

[D. Wythe]

On 5/17/23 4:14 PM, Martin KaFai Lau wrote:
> On 5/17/23 12:08 AM, D. Wythe wrote:
>>
>>
>> On 5/16/23 6:52 AM, Martin KaFai Lau wrote:
>>> On 5/11/23 11:24 PM, D. Wythe wrote:
>>>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>>>
>>>> As we all know, the SMC protocol is not suitable for all scenarios,
>>>> especially for short-lived. However, for most applications, they 
>>>> cannot
>>>> guarantee that there are no such scenarios at all. Therefore, apps
>>>> may need some specific strategies to decide shall we need to use SMC
>>>> or not.
>>>>
>>>> Just like the congestion control implementation in TCP, this patch
>>>> provides a generic negotiator implementation. If necessary,
>>>> we can provide different protocol negotiation strategies for
>>>> apps based on this implementation.
>>>>
>>>> But most importantly, this patch provides the possibility of
>>>> eBPF injection, allowing users to implement their own protocol
>>>> negotiation policy in userspace.
>>>>
>>>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>>>> ---
>>>>   include/net/smc.h        |  32 +++++++++++
>>>>   net/Makefile             |   1 +
>>>>   net/smc/Kconfig          |  11 ++++
>>>>   net/smc/af_smc.c         | 134 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>>>   net/smc/smc_negotiator.c | 119 
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>   net/smc/smc_negotiator.h | 116 
>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>   6 files changed, 412 insertions(+), 1 deletion(-)
>>>>   create mode 100644 net/smc/smc_negotiator.c
>>>>   create mode 100644 net/smc/smc_negotiator.h
>>>>
>>>> diff --git a/include/net/smc.h b/include/net/smc.h
>>>> index 6d076f5..191061c 100644
>>>> --- a/include/net/smc.h
>>>> +++ b/include/net/smc.h
>>>> @@ -296,6 +296,8 @@ struct smc_sock {                /* smc sock 
>>>> container */
>>>>       atomic_t                queued_smc_hs;  /* queued smc 
>>>> handshakes */
>>>>       struct inet_connection_sock_af_ops        af_ops;
>>>>       const struct inet_connection_sock_af_ops *ori_af_ops;
>>>> +    /* protocol negotiator ops */
>>>> +    const struct smc_sock_negotiator_ops *negotiator_ops;
>>>>                           /* original af ops */
>>>>       int            sockopt_defer_accept;
>>>>                           /* sockopt TCP_DEFER_ACCEPT
>>>> @@ -316,4 +318,34 @@ struct smc_sock {                /* smc sock 
>>>> container */
>>>>                            */
>>>>   };
>>>>   +#ifdef CONFIG_SMC_BPF
>>>> +/* BPF struct ops for smc protocol negotiator */
>>>> +struct smc_sock_negotiator_ops {
>>>> +
>>>> +    struct list_head    list;
>>>> +
>>>> +    /* ops name */
>>>> +    char        name[16];
>>>> +    /* key for name */
>>>> +    u32            key;
>>>> +
>>>> +    /* init with sk */
>>>> +    void (*init)(struct sock *sk);
>>>> +
>>>> +    /* release with sk */
>>>> +    void (*release)(struct sock *sk);
>>>> +
>>>> +    /* advice for negotiate */
>>>> +    int (*negotiate)(struct sock *sk);
>>>> +
>>>> +    /* info gathering timing */
>>>> +    void (*collect_info)(struct sock *sk, int timing);
>>>> +
>>>> +    /* module owner */
>>>> +    struct module *owner;
>>>> +};
>>>> +#else
>>>> +struct smc_sock_negotiator_ops {};
>>>> +#endif
>>>> +
>>>>   #endif    /* _SMC_H */
>>>> diff --git a/net/Makefile b/net/Makefile
>>>> index 4c4dc53..222916a 100644
>>>> --- a/net/Makefile
>>>> +++ b/net/Makefile
>>>> @@ -52,6 +52,7 @@ obj-$(CONFIG_TIPC)        += tipc/
>>>>   obj-$(CONFIG_NETLABEL)        += netlabel/
>>>>   obj-$(CONFIG_IUCV)        += iucv/
>>>>   obj-$(CONFIG_SMC)        += smc/
>>>> +obj-$(CONFIG_SMC_BPF)        += smc/smc_negotiator.o > 
>>>> obj-$(CONFIG_RFKILL)        += rfkill/
>>>>   obj-$(CONFIG_NET_9P)        += 9p/
>>>>   obj-$(CONFIG_CAIF)        += caif/
>>>> diff --git a/net/smc/Kconfig b/net/smc/Kconfig
>>>> index 1ab3c5a..bdcc9f1 100644
>>>> --- a/net/smc/Kconfig
>>>> +++ b/net/smc/Kconfig
>>>> @@ -19,3 +19,14 @@ config SMC_DIAG
>>>>         smcss.
>>>>           if unsure, say Y.
>>>> +
>>>> +config SMC_BPF
>>>> +    bool "SMC: support eBPF" if SMC
>>>
>>>
>>> so smc_negotiator will always be in the kernel image even af_smc is 
>>> compiled as a module? If the SMC_BPF needs to support af_smc as a 
>>> module, proper implementation needs to be added to bpf_struct_ops to 
>>> support module first. It is work-in-progress.
>>>
>>
>> smc_negotiator will not no in the kernel image when af_smc is 
>> compiled as a module,
>> it's requires config SMC_BPF also sets to be Y,  while it's default 
>> to be N. That's is,
>> even if af_smc is compiled as a module but with no SMC_BPF set, 
>> smc_negotiator
>> doesn't exist anywhere.
>
> CONFIG_SMC_BPF could be "y" while CONFIG_SMC is "m", no?
>
> Anyway, there is a build error when CONFIG_SMC is "m" :(
>

I am curious if users who proactively set CONFIG_SMC_BPF to Y would care 
about the issue you mentioned, while
CONFIG_SMC_BPF defaults to N ?

And I'm really sorry about this compilation error. Last time, I had got 
some comments about symbol export, so I tried to remove some symbol exports,
unfortunately, there are compilation issues when BPF_JIT is set 
(bpf_struct_ops_get is no exported), sorry for my incomplete
testing. I will fix this issue in the new version.

Anyway, if bpf_struct_ops can support module, that would be better, and 
can greatly reduce the trade-offs I make between modules and built-in.
Is there any details can shared on your progress ?

>>>> +    depends on BPF_SYSCALL
>>>> +    default n
>>>> +    help
>>>> +      Supports eBPF to allows user mode participation in SMC's 
>>>> protocol process
>>>> +      via ebpf programs. Alternatively, obtain information about 
>>>> the SMC socks
>>>> +      through the ebpf program.
>>>> +
>>>> +      If unsure, say N.
>>>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>>>> index 50c38b6..7406fd4 100644
>>>> --- a/net/smc/af_smc.c
>>>> +++ b/net/smc/af_smc.c
>>>> @@ -52,6 +52,7 @@
>>>>   #include "smc_close.h"
>>>>   #include "smc_stats.h"
>>>>   #include "smc_tracepoint.h"
>>>> +#include "smc_negotiator.h"
>>>>   #include "smc_sysctl.h"
>>>>     static DEFINE_MUTEX(smc_server_lgr_pending);    /* serialize 
>>>> link group
>>>> @@ -68,6 +69,119 @@
>>>>   static void smc_tcp_listen_work(struct work_struct *);
>>>>   static void smc_connect_work(struct work_struct *);
>>>>   +#ifdef CONFIG_SMC_BPF
>>>> +
>>>> +/* Check if sock should use smc */
>>>> +int smc_sock_should_select_smc(const struct smc_sock *smc)
>>>> +{
>>>> +    const struct smc_sock_negotiator_ops *ops;
>>>> +    int ret;
>>>> +
>>>> +    rcu_read_lock();
>>>> +    ops = READ_ONCE(smc->negotiator_ops);
>>>> +
>>>> +    /* No negotiator_ops supply or no negotiate func set,
>>>> +     * always pass it.
>>>> +     */
>>>> +    if (!ops || !ops->negotiate) {
>>>
>>> A smc_sock_negotiator_ops without ->negotiate? Is it useful at all 
>>> to allow the register in the first place?
>>>
>>
>> You are right, this can be avoid before registration. I'll fix it.
>>
>>>> +        rcu_read_unlock();
>>>> +        return SK_PASS;
>>>> +    }
>>>> +
>>>> +    ret = ops->negotiate((struct sock *)&smc->sk);
>>>> +    rcu_read_unlock();
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +void smc_sock_perform_collecting_info(const struct smc_sock *smc, 
>>>> int timing)
>>>> +{
>>>> +    const struct smc_sock_negotiator_ops *ops;
>>>> +
>>>> +    rcu_read_lock();
>>>> +    ops = READ_ONCE(smc->negotiator_ops);
>>>> +
>>>> +    if (!ops || !ops->collect_info) {
>>>> +        rcu_read_unlock();
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    ops->collect_info((struct sock *)&smc->sk, timing);
>>>> +    rcu_read_unlock();
>>>> +}
>>>> +
>>>> +int smc_sock_assign_negotiator_ops(struct smc_sock *smc, const 
>>>> char *name)
>>>> +{
>>>> +    struct smc_sock_negotiator_ops *ops;
>>>> +    int ret = -EINVAL;
>>>> +
>>>> +    /* already set */
>>>> +    if (READ_ONCE(smc->negotiator_ops))
>>>> +        smc_sock_cleanup_negotiator_ops(smc, /* might be still 
>>>> referenced */ false);
>>>> +
>>>> +    /* Just for clear negotiator_ops */
>>>> +    if (!name || !strlen(name))
>>>> +        return 0;
>>>> +
>>>> +    rcu_read_lock();
>>>> +    ops = smc_negotiator_ops_get_by_name(name);
>>>> +    if (likely(ops)) {
>>>> +        if (unlikely(!bpf_try_module_get(ops, ops->owner))) {
>>>> +            ret = -EACCES;
>>>> +        } else {
>>>> +            WRITE_ONCE(smc->negotiator_ops, ops);
>>>> +            /* make sure ops can be seen */
>>>> +            smp_wmb();
>>>
>>> This rcu_read_lock(), WRITE_ONCE, and smp_wmb() combo looks very 
>>> suspicious. smc->negotiator_ops is protected by rcu (+refcnt) or 
>>> lock_sock()?
>>>
>>
>> All access to ops is protected by RCU, and there are no lock_sock. 
>> WRITE_ONCE() and smp_wmb() do
>> not participate in any guarantee of the availability of ops, The 
>> purpose to using them is just wish the latest values
>> can be read as soon as possible , In fact, even if old value is read, 
>> there will be no problem in logic because all updates
>> will do synchronize_rcu() and all access to ops is under in 
>> rcu_read_lock().
>
> The explanation is not encouraging. No clear benefit while having this 
> kind of complexity here. Switching tcp congestion ops also does not 
> require this. Some of the new codes is in af_smc but bpf is the 
> primary user. It is not something that I would like to maintain and 
> then need to reason about this unusual pattern a year later. Beside, 
> this negotiator_ops assignment must be done under a lock_sock(). The 
> same probably is true for calling ops->negotiate() where the bpf prog 
> may be looking at the sk and calling bpf_setsockopt.

I got you point, If you feel that those code are complexity and 
unnecessary, I can remove them of course.

Additionally, smc_sock_assign_negotiator_ops is indeed executed under 
sock lock,  __smc_setsockopt will lock sock
for it. I misunderstood your meaning before.

As for ops ->negotiate(), thanks for this point, but considering 
performance,
I might prohibit calling setsockopt in negotiate().

>>
>>> I am going to stop reviewing here.
>>>
>>
>> Hoping my explanation can answer your questions and still looking 
>> forward to
>> your more feedback 😁.
>
> Sorry, based on the review so far (there was some RFC before), it is 
> not something that I want to continue to review and maintain a bpf 
> hook for it. You have to solicit other known community members for 
> review and sponsor this set from now on.

Okay, thank you very much for pointing out some issues and suggestions.

Thanks.