[PATCH net v2 0/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[PATCH net v2 0/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[Guangguan Wang]

The function smc_find_ism_store_rc is not only used for ism, so it is
reasonable to change the function name to smc_find_device_store_rc.

The ini->rc is used to store the last error happened when finding usable
ism or rdma device in smc_listen_find_device, and is set by calling smc_
find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
the value can not be overwritten anymore. So the ini-rc should be set to
the error reason only when an error actually occurs.

When finding ISM/RDMA devices, device not found is not a real error, as
not all machine have ISM/RDMA devices. Failures after device found, when
initializing device or when initializing connection, is real errors, and
should be store in ini->rc.

SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
not require same prefix.

Guangguan Wang (2):
  net/smc: change function name from smc_find_ism_store_rc to
    smc_find_device_store_rc
  net/smc: correct the reason code in smc_listen_find_device when
    fallback

 net/smc/af_smc.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH net v2 1/2] net/smc: change function name from smc_find_ism_store_rc to smc_find_device_store_rc

[Guangguan Wang]

The function smc_find_ism_store_rc is not only used for ism, so it is
reasonable to change the function name to smc_find_device_store_rc.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
---
 net/smc/af_smc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 35ddebae8894..b3a67a168495 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -2122,7 +2122,7 @@ static void smc_check_ism_v2_match(struct smc_init_info *ini,
 	}
 }
 
-static void smc_find_ism_store_rc(u32 rc, struct smc_init_info *ini)
+static void smc_find_device_store_rc(u32 rc, struct smc_init_info *ini)
 {
 	if (!ini->rc)
 		ini->rc = rc;
@@ -2164,7 +2164,7 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
 	mutex_unlock(&smcd_dev_list.mutex);
 
 	if (!ini->ism_dev[0]) {
-		smc_find_ism_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
+		smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
 		goto not_found;
 	}
 
@@ -2181,7 +2181,7 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
 		ini->ism_selected = i;
 		rc = smc_listen_ism_init(new_smc, ini);
 		if (rc) {
-			smc_find_ism_store_rc(rc, ini);
+			smc_find_device_store_rc(rc, ini);
 			/* try next active ISM device */
 			continue;
 		}
@@ -2218,7 +2218,7 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
 		return;		/* V1 ISM device found */
 
 not_found:
-	smc_find_ism_store_rc(rc, ini);
+	smc_find_device_store_rc(rc, ini);
 	ini->smcd_version &= ~SMC_V1;
 	ini->ism_dev[0] = NULL;
 	ini->is_smcd = false;
@@ -2268,7 +2268,7 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
 	ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
 	rc = smc_find_rdma_device(new_smc, ini);
 	if (rc) {
-		smc_find_ism_store_rc(rc, ini);
+		smc_find_device_store_rc(rc, ini);
 		goto not_found;
 	}
 	if (!ini->smcrv2.uses_gateway)
@@ -2285,7 +2285,7 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
 	if (!rc)
 		return;
 	ini->smcr_version = smcr_version;
-	smc_find_ism_store_rc(rc, ini);
+	smc_find_device_store_rc(rc, ini);
 
 not_found:
 	ini->smcr_version &= ~SMC_V2;
@@ -2332,7 +2332,7 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
 	/* check for matching IP prefix and subnet length (V1) */
 	prfx_rc = smc_listen_prfx_check(new_smc, pclc);
 	if (prfx_rc)
-		smc_find_ism_store_rc(prfx_rc, ini);
+		smc_find_device_store_rc(prfx_rc, ini);
 
 	/* get vlan id from IP device */
 	if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
@@ -2359,7 +2359,7 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
 		int rc;
 
 		rc = smc_find_rdma_v1_device_serv(new_smc, pclc, ini);
-		smc_find_ism_store_rc(rc, ini);
+		smc_find_device_store_rc(rc, ini);
 		return (!rc) ? 0 : ini->rc;
 	}
 	return prfx_rc;
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net v2 1/2] net/smc: change function name from smc_find_ism_store_rc to smc_find_device_store_rc

[Dust Li]

