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@xxxxxxxxxxxxxxxxx>
>> Reviewed-by: Tony Lu <tonylu@xxxxxxxxxxxxxxxxx>
>> ---
>> 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.
> 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
