[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <af4eaa36-75ec-10f2-3a41-81895730f435@linux.ibm.com>
Date: Thu, 17 Aug 2023 08:42:19 +0200
From: Jan Karcher <jaka@...ux.ibm.com>
To: Guangguan Wang <guangguan.wang@...ux.alibaba.com>,
wenjia@...ux.ibm.com, kgraul@...ux.ibm.com,
tonylu@...ux.alibaba.com, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com
Cc: horms@...nel.org, alibuda@...ux.alibaba.com,
guwen@...ux.alibaba.com, linux-s390@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/6] net/smc: support smc release version
negotiation in clc handshake
On 17/08/2023 05:18, Guangguan Wang wrote:
>
>
> On 2023/8/16 22:14, Jan Karcher wrote:
>>
>>
>> On 16/08/2023 10:33, Guangguan Wang wrote:
>>> Support smc release version negotiation in clc handshake based on
>>> SMC v2, where no negotiation process for different releases, but
>>> for different versions. The latest smc release version was updated
>>> to v2.1. And currently there are two release versions of SMCv2, v2.0
>>> and v2.1. In the release version negotiation, client sends the preferred
>>> release version by CLC Proposal Message, server makes decision for which
>>> release version to use based on the client preferred release version and
>>> self-supported release version (here choose the minimum release version
>>> of the client preferred and server latest supported), then the decision
>>> returns to client by CLC Accept Message. Client confirms the decision by
>>> CLC Confirm Message.
>>>
>>> Client Server
>>> Proposal(preferred release version)
>>> ------------------------------------>
>>>
>>> Accept(accpeted release version)
>>> min(client preferred, server latest supported)
>>> <------------------------------------
>>>
>>> Confirm(accpeted release version)
>>> ------------------------------------>
>>>
>>> Signed-off-by: Guangguan Wang <guangguan.wang@...ux.alibaba.com>
>>> Reviewed-by: Tony Lu <tonylu@...ux.alibaba.com>
>>> ---
>>> net/smc/af_smc.c | 18 ++++++++++++++++--
>>> net/smc/smc.h | 5 ++++-
>>> net/smc/smc_clc.c | 14 +++++++-------
>>> net/smc/smc_clc.h | 23 ++++++++++++++++++++++-
>>> net/smc/smc_core.h | 1 +
>>> 5 files changed, 50 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>>> index a7f887d91d89..97265691bc95 100644
>>> --- a/net/smc/af_smc.c
>>> +++ b/net/smc/af_smc.c
>>> @@ -1187,6 +1187,9 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
>>> return SMC_CLC_DECL_NOINDIRECT;
>>> }
>>> }
>>> +
>>> + ini->release_nr = fce->release;
>>> +
>>
>> why would we do this and vvvvv
>>> return 0;
>>> }
>>> @@ -1355,6 +1358,13 @@ static int smc_connect_ism(struct smc_sock *smc,
>>> struct smc_clc_msg_accept_confirm_v2 *aclc_v2 =
>>> (struct smc_clc_msg_accept_confirm_v2 *)aclc;
>>> + if (ini->first_contact_peer) {
>>> + struct smc_clc_first_contact_ext *fce =
>>> + smc_get_clc_first_contact_ext(aclc_v2, true);
>>> +
>>> + ini->release_nr = fce->release;
>>> + }
>>> +
>>
>> this two times?
>> Can't we put this together into __smc_connect where those functions get called (via smc_connect_rdma and smc_connect_ism)?
>>
>> Please provide reasoning, it might be that i oversaw the reasoning behind this duplication.
>>
> ini->release_nr is assigned only when doing first connect, thus this depends on the value test of
> ini->first_contact_peer. I have to follow the ini->first_contact_peer code logic, which may also
> make us wonder that why not put ini->first_contact_peer together into __smc_connect.
>
> Indeed, both of ini->first_contact_peer and ini->release_nr can put together into __smc_connect.
> But I think it is better to start a new patch series to refactor those code, not in v2.1 features.
True. It would only be clean if move both. Works for me.
>
>
>> Also note: Even if there is a reason to set this information seperate for SMC-D and SMC-R think about using your very neat helper function (smc_get_clc_first_contact_ext) in smc_connect_rdma_v2_prepare as well.
>>
>
> OK, I will replace the code to smc_get_clc_first_contact_ext.
>
> Thanks,
> Guangguan Wang
>
Powered by blists - more mailing lists