On 2025-03-12 21:30:14, D. Wythe wrote:
>Currently, after fallback, SMC will occupy the sk_user_data of the TCP sock(clcsk) to
>forward events. As a result, we cannot use sockmap after that, since sockmap also
>requires the use of the sk_user_data. Even more, in some cases, this may result in
>abnormal panic.
Looks like this is a bugfix more then a feature.
>
>To enable sockmap after SMC fallback , we need to avoid using sk_user_data and
>instead introduce an additional smc_ctx in tcp_sock to index from TCP sock to SMC sock.
>
>Additionally, we bind the lifecycle of the SMC sock to that of the TCP sock, ensuring
>that the indexing to the SMC sock remains valid throughout the lifetime of the TCP sock.
>
>One key reason is that SMC overrides inet_connection_sock_af_ops, which introduces
>potential dependencies. We must ensure that the af_ops remain visible throughout the
>lifecycle of the TCP socket. In addition, this also resolves potential issues in some
>scenarios where the SMC sock might be invalid.
>
>Signed-off-by: D. Wythe <alibuda@xxxxxxxxxxxxxxxxx>
>---
> include/linux/tcp.h | 1 +
> net/smc/af_smc.c | 53 ++++++++++++++++++++++-----------------------
> net/smc/smc.h | 8 +++----
> net/smc/smc_close.c | 1 -
> 4 files changed, 30 insertions(+), 33 deletions(-)
>
>diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>index f88daaa76d83..f2223b1cc0d0 100644
>--- a/include/linux/tcp.h
>+++ b/include/linux/tcp.h
>@@ -478,6 +478,7 @@ struct tcp_sock {
> #if IS_ENABLED(CONFIG_SMC)
> bool syn_smc; /* SYN includes SMC */
> bool (*smc_hs_congested)(const struct sock *sk);
>+ void *smc_ctx;
> #endif
>
> #if defined(CONFIG_TCP_MD5SIG) || defined(CONFIG_TCP_AO)
>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>index bc356f77ff1d..d434105639c1 100644
>--- a/net/smc/af_smc.c
>+++ b/net/smc/af_smc.c
>@@ -127,7 +127,7 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> struct smc_sock *smc;
> struct sock *child;
>
>- smc = smc_clcsock_user_data(sk);
>+ smc = smc_sk_from_clcsk(sk);
>
> if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
> sk->sk_max_ack_backlog)
>@@ -143,8 +143,6 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> own_req);
> /* child must not inherit smc or its ops */
> if (child) {
>- rcu_assign_sk_user_data(child, NULL);
>-
> /* v4-mapped sockets don't inherit parent ops. Don't restore. */
> if (inet_csk(child)->icsk_af_ops == inet_csk(sk)->icsk_af_ops)
> inet_csk(child)->icsk_af_ops = smc->ori_af_ops;
>@@ -161,10 +159,7 @@ static bool smc_hs_congested(const struct sock *sk)
> {
> const struct smc_sock *smc;
>
>- smc = smc_clcsock_user_data(sk);
>-
>- if (!smc)
>- return true;
>+ smc = smc_sk_from_clcsk(sk);
I don't see any users of smc in this function. Seems it is redundant here.
>
> if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
> return true;
>@@ -250,7 +245,6 @@ static void smc_fback_restore_callbacks(struct smc_sock *smc)
> struct sock *clcsk = smc->clcsock->sk;
>
> write_lock_bh(&clcsk->sk_callback_lock);
>- clcsk->sk_user_data = NULL;
>
> smc_clcsock_restore_cb(&clcsk->sk_state_change, &smc->clcsk_state_change);
> smc_clcsock_restore_cb(&clcsk->sk_data_ready, &smc->clcsk_data_ready);
>@@ -832,11 +826,10 @@ static void smc_fback_forward_wakeup(struct smc_sock *smc, struct sock *clcsk,
>
> static void smc_fback_state_change(struct sock *clcsk)
> {
>- struct smc_sock *smc;
>+ struct smc_sock *smc = smc_sk_from_clcsk(clcsk);
>
> read_lock_bh(&clcsk->sk_callback_lock);
>- smc = smc_clcsock_user_data(clcsk);
>- if (smc)
>+ if (smc->clcsk_state_change)
> smc_fback_forward_wakeup(smc, clcsk,
> smc->clcsk_state_change);
Since we checked the clcsock_callback everywhere, why not put it into
smc_fback_forward_wakeup() ?
Best regards,
Dust