On Tue, Oct 17, 2023 at 08:42:33PM +0800, Guangguan Wang wrote:
>The function smc_find_ism_store_rc is not only used for ism, so it is
>reasonable to change the function name to smc_find_device_store_rc.
>
>Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>

Reviewed-by: Dust Li <dust.li@linux.alibaba.com>

>---
> net/smc/af_smc.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>index 35ddebae8894..b3a67a168495 100644
>--- a/net/smc/af_smc.c
>+++ b/net/smc/af_smc.c
>@@ -2122,7 +2122,7 @@ static void smc_check_ism_v2_match(struct smc_init_info *ini,
> 	}
> }
> 
>-static void smc_find_ism_store_rc(u32 rc, struct smc_init_info *ini)
>+static void smc_find_device_store_rc(u32 rc, struct smc_init_info *ini)
> {
> 	if (!ini->rc)
> 		ini->rc = rc;
>@@ -2164,7 +2164,7 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
> 	mutex_unlock(&smcd_dev_list.mutex);
> 
> 	if (!ini->ism_dev[0]) {
>-		smc_find_ism_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
>+		smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
> 		goto not_found;
> 	}
> 
>@@ -2181,7 +2181,7 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
> 		ini->ism_selected = i;
> 		rc = smc_listen_ism_init(new_smc, ini);
> 		if (rc) {
>-			smc_find_ism_store_rc(rc, ini);
>+			smc_find_device_store_rc(rc, ini);
> 			/* try next active ISM device */
> 			continue;
> 		}
>@@ -2218,7 +2218,7 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
> 		return;		/* V1 ISM device found */
> 
> not_found:
>-	smc_find_ism_store_rc(rc, ini);
>+	smc_find_device_store_rc(rc, ini);
> 	ini->smcd_version &= ~SMC_V1;
> 	ini->ism_dev[0] = NULL;
> 	ini->is_smcd = false;
>@@ -2268,7 +2268,7 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
> 	ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
> 	rc = smc_find_rdma_device(new_smc, ini);
> 	if (rc) {
>-		smc_find_ism_store_rc(rc, ini);
>+		smc_find_device_store_rc(rc, ini);
> 		goto not_found;
> 	}
> 	if (!ini->smcrv2.uses_gateway)
>@@ -2285,7 +2285,7 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
> 	if (!rc)
> 		return;
> 	ini->smcr_version = smcr_version;
>-	smc_find_ism_store_rc(rc, ini);
>+	smc_find_device_store_rc(rc, ini);
> 
> not_found:
> 	ini->smcr_version &= ~SMC_V2;
>@@ -2332,7 +2332,7 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
> 	/* check for matching IP prefix and subnet length (V1) */
> 	prfx_rc = smc_listen_prfx_check(new_smc, pclc);
> 	if (prfx_rc)
>-		smc_find_ism_store_rc(prfx_rc, ini);
>+		smc_find_device_store_rc(prfx_rc, ini);
> 
> 	/* get vlan id from IP device */
> 	if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
>@@ -2359,7 +2359,7 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
> 		int rc;
> 
> 		rc = smc_find_rdma_v1_device_serv(new_smc, pclc, ini);
>-		smc_find_ism_store_rc(rc, ini);
>+		smc_find_device_store_rc(rc, ini);
> 		return (!rc) ? 0 : ini->rc;
> 	}
> 	return prfx_rc;
>-- 
>2.24.3 (Apple Git-128)

Re: [PATCH net v2 1/2] net/smc: change function name from smc_find_ism_store_rc to smc_find_device_store_rc

[Wenjia Zhang]

On 17.10.23 14:42, Guangguan Wang wrote:
> The function smc_find_ism_store_rc is not only used for ism, so it is
> reasonable to change the function name to smc_find_device_store_rc.
> 
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>

Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

Re: [PATCH net v2 1/2] net/smc: change function name from smc_find_ism_store_rc to smc_find_device_store_rc

[Wenjia Zhang]

On 17.10.23 14:42, Guangguan Wang wrote:
> The function smc_find_ism_store_rc is not only used for ism, so it is
> reasonable to change the function name to smc_find_device_store_rc.
> 
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> ---

