Received-SPF: pass (google.com: domain of linux-kernel+bounces-129663-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <a9e1cdfba05279c36b7b85f327af66cf497d42b9.camel@linux.ibm.com>
Subject: Re: [RFC PATCH net-next v5 02/11] net/smc: introduce loopback-ism
 for SMC intra-OS shortcut
From: Gerd Bayer <gbayer@linux.ibm.com>
To: Wen Gu <guwen@linux.alibaba.com>, wintera@linux.ibm.com,
        twinkler@linux.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com,
        agordeev@linux.ibm.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, wenjia@linux.ibm.com,
        jaka@linux.ibm.com
Cc: borntraeger@linux.ibm.com, svens@linux.ibm.com, alibuda@linux.alibaba.com,
        tonylu@linux.alibaba.com, linux-kernel@vger.kernel.org,
        linux-s390@vger.kernel.org, netdev@vger.kernel.org
Date: Wed, 03 Apr 2024 13:27:54 +0200
In-Reply-To: <20240324135522.108564-3-guwen@linux.alibaba.com>
References: <20240324135522.108564-1-guwen@linux.alibaba.com>
	 <20240324135522.108564-3-guwen@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-2.fc39app4) 
Precedence: bulk
MIME-Version: 1.0

On Sun, 2024-03-24 at 21:55 +0800, Wen Gu wrote:
> Configuration of this feature is managed through the config SMC_LO.

Hi Wen,

you could omit building smc_loopback.o if CONFIG_SMC_LO was not set if
you included the following fixup-patch. I think it's cleaner to put a
few parts of net/smc/af_smc.c under conditional compile rather than
have most of the contents of net/smc/smc_loopback.c under #ifdef.

From 11a9cfce550f0c4df10eafdd30aa4226d4d522a8 Mon Sep 17 00:00:00 2001
From: Gerd Bayer <gbayer@linux.ibm.com>
Date: Wed, 3 Apr 2024 11:43:36 +0200
Subject: [PATCH] fixup! net/smc: introduce loopback-ism for SMC intra-
OS
 shortcut

---
 net/smc/Makefile       | 5 +++--
 net/smc/af_smc.c       | 6 ++++++
 net/smc/smc_loopback.c | 8 --------
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/net/smc/Makefile b/net/smc/Makefile
index a8c37111abe1..3b73c9d561bb 100644
--- a/net/smc/Makefile
+++ b/net/smc/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_SMC)	+=3D smc.o
 obj-$(CONFIG_SMC_DIAG)	+=3D smc_diag.o
 smc-y :=3D af_smc.o smc_pnet.o smc_ib.o smc_clc.o smc_core.o smc_wr.o
smc_llc.o
 smc-y +=3D smc_cdc.o smc_tx.o smc_rx.o smc_close.o smc_ism.o
smc_netlink.o smc_stats.o
-smc-y +=3D smc_tracepoint.o smc_loopback.o
-smc-$(CONFIG_SYSCTL) +=3D smc_sysctl.o
+smc-y +=3D smc_tracepoint.o
+smc-$(CONFIG_SMC_LO)	+=3D smc_loopback.o
+smc-$(CONFIG_SYSCTL)	+=3D smc_sysctl.o
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index fce7a5b2ce5c..bcbf600cd271 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -3574,11 +3574,13 @@ static int __init smc_init(void)
 		goto out_sock;
 	}
=20
+#if IS_ENABLED(CONFIG_SMC_LO)
 	rc =3D smc_loopback_init();
 	if (rc) {
 		pr_err("%s: smc_loopback_init fails with %d\n",
__func__, rc);
 		goto out_ib;
 	}
+#endif
=20
 	rc =3D tcp_register_ulp(&smc_ulp_ops);
 	if (rc) {
@@ -3590,8 +3592,10 @@ static int __init smc_init(void)
 	return 0;
=20
 out_lo:
+#if IS_ENABLED(CONFIG_SMC_LO)
 	smc_loopback_exit();
 out_ib:
+#endif
 	smc_ib_unregister_client();
 out_sock:
 	sock_unregister(PF_SMC);
@@ -3628,7 +3632,9 @@ static void __exit smc_exit(void)
 	tcp_unregister_ulp(&smc_ulp_ops);
 	sock_unregister(PF_SMC);
 	smc_core_exit();
+#if IS_ENABLED(CONFIG_SMC_LO)
 	smc_loopback_exit();
+#endif
 	smc_ib_unregister_client();
 	smc_ism_exit();
 	destroy_workqueue(smc_close_wq);
diff --git a/net/smc/smc_loopback.c b/net/smc/smc_loopback.c
index 994fe39930ad..8d0181635ded 100644
--- a/net/smc/smc_loopback.c
+++ b/net/smc/smc_loopback.c
@@ -19,7 +19,6 @@
 #include "smc_ism.h"
 #include "smc_loopback.h"
=20
-#if IS_ENABLED(CONFIG_SMC_LO)
 #define SMC_LO_V2_CAPABLE	0x1 /* loopback-ism acts as ISMv2 */
 #define SMC_LO_SUPPORT_NOCOPY	0x1
 #define SMC_DMA_ADDR_INVALID	(~(dma_addr_t)0)
@@ -442,20 +441,13 @@ static void smc_lo_dev_remove(void)
 	smc_lo_dev_exit(lo_dev);
 	put_device(&lo_dev->dev); /* device_initialize in
smc_lo_dev_probe */
 }
-#endif
=20
 int smc_loopback_init(void)
 {
-#if IS_ENABLED(CONFIG_SMC_LO)
 	return smc_lo_dev_probe();
-#else
-	return 0;
-#endif
 }
=20
 void smc_loopback_exit(void)
 {
-#if IS_ENABLED(CONFIG_SMC_LO)
 	smc_lo_dev_remove();
-#endif
 }
--=20
2.40.1


Even nicer would be, if smc_loopback_init() and smc_loopback_exit()
would have an NO-OP implementation { } straight in
net/smc/smc_loopback.h if CONFIG_SMC_LO was not enabled. That way, one
could leave out the #if ENABLED's in net/smc/af_smc.c entirely.

I think that's more kernel-style like...
Gerd