Re: [PATCH net-next v6 3/5] net/smc: Fallback when handshake workqueue congested

["D. Wythe" <alibuda@linux.alibaba.com>]

在 2022/2/10 上午12:11, Karsten Graul 写道:
> On 09/02/2022 15:11, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.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@linux.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.