same as the patch [2/2], please see the comment from Jakub

>   net/smc/af_smc.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 35ddebae8894..b3a67a168495 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -2122,7 +2122,7 @@ static void smc_check_ism_v2_match(struct smc_init_info *ini,
>   	}
>   }
>   
> -static void smc_find_ism_store_rc(u32 rc, struct smc_init_info *ini)
> +static void smc_find_device_store_rc(u32 rc, struct smc_init_info *ini)
>   {
>   	if (!ini->rc)
>   		ini->rc = rc;
> @@ -2164,7 +2164,7 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
>   	mutex_unlock(&smcd_dev_list.mutex);
>   
>   	if (!ini->ism_dev[0]) {
> -		smc_find_ism_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
> +		smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
>   		goto not_found;
>   	}
>   
> @@ -2181,7 +2181,7 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
>   		ini->ism_selected = i;
>   		rc = smc_listen_ism_init(new_smc, ini);
>   		if (rc) {
> -			smc_find_ism_store_rc(rc, ini);
> +			smc_find_device_store_rc(rc, ini);
>   			/* try next active ISM device */
>   			continue;
>   		}
> @@ -2218,7 +2218,7 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
>   		return;		/* V1 ISM device found */
>   
>   not_found:
> -	smc_find_ism_store_rc(rc, ini);
> +	smc_find_device_store_rc(rc, ini);
>   	ini->smcd_version &= ~SMC_V1;
>   	ini->ism_dev[0] = NULL;
>   	ini->is_smcd = false;
> @@ -2268,7 +2268,7 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
>   	ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
>   	rc = smc_find_rdma_device(new_smc, ini);
>   	if (rc) {
> -		smc_find_ism_store_rc(rc, ini);
> +		smc_find_device_store_rc(rc, ini);
>   		goto not_found;
>   	}
>   	if (!ini->smcrv2.uses_gateway)
> @@ -2285,7 +2285,7 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
>   	if (!rc)
>   		return;
>   	ini->smcr_version = smcr_version;
> -	smc_find_ism_store_rc(rc, ini);
> +	smc_find_device_store_rc(rc, ini);
>   
>   not_found:
>   	ini->smcr_version &= ~SMC_V2;
> @@ -2332,7 +2332,7 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
>   	/* check for matching IP prefix and subnet length (V1) */
>   	prfx_rc = smc_listen_prfx_check(new_smc, pclc);
>   	if (prfx_rc)
> -		smc_find_ism_store_rc(prfx_rc, ini);
> +		smc_find_device_store_rc(prfx_rc, ini);
>   
>   	/* get vlan id from IP device */
>   	if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
> @@ -2359,7 +2359,7 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
>   		int rc;
>   
>   		rc = smc_find_rdma_v1_device_serv(new_smc, pclc, ini);
> -		smc_find_ism_store_rc(rc, ini);
> +		smc_find_device_store_rc(rc, ini);
>   		return (!rc) ? 0 : ini->rc;
>   	}
>   	return prfx_rc;

[PATCH net v2 2/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[Guangguan Wang]

The ini->rc is used to store the last error happened when finding usable
ism or rdma device in smc_listen_find_device, and is set by calling smc_
find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
the value can not be overwritten anymore. So the ini-rc should be set to
the error reason only when an error actually occurs.

When finding ISM/RDMA devices, device not found is not a real error, as
not all machine have ISM/RDMA devices. Failures after device found, when
initializing device or when initializing connection, is real errors, and
should be store in ini->rc.

SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
not require same prefix.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
---
 net/smc/af_smc.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index b3a67a168495..21e9c6ec4d01 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -2163,10 +2163,8 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
 	}
 	mutex_unlock(&smcd_dev_list.mutex);
 
