| Message ID | 20230807062720.20555-5-guangguan.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/smc: several features's implementation for smc v2.1 | expand |
On 07.08.23 08:27, Guangguan Wang wrote: > Support max connections per lgr negotiation for SMCR v2.1, > which is one of smc v2.1 features. > > Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com> > Reviewed-by: Tony Lu <tonylu@linux.alibaba.com> > --- > net/smc/af_smc.c | 1 + > net/smc/smc_clc.c | 41 +++++++++++++++++++++++++++++++++++++++-- > net/smc/smc_clc.h | 7 +++++-- > net/smc/smc_core.c | 4 +++- > net/smc/smc_core.h | 5 +++++ > net/smc/smc_llc.c | 6 +++++- > 6 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > index fd58e25beddf..b95d3fd48c28 100644 > --- a/net/smc/af_smc.c > +++ b/net/smc/af_smc.c > @@ -1214,6 +1214,7 @@ static int smc_connect_rdma(struct smc_sock *smc, > memcpy(ini->peer_systemid, aclc->r0.lcl.id_for_peer, SMC_SYSTEMID_LEN); > memcpy(ini->peer_gid, aclc->r0.lcl.gid, SMC_GID_SIZE); > memcpy(ini->peer_mac, aclc->r0.lcl.mac, ETH_ALEN); > + ini->max_conns = SMC_RMBS_PER_LGR_MAX; > > reason_code = smc_connect_rdma_v2_prepare(smc, aclc, ini); > if (reason_code) > diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c > index 4f6b69af2b80..e2b224063dcc 100644 > --- a/net/smc/smc_clc.c > +++ b/net/smc/smc_clc.c > @@ -427,9 +427,17 @@ static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce, > fce->fce_v20.os_type = SMC_CLC_OS_LINUX; > fce->fce_v20.release = ini->release_ver; > memcpy(fce->fce_v20.hostname, smc_hostname, sizeof(smc_hostname)); > - if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1) > + if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1) { > ret = sizeof(struct smc_clc_first_contact_ext); > + goto out; > + } > + > + if (ini->release_ver >= SMC_RELEASE_1) { > + if (!ini->is_smcd) > + fce->max_conns = ini->max_conns; > + } > > +out: > return ret; > } > > @@ -931,8 +939,10 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) > sizeof(struct smc_clc_smcd_gid_chid); > } > } > - if (smcr_indicated(ini->smc_type_v2)) > + if (smcr_indicated(ini->smc_type_v2)) { > memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE); > + v2_ext->max_conns = SMC_CONN_PER_LGR_MAX; > + } > > pclc_base->hdr.length = htons(plen); > memcpy(trl->eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER)); > @@ -1163,6 +1173,11 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc, > { > struct smc_clc_v2_extension *pclc_v2_ext; > > + /* default max conn is SMC_RMBS_PER_LGR_MAX(255), > + * which is the default value in smc v1 and v2.0. > + */ > + ini->max_conns = SMC_RMBS_PER_LGR_MAX; > + > if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) || > ini->release_ver < SMC_RELEASE_1) > return 0; > @@ -1171,15 +1186,30 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc, > if (!pclc_v2_ext) > return SMC_CLC_DECL_NOV2EXT; > > + if (ini->smcr_version & SMC_V2) { > + ini->max_conns = min_t(u8, pclc_v2_ext->max_conns, SMC_CONN_PER_LGR_MAX); > + if (ini->max_conns < SMC_CONN_PER_LGR_MIN) > + return SMC_CLC_DECL_MAXCONNERR; > + } > + > return 0; > } > > int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce, > struct smc_init_info *ini) > { > + struct smc_clc_first_contact_ext_v2x *fce_v2x = > + (struct smc_clc_first_contact_ext_v2x *)fce; > + > if (ini->release_ver < SMC_RELEASE_1) > return 0; > > + if (!ini->is_smcd) { > + if (fce_v2x->max_conns < SMC_CONN_PER_LGR_MIN) > + return SMC_CLC_DECL_MAXCONNERR; > + ini->max_conns = fce_v2x->max_conns; > + } > + > return 0; > } > > @@ -1190,6 +1220,8 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc, > (struct smc_clc_msg_accept_confirm_v2 *)cclc; > struct smc_clc_first_contact_ext *fce = > smc_get_clc_first_contact_ext(clc_v2, ini->is_smcd); > + struct smc_clc_first_contact_ext_v2x *fce_v2x = > + (struct smc_clc_first_contact_ext_v2x *)fce; > > if (cclc->hdr.version == SMC_V1 || > !(cclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK)) > @@ -1201,6 +1233,11 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc, > if (fce->release < SMC_RELEASE_1) > return 0; > > + if (!ini->is_smcd) { > + if (fce_v2x->max_conns != ini->max_conns) > + return SMC_CLC_DECL_MAXCONNERR; > + } > + > return 0; > } > ok, now I have the answer from the last patch. > diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h > index 66932bfdc6d0..54077e50c368 100644 > --- a/net/smc/smc_clc.h > +++ b/net/smc/smc_clc.h > @@ -46,6 +46,7 @@ > #define SMC_CLC_DECL_NOSMCD2DEV 0x03030007 /* no SMC-Dv2 device found */ > #define SMC_CLC_DECL_NOUEID 0x03030008 /* peer sent no UEID */ > #define SMC_CLC_DECL_RELEASEERR 0x03030009 /* release version negotiate failed */ > +#define SMC_CLC_DECL_MAXCONNERR 0x0303000a /* max connections negotiate failed */ > #define SMC_CLC_DECL_MODEUNSUPP 0x03040000 /* smc modes do not match (R or D)*/ > #define SMC_CLC_DECL_RMBE_EC 0x03050000 /* peer has eyecatcher in RMBE */ > #define SMC_CLC_DECL_OPTUNSUPP 0x03060000 /* fastopen sockopt not supported */ > @@ -134,7 +135,8 @@ struct smc_clc_smcd_gid_chid { > struct smc_clc_v2_extension { > struct smc_clnt_opts_area_hdr hdr; > u8 roce[16]; /* RoCEv2 GID */ > - u8 reserved[16]; > + u8 max_conns; > + u8 reserved[15]; > u8 user_eids[][SMC_MAX_EID_LEN]; > }; > > @@ -236,7 +238,8 @@ struct smc_clc_first_contact_ext { > > struct smc_clc_first_contact_ext_v2x { > struct smc_clc_first_contact_ext fce_v20; > - u8 reserved3[4]; > + u8 max_conns; /* for SMC-R only */ > + u8 reserved3[3]; > __be32 vendor_exp_options; > u8 reserved4[8]; > } __packed; /* format defined in > diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c > index 6aa3db47a956..5de1fbaa6e28 100644 > --- a/net/smc/smc_core.c > +++ b/net/smc/smc_core.c > @@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini) > lgr->uses_gateway = ini->smcrv2.uses_gateway; > memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac, > ETH_ALEN); > + lgr->max_conns = ini->max_conns; > } else { > ibdev = ini->ib_dev; > ibport = ini->ib_port; > + lgr->max_conns = SMC_RMBS_PER_LGR_MAX; It is kind of confused sometimes SMC_RMBS_PER_LGR_MAX is used and sometimes SMC_CONN_PER_LGR_MAX. IMO, you can use SMC_CONN_PER_LGR_MAX in the patches series for the new feature, because they are the same value and the name is more suiable. > } > memcpy(lgr->pnet_id, ibdev->pnetid[ibport - 1], > SMC_MAX_PNETID_LEN); > @@ -1890,7 +1892,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini) > (ini->smcd_version == SMC_V2 || > lgr->vlan_id == ini->vlan_id) && > (role == SMC_CLNT || ini->is_smcd || > - (lgr->conns_num < SMC_RMBS_PER_LGR_MAX && > + (lgr->conns_num < lgr->max_conns && > !bitmap_full(lgr->rtokens_used_mask, SMC_RMBS_PER_LGR_MAX)))) { > /* link group found */ > ini->first_contact_local = 0; > diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h > index 1a97fef39127..f4f7299c810a 100644 > --- a/net/smc/smc_core.h > +++ b/net/smc/smc_core.h > @@ -22,6 +22,8 @@ > #include "smc_ib.h" > > #define SMC_RMBS_PER_LGR_MAX 255 /* max. # of RMBs per link group */ > +#define SMC_CONN_PER_LGR_MAX 255 /* max. # of connections per link group */ > +#define SMC_CONN_PER_LGR_MIN 16 /* min. # of connections per link group */ > > struct smc_lgr_list { /* list of link group definition */ > struct list_head list; > @@ -331,6 +333,8 @@ struct smc_link_group { > __be32 saddr; > /* net namespace */ > struct net *net; > + u8 max_conns; > + /* max conn can be assigned to lgr */ > }; > struct { /* SMC-D */ > u64 peer_gid; > @@ -375,6 +379,7 @@ struct smc_init_info { > u8 smc_type_v1; > u8 smc_type_v2; > u8 release_ver; > + u8 max_conns; > u8 first_contact_peer; > u8 first_contact_local; > unsigned short vlan_id; > diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c > index 90f0b60b196a..5347b62f1518 100644 > --- a/net/smc/smc_llc.c > +++ b/net/smc/smc_llc.c > @@ -52,7 +52,8 @@ struct smc_llc_msg_confirm_link { /* type 0x01 */ > u8 link_num; > u8 link_uid[SMC_LGR_ID_SIZE]; > u8 max_links; > - u8 reserved[9]; > + u8 max_conns; > + u8 reserved[8]; > }; > > #define SMC_LLC_FLAG_ADD_LNK_REJ 0x40 > @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, > confllc->link_num = link->link_id; > memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); > confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; > + if (link->lgr->smc_version == SMC_V2 && > + link->lgr->peer_smc_release >= SMC_RELEASE_1) > + confllc->max_conns = link->lgr->max_conns; > /* send llc message */ > rc = smc_wr_tx_send(link, pend); > put_out: Did I miss the negotiation process somewhere for the following scenario? (Example 4 in the document) Client Server Proposal(max conns(16)) -----------------------> Accept(max conns(32)) <----------------------- Confirm(max conns(32)) ----------------------->
On 2023/8/10 00:04, Wenjia Zhang wrote: > > > On 07.08.23 08:27, Guangguan Wang wrote: >> Support max connections per lgr negotiation for SMCR v2.1, >> which is one of smc v2.1 features. ... >> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c >> index 6aa3db47a956..5de1fbaa6e28 100644 >> --- a/net/smc/smc_core.c >> +++ b/net/smc/smc_core.c >> @@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini) >> lgr->uses_gateway = ini->smcrv2.uses_gateway; >> memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac, >> ETH_ALEN); >> + lgr->max_conns = ini->max_conns; >> } else { >> ibdev = ini->ib_dev; >> ibport = ini->ib_port; >> + lgr->max_conns = SMC_RMBS_PER_LGR_MAX; > > > It is kind of confused sometimes SMC_RMBS_PER_LGR_MAX is used and sometimes SMC_CONN_PER_LGR_MAX. IMO, you can use SMC_CONN_PER_LGR_MAX in the patches series for the new feature, because they are the same value and the name is more suiable. OK, I will re-define the macros like this: #define SMC_CONN_PER_LGR_MAX 255 #define SMC_CONN_PER_LGR_MIN 16 #define SMC_CONN_PER_LGR_PREFER 255 //vendors or distrubutions can modify this to a value between 16-255 as needed. ... >> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, >> confllc->link_num = link->link_id; >> memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); >> confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; >> + if (link->lgr->smc_version == SMC_V2 && >> + link->lgr->peer_smc_release >= SMC_RELEASE_1) >> + confllc->max_conns = link->lgr->max_conns; >> /* send llc message */ >> rc = smc_wr_tx_send(link, pend); >> put_out: > > Did I miss the negotiation process somewhere for the following scenario? > (Example 4 in the document) > Client Server > Proposal(max conns(16)) > -----------------------> > > Accept(max conns(32)) > <----------------------- > > Confirm(max conns(32)) > -----------------------> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16? As described in (https://www.ibm.com/support/pages/node/7009315) page 41: ... 2. Max conns and max links values sent in the CLC Proposal are the client preferred values. 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or DECLINE the connection. 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can either honor the client’s preferred values or return different (negotiated but final) values. ... If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred value and server preferred value as the final value of max conns. The max links is negotiated with the same logic. Client Server Proposal(max conns(client preferred)) -----------------------> Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred) <----------------------- Confirm(max conns(accepted value)) -----------------------> I also will add this description into commit message for better understanding. Thanks, Guangguan Wang
On 15.08.23 08:31, Guangguan Wang wrote: > > > On 2023/8/10 00:04, Wenjia Zhang wrote: >> >> >> On 07.08.23 08:27, Guangguan Wang wrote: >>> Support max connections per lgr negotiation for SMCR v2.1, >>> which is one of smc v2.1 features. > ... >>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c >>> index 6aa3db47a956..5de1fbaa6e28 100644 >>> --- a/net/smc/smc_core.c >>> +++ b/net/smc/smc_core.c >>> @@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini) >>> lgr->uses_gateway = ini->smcrv2.uses_gateway; >>> memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac, >>> ETH_ALEN); >>> + lgr->max_conns = ini->max_conns; >>> } else { >>> ibdev = ini->ib_dev; >>> ibport = ini->ib_port; >>> + lgr->max_conns = SMC_RMBS_PER_LGR_MAX; >> >> >> It is kind of confused sometimes SMC_RMBS_PER_LGR_MAX is used and sometimes SMC_CONN_PER_LGR_MAX. IMO, you can use SMC_CONN_PER_LGR_MAX in the patches series for the new feature, because they are the same value and the name is more suiable. > > OK, I will re-define the macros like this: > #define SMC_CONN_PER_LGR_MAX 255 > #define SMC_CONN_PER_LGR_MIN 16 > #define SMC_CONN_PER_LGR_PREFER 255 //vendors or distrubutions can modify this to a value between 16-255 as needed. > > ... >>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, >>> confllc->link_num = link->link_id; >>> memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); >>> confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; >>> + if (link->lgr->smc_version == SMC_V2 && >>> + link->lgr->peer_smc_release >= SMC_RELEASE_1) >>> + confllc->max_conns = link->lgr->max_conns; >>> /* send llc message */ >>> rc = smc_wr_tx_send(link, pend); >>> put_out: >> >> Did I miss the negotiation process somewhere for the following scenario? >> (Example 4 in the document) >> Client Server >> Proposal(max conns(16)) >> -----------------------> >> >> Accept(max conns(32)) >> <----------------------- >> >> Confirm(max conns(32)) >> -----------------------> > > Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16? > > As described in (https://www.ibm.com/support/pages/node/7009315) page 41: > ... > 2. Max conns and max links values sent in the CLC Proposal are the client preferred values. > 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or > DECLINE the connection. > 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can > either honor the client’s preferred values or return different (negotiated but final) values. > ... > > If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final > value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred > value and server preferred value as the final value of max conns. The max links is negotiated with the same logic. > > Client Server > Proposal(max conns(client preferred)) > -----------------------> > > Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred) > <----------------------- > > Confirm(max conns(accepted value)) > -----------------------> > > I also will add this description into commit message for better understanding. > > Thanks, > Guangguan Wang > > > Sorry for the late answer, I'm just back from vacation. That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that? Best, Wenjia
On 2023/8/28 20:54, Wenjia Zhang wrote: > > > On 15.08.23 08:31, Guangguan Wang wrote: >> >> >> On 2023/8/10 00:04, Wenjia Zhang wrote: >>> >>> >>> On 07.08.23 08:27, Guangguan Wang wrote: >>>> Support max connections per lgr negotiation for SMCR v2.1, >>>> which is one of smc v2.1 features. >> ... >>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, >>>> confllc->link_num = link->link_id; >>>> memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); >>>> confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; >>>> + if (link->lgr->smc_version == SMC_V2 && >>>> + link->lgr->peer_smc_release >= SMC_RELEASE_1) >>>> + confllc->max_conns = link->lgr->max_conns; >>>> /* send llc message */ >>>> rc = smc_wr_tx_send(link, pend); >>>> put_out: >>> >>> Did I miss the negotiation process somewhere for the following scenario? >>> (Example 4 in the document) >>> Client Server >>> Proposal(max conns(16)) >>> -----------------------> >>> >>> Accept(max conns(32)) >>> <----------------------- >>> >>> Confirm(max conns(32)) >>> -----------------------> >> >> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16? >> >> As described in (https://www.ibm.com/support/pages/node/7009315) page 41: >> ... >> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values. >> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or >> DECLINE the connection. >> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can >> either honor the client’s preferred values or return different (negotiated but final) values. >> ... >> >> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final >> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred >> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic. >> >> Client Server >> Proposal(max conns(client preferred)) >> -----------------------> >> Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred) >> <----------------------- >> Confirm(max conns(accepted value)) >> -----------------------> >> >> I also will add this description into commit message for better understanding. >> >> Thanks, >> Guangguan Wang >> >> >> > > Sorry for the late answer, I'm just back from vacation. > > That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that? > > Best, > Wenjia Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255. So does the max links. Thanks, Guangguan
On 29.08.23 04:31, Guangguan Wang wrote: > > > On 2023/8/28 20:54, Wenjia Zhang wrote: >> >> >> On 15.08.23 08:31, Guangguan Wang wrote: >>> >>> >>> On 2023/8/10 00:04, Wenjia Zhang wrote: >>>> >>>> >>>> On 07.08.23 08:27, Guangguan Wang wrote: >>>>> Support max connections per lgr negotiation for SMCR v2.1, >>>>> which is one of smc v2.1 features. >>> ... >>>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, >>>>> confllc->link_num = link->link_id; >>>>> memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); >>>>> confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; >>>>> + if (link->lgr->smc_version == SMC_V2 && >>>>> + link->lgr->peer_smc_release >= SMC_RELEASE_1) >>>>> + confllc->max_conns = link->lgr->max_conns; >>>>> /* send llc message */ >>>>> rc = smc_wr_tx_send(link, pend); >>>>> put_out: >>>> >>>> Did I miss the negotiation process somewhere for the following scenario? >>>> (Example 4 in the document) >>>> Client Server >>>> Proposal(max conns(16)) >>>> -----------------------> >>>> >>>> Accept(max conns(32)) >>>> <----------------------- >>>> >>>> Confirm(max conns(32)) >>>> -----------------------> >>> >>> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16? >>> >>> As described in (https://www.ibm.com/support/pages/node/7009315) page 41: >>> ... >>> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values. >>> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or >>> DECLINE the connection. >>> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can >>> either honor the client’s preferred values or return different (negotiated but final) values. >>> ... >>> >>> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final >>> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred >>> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic. >>> >>> Client Server >>> Proposal(max conns(client preferred)) >>> -----------------------> >>> Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred) >>> <----------------------- >>> Confirm(max conns(accepted value)) >>> -----------------------> >>> >>> I also will add this description into commit message for better understanding. >>> >>> Thanks, >>> Guangguan Wang >>> >>> >>> >> >> Sorry for the late answer, I'm just back from vacation. >> >> That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that? >> >> Best, >> Wenjia > > Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering > whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255. > So does the max links. > > Thanks, > Guangguan I don't think the server's default maxconns must be the maximum value, i.e 255. Since the patches series are already applied, we say the previous implementation uses maximus value because the maxconns is not tunable, so that we choose an appropriate value as the default value. Now the value is negotiable, the default value could be also the server's prefer value. But regarding maxlinks, I'm fine with the minimus, and actually it should be, because it should not be possible to try to add another link if one of the peers can and want to support only one link, i.e. down-level. Any opinion? Best, Wenjia
On 2023/8/29 21:18, Wenjia Zhang wrote: > > > On 29.08.23 04:31, Guangguan Wang wrote: >> >> >> On 2023/8/28 20:54, Wenjia Zhang wrote: >>> >>> >>> On 15.08.23 08:31, Guangguan Wang wrote: >>>> >>>> >>>> On 2023/8/10 00:04, Wenjia Zhang wrote: >>>>> >>>>> >>>>> On 07.08.23 08:27, Guangguan Wang wrote: >>>>>> Support max connections per lgr negotiation for SMCR v2.1, >>>>>> which is one of smc v2.1 features. >>>> ... >>>>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, >>>>>> confllc->link_num = link->link_id; >>>>>> memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); >>>>>> confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; >>>>>> + if (link->lgr->smc_version == SMC_V2 && >>>>>> + link->lgr->peer_smc_release >= SMC_RELEASE_1) >>>>>> + confllc->max_conns = link->lgr->max_conns; >>>>>> /* send llc message */ >>>>>> rc = smc_wr_tx_send(link, pend); >>>>>> put_out: >>>>> >>>>> Did I miss the negotiation process somewhere for the following scenario? >>>>> (Example 4 in the document) >>>>> Client Server >>>>> Proposal(max conns(16)) >>>>> -----------------------> >>>>> >>>>> Accept(max conns(32)) >>>>> <----------------------- >>>>> >>>>> Confirm(max conns(32)) >>>>> -----------------------> >>>> >>>> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16? >>>> >>>> As described in (https://www.ibm.com/support/pages/node/7009315) page 41: >>>> ... >>>> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values. >>>> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or >>>> DECLINE the connection. >>>> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can >>>> either honor the client’s preferred values or return different (negotiated but final) values. >>>> ... >>>> >>>> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final >>>> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred >>>> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic. >>>> >>>> Client Server >>>> Proposal(max conns(client preferred)) >>>> -----------------------> >>>> Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred) >>>> <----------------------- >>>> Confirm(max conns(accepted value)) >>>> -----------------------> >>>> >>>> I also will add this description into commit message for better understanding. >>>> >>>> Thanks, >>>> Guangguan Wang >>>> >>>> >>>> >>> >>> Sorry for the late answer, I'm just back from vacation. >>> >>> That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that? >>> >>> Best, >>> Wenjia >> >> Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering >> whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255. >> So does the max links. >> >> Thanks, >> Guangguan > > I don't think the server's default maxconns must be the maximum value, i.e 255. Since the patches series are already applied, we say the previous implementation uses maximus value because the maxconns is not tunable, so that we choose an appropriate value as the default value. > Now the value is negotiable, the default value could be also the server's prefer value. If the server's default maxconns could be other value rather than maximum value, it's OK to use other decision algorithm(minimum, maximum or others). But it is still a question that how to tune the default maxconns, maybe it is different from different linux distributions and different vendors of rdma nic. > But regarding maxlinks, I'm fine with the minimus, and actually it should be, because it should not be possible to try to add another link if one of the peers can and want to support only one link, i.e. down-level. Agree with you. > Any opinion? > > Best, > Wenjia Thanks, Guangguan Wang
On 30.08.23 05:17, Guangguan Wang wrote: > > > On 2023/8/29 21:18, Wenjia Zhang wrote: >> >> >> On 29.08.23 04:31, Guangguan Wang wrote: >>> >>> >>> On 2023/8/28 20:54, Wenjia Zhang wrote: >>>> >>>> >>>> On 15.08.23 08:31, Guangguan Wang wrote: >>>>> >>>>> >>>>> On 2023/8/10 00:04, Wenjia Zhang wrote: >>>>>> >>>>>> >>>>>> On 07.08.23 08:27, Guangguan Wang wrote: >>>>>>> Support max connections per lgr negotiation for SMCR v2.1, >>>>>>> which is one of smc v2.1 features. >>>>> ... >>>>>>> @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, >>>>>>> confllc->link_num = link->link_id; >>>>>>> memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); >>>>>>> confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; >>>>>>> + if (link->lgr->smc_version == SMC_V2 && >>>>>>> + link->lgr->peer_smc_release >= SMC_RELEASE_1) >>>>>>> + confllc->max_conns = link->lgr->max_conns; >>>>>>> /* send llc message */ >>>>>>> rc = smc_wr_tx_send(link, pend); >>>>>>> put_out: >>>>>> >>>>>> Did I miss the negotiation process somewhere for the following scenario? >>>>>> (Example 4 in the document) >>>>>> Client Server >>>>>> Proposal(max conns(16)) >>>>>> -----------------------> >>>>>> >>>>>> Accept(max conns(32)) >>>>>> <----------------------- >>>>>> >>>>>> Confirm(max conns(32)) >>>>>> -----------------------> >>>>> >>>>> Did you mean the accepted max conns is different(not 32) from the Example 4 when the proposal max conns is 16? >>>>> >>>>> As described in (https://www.ibm.com/support/pages/node/7009315) page 41: >>>>> ... >>>>> 2. Max conns and max links values sent in the CLC Proposal are the client preferred values. >>>>> 3. The v2.1 values sent in the Accept message are the final values. The client must accept the values or >>>>> DECLINE the connection. >>>>> 4. Max conns and links values sent in the CLC Accept are the final values (server dictates). The server can >>>>> either honor the client’s preferred values or return different (negotiated but final) values. >>>>> ... >>>>> >>>>> If I understand correctly, the server dictates the final value of max conns, but how the server dictates the final >>>>> value of max conns is not defined in SMC v2.1. In this patch, the server use the minimum value of client preferred >>>>> value and server preferred value as the final value of max conns. The max links is negotiated with the same logic. >>>>> >>>>> Client Server >>>>> Proposal(max conns(client preferred)) >>>>> -----------------------> >>>>> Accept(max conns(accepted value)) accepted value=min(client preferred, server preferred) >>>>> <----------------------- >>>>> Confirm(max conns(accepted value)) >>>>> -----------------------> >>>>> >>>>> I also will add this description into commit message for better understanding. >>>>> >>>>> Thanks, >>>>> Guangguan Wang >>>>> >>>>> >>>>> >>>> >>>> Sorry for the late answer, I'm just back from vacation. >>>> >>>> That's true that the protocol does not define how the server decides the final value(s). I'm wondering if there is some reason for you to use the minimum value instead of maximum (corresponding to the examples in the document). If the both prefered values (client's and server's) are in the range of the acceptable value, why not the maximum? Is there any consideration on that? >>>> >>>> Best, >>>> Wenjia >>> >>> Since the value of the default preferred max conns is already the maximum value of the range(16-255), I am wondering >>> whether it makes any sense to use the maximum for decision, where the negotiated result of max conns is always 255. >>> So does the max links. >>> >>> Thanks, >>> Guangguan >> >> I don't think the server's default maxconns must be the maximum value, i.e 255. Since the patches series are already applied, we say the previous implementation uses maximus value because the maxconns is not tunable, so that we choose an appropriate value as the default value. >> Now the value is negotiable, the default value could be also the server's prefer value. > If the server's default maxconns could be other value rather than maximum value, it's OK to use other decision algorithm(minimum, maximum or others). > But it is still a question that how to tune the default maxconns, maybe it is different from different linux distributions and different vendors of rdma nic. > That's true. I think more discussion is needed. Let's talk about it offline first, since these patches are already applied. BTW, thank you for the efforts! Best, Wenjia >> But regarding maxlinks, I'm fine with the minimus, and actually it should be, because it should not be possible to try to add another link if one of the peers can and want to support only one link, i.e. down-level. > Agree with you. > >> Any opinion? >> >> Best, >> Wenjia > > Thanks, > Guangguan Wang
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index fd58e25beddf..b95d3fd48c28 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -1214,6 +1214,7 @@ static int smc_connect_rdma(struct smc_sock *smc, memcpy(ini->peer_systemid, aclc->r0.lcl.id_for_peer, SMC_SYSTEMID_LEN); memcpy(ini->peer_gid, aclc->r0.lcl.gid, SMC_GID_SIZE); memcpy(ini->peer_mac, aclc->r0.lcl.mac, ETH_ALEN); + ini->max_conns = SMC_RMBS_PER_LGR_MAX; reason_code = smc_connect_rdma_v2_prepare(smc, aclc, ini); if (reason_code) diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c index 4f6b69af2b80..e2b224063dcc 100644 --- a/net/smc/smc_clc.c +++ b/net/smc/smc_clc.c @@ -427,9 +427,17 @@ static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce, fce->fce_v20.os_type = SMC_CLC_OS_LINUX; fce->fce_v20.release = ini->release_ver; memcpy(fce->fce_v20.hostname, smc_hostname, sizeof(smc_hostname)); - if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1) + if (ini->is_smcd && ini->release_ver < SMC_RELEASE_1) { ret = sizeof(struct smc_clc_first_contact_ext); + goto out; + } + + if (ini->release_ver >= SMC_RELEASE_1) { + if (!ini->is_smcd) + fce->max_conns = ini->max_conns; + } +out: return ret; } @@ -931,8 +939,10 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) sizeof(struct smc_clc_smcd_gid_chid); } } - if (smcr_indicated(ini->smc_type_v2)) + if (smcr_indicated(ini->smc_type_v2)) { memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE); + v2_ext->max_conns = SMC_CONN_PER_LGR_MAX; + } pclc_base->hdr.length = htons(plen); memcpy(trl->eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER)); @@ -1163,6 +1173,11 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc, { struct smc_clc_v2_extension *pclc_v2_ext; + /* default max conn is SMC_RMBS_PER_LGR_MAX(255), + * which is the default value in smc v1 and v2.0. + */ + ini->max_conns = SMC_RMBS_PER_LGR_MAX; + if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) || ini->release_ver < SMC_RELEASE_1) return 0; @@ -1171,15 +1186,30 @@ int smc_clc_srv_v2x_features_validate(struct smc_clc_msg_proposal *pclc, if (!pclc_v2_ext) return SMC_CLC_DECL_NOV2EXT; + if (ini->smcr_version & SMC_V2) { + ini->max_conns = min_t(u8, pclc_v2_ext->max_conns, SMC_CONN_PER_LGR_MAX); + if (ini->max_conns < SMC_CONN_PER_LGR_MIN) + return SMC_CLC_DECL_MAXCONNERR; + } + return 0; } int smc_clc_cli_v2x_features_validate(struct smc_clc_first_contact_ext *fce, struct smc_init_info *ini) { + struct smc_clc_first_contact_ext_v2x *fce_v2x = + (struct smc_clc_first_contact_ext_v2x *)fce; + if (ini->release_ver < SMC_RELEASE_1) return 0; + if (!ini->is_smcd) { + if (fce_v2x->max_conns < SMC_CONN_PER_LGR_MIN) + return SMC_CLC_DECL_MAXCONNERR; + ini->max_conns = fce_v2x->max_conns; + } + return 0; } @@ -1190,6 +1220,8 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc, (struct smc_clc_msg_accept_confirm_v2 *)cclc; struct smc_clc_first_contact_ext *fce = smc_get_clc_first_contact_ext(clc_v2, ini->is_smcd); + struct smc_clc_first_contact_ext_v2x *fce_v2x = + (struct smc_clc_first_contact_ext_v2x *)fce; if (cclc->hdr.version == SMC_V1 || !(cclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK)) @@ -1201,6 +1233,11 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc, if (fce->release < SMC_RELEASE_1) return 0; + if (!ini->is_smcd) { + if (fce_v2x->max_conns != ini->max_conns) + return SMC_CLC_DECL_MAXCONNERR; + } + return 0; } diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h index 66932bfdc6d0..54077e50c368 100644 --- a/net/smc/smc_clc.h +++ b/net/smc/smc_clc.h @@ -46,6 +46,7 @@ #define SMC_CLC_DECL_NOSMCD2DEV 0x03030007 /* no SMC-Dv2 device found */ #define SMC_CLC_DECL_NOUEID 0x03030008 /* peer sent no UEID */ #define SMC_CLC_DECL_RELEASEERR 0x03030009 /* release version negotiate failed */ +#define SMC_CLC_DECL_MAXCONNERR 0x0303000a /* max connections negotiate failed */ #define SMC_CLC_DECL_MODEUNSUPP 0x03040000 /* smc modes do not match (R or D)*/ #define SMC_CLC_DECL_RMBE_EC 0x03050000 /* peer has eyecatcher in RMBE */ #define SMC_CLC_DECL_OPTUNSUPP 0x03060000 /* fastopen sockopt not supported */ @@ -134,7 +135,8 @@ struct smc_clc_smcd_gid_chid { struct smc_clc_v2_extension { struct smc_clnt_opts_area_hdr hdr; u8 roce[16]; /* RoCEv2 GID */ - u8 reserved[16]; + u8 max_conns; + u8 reserved[15]; u8 user_eids[][SMC_MAX_EID_LEN]; }; @@ -236,7 +238,8 @@ struct smc_clc_first_contact_ext { struct smc_clc_first_contact_ext_v2x { struct smc_clc_first_contact_ext fce_v20; - u8 reserved3[4]; + u8 max_conns; /* for SMC-R only */ + u8 reserved3[3]; __be32 vendor_exp_options; u8 reserved4[8]; } __packed; /* format defined in diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c index 6aa3db47a956..5de1fbaa6e28 100644 --- a/net/smc/smc_core.c +++ b/net/smc/smc_core.c @@ -895,9 +895,11 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini) lgr->uses_gateway = ini->smcrv2.uses_gateway; memcpy(lgr->nexthop_mac, ini->smcrv2.nexthop_mac, ETH_ALEN); + lgr->max_conns = ini->max_conns; } else { ibdev = ini->ib_dev; ibport = ini->ib_port; + lgr->max_conns = SMC_RMBS_PER_LGR_MAX; } memcpy(lgr->pnet_id, ibdev->pnetid[ibport - 1], SMC_MAX_PNETID_LEN); @@ -1890,7 +1892,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini) (ini->smcd_version == SMC_V2 || lgr->vlan_id == ini->vlan_id) && (role == SMC_CLNT || ini->is_smcd || - (lgr->conns_num < SMC_RMBS_PER_LGR_MAX && + (lgr->conns_num < lgr->max_conns && !bitmap_full(lgr->rtokens_used_mask, SMC_RMBS_PER_LGR_MAX)))) { /* link group found */ ini->first_contact_local = 0; diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h index 1a97fef39127..f4f7299c810a 100644 --- a/net/smc/smc_core.h +++ b/net/smc/smc_core.h @@ -22,6 +22,8 @@ #include "smc_ib.h" #define SMC_RMBS_PER_LGR_MAX 255 /* max. # of RMBs per link group */ +#define SMC_CONN_PER_LGR_MAX 255 /* max. # of connections per link group */ +#define SMC_CONN_PER_LGR_MIN 16 /* min. # of connections per link group */ struct smc_lgr_list { /* list of link group definition */ struct list_head list; @@ -331,6 +333,8 @@ struct smc_link_group { __be32 saddr; /* net namespace */ struct net *net; + u8 max_conns; + /* max conn can be assigned to lgr */ }; struct { /* SMC-D */ u64 peer_gid; @@ -375,6 +379,7 @@ struct smc_init_info { u8 smc_type_v1; u8 smc_type_v2; u8 release_ver; + u8 max_conns; u8 first_contact_peer; u8 first_contact_local; unsigned short vlan_id; diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c index 90f0b60b196a..5347b62f1518 100644 --- a/net/smc/smc_llc.c +++ b/net/smc/smc_llc.c @@ -52,7 +52,8 @@ struct smc_llc_msg_confirm_link { /* type 0x01 */ u8 link_num; u8 link_uid[SMC_LGR_ID_SIZE]; u8 max_links; - u8 reserved[9]; + u8 max_conns; + u8 reserved[8]; }; #define SMC_LLC_FLAG_ADD_LNK_REJ 0x40 @@ -472,6 +473,9 @@ int smc_llc_send_confirm_link(struct smc_link *link, confllc->link_num = link->link_id; memcpy(confllc->link_uid, link->link_uid, SMC_LGR_ID_SIZE); confllc->max_links = SMC_LLC_ADD_LNK_MAX_LINKS; + if (link->lgr->smc_version == SMC_V2 && + link->lgr->peer_smc_release >= SMC_RELEASE_1) + confllc->max_conns = link->lgr->max_conns; /* send llc message */ rc = smc_wr_tx_send(link, pend); put_out: