[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c2e41aa7-2843-5906-29d0-0e8deb06d1f7@linux.alibaba.com>
Date: Thu, 10 Feb 2022 11:43:23 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com>
To: Karsten Graul <kgraul@...ux.ibm.com>
Cc: kuba@...nel.org, davem@...emloft.net, netdev@...r.kernel.org,
linux-s390@...r.kernel.org, linux-rdma@...r.kernel.org
Subject: Re: [PATCH net-next v6 3/5] net/smc: Fallback when handshake
workqueue congested
在 2022/2/10 上午12:11, Karsten Graul 写道:
> On 09/02/2022 15:11, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@...ux.alibaba.com>
>>
>> This patch intends to provide a mechanism to allow automatic fallback to
>
> I would like to avoid the wording fallback all over here. The term SMC fallback
> is used for SMC connections that are in our socket list, but use TCP because
> something went wrong during handshake.
> What you changes result in are TCP-only connections which are not handled by
> the SMC module at all. So the comments should use a different naming for that.
> What the patch actually does is to disable the SMC experimental TCP header option,
> so the client receives no SMC indication and does not proceed with SMC.
> Is this correct?
>
> Please also see my comments below.
I agree with you, the wording fallback doesn't fit here. I'll try
limitation.
>> TCP according to the pressure of SMC handshake process. At present,
>> frequent visits will cause the incoming connections to be backlogged in
>> SMC handshake queue, raise the connections established time. Which is
>> quite unacceptable for those applications who base on short lived
>> connections.
>>
>> There are two ways to implement this mechanism:
>>
>> 1. Fallback when TCP established.
>> 2. Fallback before TCP established.
>>
>> In the first way, we need to wait and receive CLC messages that the
>> client will potentially send, and then actively reply with a decline
>> message, in a sense, which is also a sort of SMC handshake, affect the
>> connections established time on its way.
>>
>> In the second way, the only problem is that we need to inject SMC logic
>> into TCP when it is about to reply the incoming SYN, since we already do
>> that, it's seems not a problem anymore. And advantage is obvious, few
>> additional processes are required to complete the fallback.
>>
>> This patch use the second way.
>>
>> Link: https://lore.kernel.org/all/1641301961-59331-1-git-send-email-alibuda@linux.alibaba.com/
>> Signed-off-by: D. Wythe <alibuda@...ux.alibaba.com>
>> ---
>> include/linux/tcp.h | 1 +
>> net/ipv4/tcp_input.c | 3 ++-
>> net/smc/af_smc.c | 18 ++++++++++++++++++
>> 3 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>> index 78b91bb..1c4ae5d 100644
>> --- a/include/linux/tcp.h
>> +++ b/include/linux/tcp.h
>> @@ -394,6 +394,7 @@ struct tcp_sock {
>> bool is_mptcp;
>> #endif
>> #if IS_ENABLED(CONFIG_SMC)
>> + bool (*smc_in_limited)(const struct sock *sk);
>
> Better variable name: smc_hs_congested
>
>> bool syn_smc; /* SYN includes SMC */
>> #endif
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index af94a6d..e817ec6 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -6703,7 +6703,8 @@ static void tcp_openreq_init(struct request_sock *req,
>> ireq->ir_num = ntohs(tcp_hdr(skb)->dest);
>> ireq->ir_mark = inet_request_mark(sk, skb);
>> #if IS_ENABLED(CONFIG_SMC)
>> - ireq->smc_ok = rx_opt->smc_ok;
>> + ireq->smc_ok = rx_opt->smc_ok && !(tcp_sk(sk)->smc_in_limited &&
>> + tcp_sk(sk)->smc_in_limited(sk));
>
> Use new name here and elsewhere ...
>
>> #endif
>> }
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index ebfce3d..8175f60 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -101,6 +101,22 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, struct sk_buff
>> return NULL;
>> }
>>
>> +static bool smc_is_in_limited(const struct sock *sk)
>
> Better function name: smc_hs_congested()
>
>> +{
>> + const struct smc_sock *smc;
>> +
>> + smc = (const struct smc_sock *)
>> + ((uintptr_t)sk->sk_user_data & ~SK_USER_DATA_NOCOPY);
>> +
>> + if (!smc)
>> + return true;
>> +
>> + if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> static struct smc_hashinfo smc_v4_hashinfo = {
>> .lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
>> };
>> @@ -2309,6 +2325,8 @@ static int smc_listen(struct socket *sock, int backlog)
>>
>> inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
>>
>> + tcp_sk(smc->clcsock->sk)->smc_in_limited = smc_is_in_limited;
>
> Use new names here, too.
>
>> +
>> rc = kernel_listen(smc->clcsock, backlog);
>> if (rc) {
>> smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;
Copy that, i'll rename it all soon.
Thanks.
Powered by blists - more mailing lists