On 10/11/24 11:37 PM, Alexei Starovoitov wrote:
On Thu, Oct 10, 2024 at 11:44 PM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:
On 10/11/24 12:21 AM, Alexei Starovoitov wrote:
On Wed, Oct 9, 2024 at 8:58 PM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:
+__bpf_hook_start();
+
+__weak noinline int select_syn_smc(const struct sock *sk, struct sockaddr *peer)
+{
+ return 1;
+}
+
+__bpf_hook_end();
+
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);
@@ -156,19 +165,43 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
return NULL;
}
-static bool smc_hs_congested(const struct sock *sk)
+static void smc_openreq_init(struct request_sock *req,
+ const struct tcp_options_received *rx_opt,
+ struct sk_buff *skb, const struct sock *sk)
{
+ struct inet_request_sock *ireq = inet_rsk(req);
+ struct sockaddr_storage rmt_sockaddr = {};
const struct smc_sock *smc;
smc = smc_clcsock_user_data(sk);
if (!smc)
- return true;
+ return;
- if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
- return true;
+ if (smc->limit_smc_hs && workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
+ goto out_no_smc;
- return false;
+ rmt_sockaddr.ss_family = sk->sk_family;
+
+ if (rmt_sockaddr.ss_family == AF_INET) {
+ struct sockaddr_in *rmt4_sockaddr = (struct sockaddr_in *)&rmt_sockaddr;
+
+ rmt4_sockaddr->sin_addr.s_addr = ireq->ir_rmt_addr;
+ rmt4_sockaddr->sin_port = ireq->ir_rmt_port;
+#if IS_ENABLED(CONFIG_IPV6)
+ } else {
+ struct sockaddr_in6 *rmt6_sockaddr = (struct sockaddr_in6 *)&rmt_sockaddr;
+
+ rmt6_sockaddr->sin6_addr = ireq->ir_v6_rmt_addr;
+ rmt6_sockaddr->sin6_port = ireq->ir_rmt_port;
+#endif /* CONFIG_IPV6 */
+ }
+
+ ireq->smc_ok = select_syn_smc(sk, (struct sockaddr *)&rmt_sockaddr);
+ return;
+out_no_smc:
+ ireq->smc_ok = 0;
+ return;
}
struct smc_hashinfo smc_v4_hashinfo = {
@@ -1671,7 +1704,7 @@ int smc_connect(struct socket *sock, struct sockaddr *addr,
}
smc_copy_sock_settings_to_clc(smc);
- tcp_sk(smc->clcsock->sk)->syn_smc = 1;
+ tcp_sk(smc->clcsock->sk)->syn_smc = select_syn_smc(sk, addr);
if (smc->connect_nonblock) {
rc = -EALREADY;
goto out;
@@ -2650,8 +2683,7 @@ int smc_listen(struct socket *sock, int backlog)
inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
- if (smc->limit_smc_hs)
- tcp_sk(smc->clcsock->sk)->smc_hs_congested = smc_hs_congested;
+ tcp_sk(smc->clcsock->sk)->smc_openreq_init = smc_openreq_init;
rc = kernel_listen(smc->clcsock, backlog);
if (rc) {
@@ -3475,6 +3507,24 @@ static void __net_exit smc_net_stat_exit(struct net *net)
.exit = smc_net_stat_exit,
};
+#if IS_ENABLED(CONFIG_BPF_SYSCALL)
+BTF_SET8_START(bpf_smc_fmodret_ids)
+BTF_ID_FLAGS(func, select_syn_smc)
+BTF_SET8_END(bpf_smc_fmodret_ids)
+
+static const struct btf_kfunc_id_set bpf_smc_fmodret_set = {
+ .owner = THIS_MODULE,
+ .set = &bpf_smc_fmodret_ids,
+};
+
+static int bpf_smc_kfunc_init(void)
+{
+ return register_btf_fmodret_id_set(&bpf_smc_fmodret_set);
+}
fmodret was an approach that hid-bpf took initially,
but eventually they removed it all and switched to struct-ops approach.
Please learn that lesson.
Use struct_ops from the beginning.
I did a presentation recently explaining the motivation behind
struct_ops and tips on how to extend the kernel.
TLDR: the step one is to design the extension _without_ bpf.
The interface should be usable for kernel modules.
And then when you have *_ops style api in place
the bpf progs will plug-in without extra work.
Slides:
https://github.com/4ast/docs/blob/main/BPF%20struct-ops.pdf
Hi Alexei,
Thanks very much for your suggestion.
In fact, I tried struct_ops in SMC about a year ago. Unfortunately, at that time struct_ops did not
support registration from modules, and I had to move some smc dependencies into bpf, which met with
community opposition. However, I noticed that this feature is now supported, so perhaps this is an
opportunity.
But on the other hand, given the current functionality, I wonder if struct_ops might be an overkill.
I haven't been able to come up with a suitable abstraction to define this ops, and in the future,
this ops might only contain the very one callback (select_syn_smc).
Looking forward for your advises.
I guess I wasn't clear. It's a Nack to the current fmodret approach.
Understood, we do not oppose the use of struct_ops, especially when modules registration
was already supported.