-	if (!ini->ism_dev[0]) {
-		smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
+	if (!ini->ism_dev[0])
 		goto not_found;
-	}
 
 	smc_ism_get_system_eid(&eid);
 	if (!smc_clc_match_eid(ini->negotiated_eid, smc_v2_ext,
@@ -2216,9 +2214,9 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
 	rc = smc_listen_ism_init(new_smc, ini);
 	if (!rc)
 		return;		/* V1 ISM device found */
+	smc_find_device_store_rc(rc, ini);
 
 not_found:
-	smc_find_device_store_rc(rc, ini);
 	ini->smcd_version &= ~SMC_V1;
 	ini->ism_dev[0] = NULL;
 	ini->is_smcd = false;
@@ -2267,10 +2265,8 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
 	ini->smcrv2.saddr = new_smc->clcsock->sk->sk_rcv_saddr;
 	ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
 	rc = smc_find_rdma_device(new_smc, ini);
-	if (rc) {
-		smc_find_device_store_rc(rc, ini);
+	if (rc)
 		goto not_found;
-	}
 	if (!ini->smcrv2.uses_gateway)
 		memcpy(ini->smcrv2.nexthop_mac, pclc->lcl.mac, ETH_ALEN);
 
@@ -2331,8 +2327,6 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
 
 	/* check for matching IP prefix and subnet length (V1) */
 	prfx_rc = smc_listen_prfx_check(new_smc, pclc);
-	if (prfx_rc)
-		smc_find_device_store_rc(prfx_rc, ini);
 
 	/* get vlan id from IP device */
 	if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net v2 2/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[Dust Li]

On Tue, Oct 17, 2023 at 08:42:34PM +0800, Guangguan Wang wrote:
>The ini->rc is used to store the last error happened when finding usable
>ism or rdma device in smc_listen_find_device, and is set by calling smc_
>find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
>the value can not be overwritten anymore. So the ini-rc should be set to
>the error reason only when an error actually occurs.
>
>When finding ISM/RDMA devices, device not found is not a real error, as
>not all machine have ISM/RDMA devices. Failures after device found, when
>initializing device or when initializing connection, is real errors, and
>should be store in ini->rc.
>
>SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
>not require same prefix.
>
>Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
>---
> net/smc/af_smc.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>index b3a67a168495..21e9c6ec4d01 100644
>--- a/net/smc/af_smc.c
>+++ b/net/smc/af_smc.c
>@@ -2163,10 +2163,8 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
> 	}
> 	mutex_unlock(&smcd_dev_list.mutex);
> 
>-	if (!ini->ism_dev[0]) {
>-		smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
>+	if (!ini->ism_dev[0])

Hi Guangguan,

Generally, I think this is right. Fallback in one kind of device should
not be the final fallback reason.

But what if we have more than one device and failed more than once, for
example:
Let's say we have an ISM device, a RDMA device. First we looked the ISM device
and failed during the initialization, we got a fallback reason A. Then we
looked for the RDMA device, and failed again, with another reason B.
Finally, we fallback to TCP. What should fallback reason be ?

OTOH, SMC_CLC_DECL_NOSMCD2DEV is only used here. Removing it would mean
that we would never see SMC_CLC_DECL_NOSMCD2DEV in the fallback reason,
which makes it meaningless.

Best regards,
Dust

> 		goto not_found;
>-	}
> 
> 	smc_ism_get_system_eid(&eid);
> 	if (!smc_clc_match_eid(ini->negotiated_eid, smc_v2_ext,
>@@ -2216,9 +2214,9 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
> 	rc = smc_listen_ism_init(new_smc, ini);
> 	if (!rc)
> 		return;		/* V1 ISM device found */
>+	smc_find_device_store_rc(rc, ini);
> 
> not_found:
>-	smc_find_device_store_rc(rc, ini);
> 	ini->smcd_version &= ~SMC_V1;
> 	ini->ism_dev[0] = NULL;
> 	ini->is_smcd = false;
>@@ -2267,10 +2265,8 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
> 	ini->smcrv2.saddr = new_smc->clcsock->sk->sk_rcv_saddr;
> 	ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
> 	rc = smc_find_rdma_device(new_smc, ini);
>-	if (rc) {
>-		smc_find_device_store_rc(rc, ini);
>+	if (rc)
> 		goto not_found;
>-	}
> 	if (!ini->smcrv2.uses_gateway)
> 		memcpy(ini->smcrv2.nexthop_mac, pclc->lcl.mac, ETH_ALEN);
> 
>@@ -2331,8 +2327,6 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
> 
> 	/* check for matching IP prefix and subnet length (V1) */
> 	prfx_rc = smc_listen_prfx_check(new_smc, pclc);
>-	if (prfx_rc)
>-		smc_find_device_store_rc(prfx_rc, ini);
> 
> 	/* get vlan id from IP device */
> 	if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
>-- 
>2.24.3 (Apple Git-128)

Re: [PATCH net v2 2/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[Guangguan Wang]

On 2023/10/18 15:01, Dust Li wrote:
> On Tue, Oct 17, 2023 at 08:42:34PM +0800, Guangguan Wang wrote:
>> The ini->rc is used to store the last error happened when finding usable
>> ism or rdma device in smc_listen_find_device, and is set by calling smc_
>> find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
>> the value can not be overwritten anymore. So the ini-rc should be set to
>> the error reason only when an error actually occurs.
>>
>> When finding ISM/RDMA devices, device not found is not a real error, as
>> not all machine have ISM/RDMA devices. Failures after device found, when
>> initializing device or when initializing connection, is real errors, and
>> should be store in ini->rc.
>>
>> SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
>> not require same prefix.
>>
>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
>> ---
>> net/smc/af_smc.c | 12 +++---------
>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index b3a67a168495..21e9c6ec4d01 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -2163,10 +2163,8 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
>> 	}
>> 	mutex_unlock(&smcd_dev_list.mutex);
>>
>> -	if (!ini->ism_dev[0]) {
>> -		smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
>> +	if (!ini->ism_dev[0])
> 
> Hi Guangguan,
> 
> Generally, I think this is right. Fallback in one kind of device should
> not be the final fallback reason.
> 
> But what if we have more than one device and failed more than once, for
> example:
> Let's say we have an ISM device, a RDMA device. First we looked the ISM device
> and failed during the initialization, we got a fallback reason A. Then we
> looked for the RDMA device, and failed again, with another reason B.
> Finally, we fallback to TCP. What should fallback reason be ?

IMO, the order of finding devices is defined by preference. ISM v2, ISM v1, RDMA v2, RDMA v1, the former the prefer.
I think it should return the most preferred device's failure reason if found and failed during the initialization.
So, here should return the first reason(reason A). 

> 
> OTOH, SMC_CLC_DECL_NOSMCD2DEV is only used here. Removing it would mean
> that we would never see SMC_CLC_DECL_NOSMCD2DEV in the fallback reason,
> which makes it meaningless.
>
 
Is SMC_CLC_DECL_NOSMCD2DEV really necessary? There is no reason names SMC_CLC_DECL_NOSMCR2DEV.

Thanks,
Guangguan Wang

> Best regards,
> Dust
> 
>> 		goto not_found;
>> -	}
>>
>> 	smc_ism_get_system_eid(&eid);
>> 	if (!smc_clc_match_eid(ini->negotiated_eid, smc_v2_ext,
>> @@ -2216,9 +2214,9 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
>> 	rc = smc_listen_ism_init(new_smc, ini);
>> 	if (!rc)
>> 		return;		/* V1 ISM device found */
>> +	smc_find_device_store_rc(rc, ini);
>>
>> not_found:
>> -	smc_find_device_store_rc(rc, ini);
>> 	ini->smcd_version &= ~SMC_V1;
>> 	ini->ism_dev[0] = NULL;
>> 	ini->is_smcd = false;
>> @@ -2267,10 +2265,8 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
>> 	ini->smcrv2.saddr = new_smc->clcsock->sk->sk_rcv_saddr;
>> 	ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
>> 	rc = smc_find_rdma_device(new_smc, ini);
>> -	if (rc) {
>> -		smc_find_device_store_rc(rc, ini);
>> +	if (rc)
>> 		goto not_found;
>> -	}
>> 	if (!ini->smcrv2.uses_gateway)
>> 		memcpy(ini->smcrv2.nexthop_mac, pclc->lcl.mac, ETH_ALEN);
>>
>> @@ -2331,8 +2327,6 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
>>
>> 	/* check for matching IP prefix and subnet length (V1) */
>> 	prfx_rc = smc_listen_prfx_check(new_smc, pclc);
>> -	if (prfx_rc)
>> -		smc_find_device_store_rc(prfx_rc, ini);
>>
>> 	/* get vlan id from IP device */
>> 	if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
>> -- 
>> 2.24.3 (Apple Git-128)

Re: [PATCH net v2 2/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[Wenjia Zhang]

On 18.10.23 10:35, Guangguan Wang wrote:
> 
> 
> On 2023/10/18 15:01, Dust Li wrote:
>> On Tue, Oct 17, 2023 at 08:42:34PM +0800, Guangguan Wang wrote:
>>> The ini->rc is used to store the last error happened when finding usable
>>> ism or rdma device in smc_listen_find_device, and is set by calling smc_
>>> find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
>>> the value can not be overwritten anymore. So the ini-rc should be set to
>>> the error reason only when an error actually occurs.
>>>
>>> When finding ISM/RDMA devices, device not found is not a real error, as
>>> not all machine have ISM/RDMA devices. Failures after device found, when
What do you mean about this sentence? Do you mean that no any (ISM/RDMA) 
device  found is not real error? If not, what is the real reason?

>>> initializing device or when initializing connection, is real errors, and
>>> should be store in ini->rc.
>>>
>>> SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
>>> not require same prefix.
>>>
>>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
>>> ---
>>> net/smc/af_smc.c | 12 +++---------
>>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>>> index b3a67a168495..21e9c6ec4d01 100644
>>> --- a/net/smc/af_smc.c
>>> +++ b/net/smc/af_smc.c
>>> @@ -2163,10 +2163,8 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
>>> 	}
>>> 	mutex_unlock(&smcd_dev_list.mutex);
>>>
>>> -	if (!ini->ism_dev[0]) {
>>> -		smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
>>> +	if (!ini->ism_dev[0])
>>
>> Hi Guangguan,
>>
>> Generally, I think this is right. Fallback in one kind of device should
>> not be the final fallback reason.
>>
>> But what if we have more than one device and failed more than once, for
>> example:
>> Let's say we have an ISM device, a RDMA device. First we looked the ISM device
>> and failed during the initialization, we got a fallback reason A. Then we
>> looked for the RDMA device, and failed again, with another reason B.
>> Finally, we fallback to TCP. What should fallback reason be ?
> 
> IMO, the order of finding devices is defined by preference. ISM v2, ISM v1, RDMA v2, RDMA v1, the former the prefer.
> I think it should return the most preferred device's failure reason if found and failed during the initialization.
> So, here should return the first reason(reason A).
> 
In this case Dust mentioned, I'd prefer a reaon including A and B, like 
current reason SMC_CLC_DECL_NOSMCDEV.

>>
>> OTOH, SMC_CLC_DECL_NOSMCD2DEV is only used here. Removing it would mean
>> that we would never see SMC_CLC_DECL_NOSMCD2DEV in the fallback reason,
>> which makes it meaningless.
>>
>   
> Is SMC_CLC_DECL_NOSMCD2DEV really necessary? There is no reason names SMC_CLC_DECL_NOSMCR2DEV.
> 
I do see the necessity for the debugging.
> Thanks,
> Guangguan Wang
> 
>> Best regards,
>> Dust
>>
>>> 		goto not_found;
>>> -	}
>>>
>>> 	smc_ism_get_system_eid(&eid);
>>> 	if (!smc_clc_match_eid(ini->negotiated_eid, smc_v2_ext,
>>> @@ -2216,9 +2214,9 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
>>> 	rc = smc_listen_ism_init(new_smc, ini);
>>> 	if (!rc)
>>> 		return;		/* V1 ISM device found */
>>> +	smc_find_device_store_rc(rc, ini);
>>>
>>> not_found:
>>> -	smc_find_device_store_rc(rc, ini);
>>> 	ini->smcd_version &= ~SMC_V1;
>>> 	ini->ism_dev[0] = NULL;
>>> 	ini->is_smcd = false;
>>> @@ -2267,10 +2265,8 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
>>> 	ini->smcrv2.saddr = new_smc->clcsock->sk->sk_rcv_saddr;
>>> 	ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
>>> 	rc = smc_find_rdma_device(new_smc, ini);
>>> -	if (rc) {
>>> -		smc_find_device_store_rc(rc, ini);
>>> +	if (rc)
>>> 		goto not_found;
>>> -	}
>>> 	if (!ini->smcrv2.uses_gateway)
>>> 		memcpy(ini->smcrv2.nexthop_mac, pclc->lcl.mac, ETH_ALEN);
>>>
>>> @@ -2331,8 +2327,6 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
>>>
>>> 	/* check for matching IP prefix and subnet length (V1) */
>>> 	prfx_rc = smc_listen_prfx_check(new_smc, pclc);
>>> -	if (prfx_rc)
>>> -		smc_find_device_store_rc(prfx_rc, ini);
>>>
>>> 	/* get vlan id from IP device */
>>> 	if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
>>> -- 
>>> 2.24.3 (Apple Git-128)

Re: [PATCH net v2 2/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[Guangguan Wang]

On 2023/10/19 03:43, Wenjia Zhang wrote:
> 
> 
> On 18.10.23 10:35, Guangguan Wang wrote:
>>
>>
>> On 2023/10/18 15:01, Dust Li wrote:
>>> On Tue, Oct 17, 2023 at 08:42:34PM +0800, Guangguan Wang wrote:
>>>> The ini->rc is used to store the last error happened when finding usable
>>>> ism or rdma device in smc_listen_find_device, and is set by calling smc_
>>>> find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
>>>> the value can not be overwritten anymore. So the ini-rc should be set to
>>>> the error reason only when an error actually occurs.
>>>>
>>>> When finding ISM/RDMA devices, device not found is not a real error, as
>>>> not all machine have ISM/RDMA devices. Failures after device found, when
> What do you mean about this sentence? Do you mean that no any (ISM/RDMA) device  found is not real error? If not, what is the real reason?
> 

Sorry for not describing it clearly.

I mean either ISM or RDMA devices not found is not real error, as not all machine have ISM and RDMA devices.

Both of ISM and RDMA devices not found is real error, and in this patch it will return SMC_CLC_DECL_NOSMCDEV.

>>>> initializing device or when initializing connection, is real errors, and
>>>> should be store in ini->rc.
>>>>
>>>> SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
>>>> not require same prefix.
>>>>
>>>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
>>>> ---
>>>> net/smc/af_smc.c | 12 +++---------
>>>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>>>> index b3a67a168495..21e9c6ec4d01 100644
>>>> --- a/net/smc/af_smc.c
>>>> +++ b/net/smc/af_smc.c
>>>> @@ -2163,10 +2163,8 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
>>>>     }
>>>>     mutex_unlock(&smcd_dev_list.mutex);
>>>>
>>>> -    if (!ini->ism_dev[0]) {
>>>> -        smc_find_device_store_rc(SMC_CLC_DECL_NOSMCD2DEV, ini);
>>>> +    if (!ini->ism_dev[0])
>>>
>>> Hi Guangguan,
>>>
>>> Generally, I think this is right. Fallback in one kind of device should
>>> not be the final fallback reason.
>>>
>>> But what if we have more than one device and failed more than once, for
>>> example:
>>> Let's say we have an ISM device, a RDMA device. First we looked the ISM device
>>> and failed during the initialization, we got a fallback reason A. Then we
>>> looked for the RDMA device, and failed again, with another reason B.
>>> Finally, we fallback to TCP. What should fallback reason be ?
>>
>> IMO, the order of finding devices is defined by preference. ISM v2, ISM v1, RDMA v2, RDMA v1, the former the prefer.
>> I think it should return the most preferred device's failure reason if found and failed during the initialization.
>> So, here should return the first reason(reason A).
>>
> In this case Dust mentioned, I'd prefer a reaon including A and B, like current reason SMC_CLC_DECL_NOSMCDEV.
> 

For debugging purpose, I'd rather give a reason that actually occurred during the initialization, 
since SMC_CLC_DECL_NOSMCDEV doesn't give any detailed information about the failure.


Thanks,
Guangguan Wang

>>>
>>> OTOH, SMC_CLC_DECL_NOSMCD2DEV is only used here. Removing it would mean
>>> that we would never see SMC_CLC_DECL_NOSMCD2DEV in the fallback reason,
>>> which makes it meaningless.
>>>
>>   Is SMC_CLC_DECL_NOSMCD2DEV really necessary? There is no reason names SMC_CLC_DECL_NOSMCR2DEV.
>>
> I do see the necessity for the debugging.
>> Thanks,
>> Guangguan Wang
>>
>>> Best regards,
>>> Dust
>>>
>>>>         goto not_found;
>>>> -    }
>>>>
>>>>     smc_ism_get_system_eid(&eid);
>>>>     if (!smc_clc_match_eid(ini->negotiated_eid, smc_v2_ext,
>>>> @@ -2216,9 +2214,9 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
>>>>     rc = smc_listen_ism_init(new_smc, ini);
>>>>     if (!rc)
>>>>         return;        /* V1 ISM device found */
>>>> +    smc_find_device_store_rc(rc, ini);
>>>>
>>>> not_found:
>>>> -    smc_find_device_store_rc(rc, ini);
>>>>     ini->smcd_version &= ~SMC_V1;
>>>>     ini->ism_dev[0] = NULL;
>>>>     ini->is_smcd = false;
>>>> @@ -2267,10 +2265,8 @@ static void smc_find_rdma_v2_device_serv(struct smc_sock *new_smc,
>>>>     ini->smcrv2.saddr = new_smc->clcsock->sk->sk_rcv_saddr;
>>>>     ini->smcrv2.daddr = smc_ib_gid_to_ipv4(smc_v2_ext->roce);
>>>>     rc = smc_find_rdma_device(new_smc, ini);
>>>> -    if (rc) {
>>>> -        smc_find_device_store_rc(rc, ini);
>>>> +    if (rc)
>>>>         goto not_found;
>>>> -    }
>>>>     if (!ini->smcrv2.uses_gateway)
>>>>         memcpy(ini->smcrv2.nexthop_mac, pclc->lcl.mac, ETH_ALEN);
>>>>
>>>> @@ -2331,8 +2327,6 @@ static int smc_listen_find_device(struct smc_sock *new_smc,
>>>>
>>>>     /* check for matching IP prefix and subnet length (V1) */
>>>>     prfx_rc = smc_listen_prfx_check(new_smc, pclc);
>>>> -    if (prfx_rc)
>>>> -        smc_find_device_store_rc(prfx_rc, ini);
>>>>
>>>>     /* get vlan id from IP device */
>>>>     if (smc_vlan_by_tcpsk(new_smc->clcsock, ini))
>>>> -- 
>>>> 2.24.3 (Apple Git-128)

Re: [PATCH net v2 2/2] net/smc: correct the reason code in smc_listen_find_device when fallback

[Jakub Kicinski]

On Tue, 17 Oct 2023 20:42:34 +0800 Guangguan Wang wrote:
> The ini->rc is used to store the last error happened when finding usable
> ism or rdma device in smc_listen_find_device, and is set by calling smc_
> find_device_store_rc. Once the ini->rc is assigned to an none-zero value,
> the value can not be overwritten anymore. So the ini-rc should be set to
> the error reason only when an error actually occurs.
> 
> When finding ISM/RDMA devices, device not found is not a real error, as
> not all machine have ISM/RDMA devices. Failures after device found, when
> initializing device or when initializing connection, is real errors, and
> should be store in ini->rc.
> 
> SMC_CLC_DECL_DIFFPREFIX also is not a real error, as for SMC-RV2, it is
> not require same prefix.

If it's important enough to be a fix this late in the release cycle,
it better come with a Fixes tag...
-- 
pw-bot: cr