[PATCH net-next v2 0/2] net/smc: Two features for smc-r

[PATCH net-next v2 0/2] net/smc: Two features for smc-r

[Guangguan Wang]

v1 -> v2:
Patch #2 fix build error reported by kernel test robot <lkp@intel.com>.
https://lore.kernel.org/oe-kbuild-all/202411282154.DjX7ilwF-lkp@intel.com/

Guangguan Wang (2):
  net/smc: support SMC-R V2 for rdma devices with max_recv_sge equals to
    1
  net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

 net/smc/af_smc.c   |  5 +++++
 net/smc/smc_core.c |  5 +++++
 net/smc/smc_core.h | 11 ++++++++++-
 net/smc/smc_ib.c   |  3 +--
 net/smc/smc_llc.c  | 21 +++++++++++++++------
 net/smc/smc_wr.c   | 42 +++++++++++++++++++++---------------------
 6 files changed, 57 insertions(+), 30 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH net-next v2 1/2] net/smc: support SMC-R V2 for rdma devices with max_recv_sge equals to 1

[Guangguan Wang]

For SMC-R V2, llc msg can be larger than SMC_WR_BUF_SIZE, thus every
recv wr has 2 sges, the first sge with length SMC_WR_BUF_SIZE is for
V1/V2 compatible llc/cdc msg, and the second sge with length
SMC_WR_BUF_V2_SIZE-SMC_WR_TX_SIZE is for V2 specific llc msg, like
SMC_LLC_DELETE_RKEY and SMC_LLC_ADD_LINK for SMC-R V2. The memory
buffer in the second sge is shared by all recv wr in one link and
all link in one lgr for saving memory usage purpose.

But not all RDMA devices with max_recv_sge greater than 1. Thus SMC-R
V2 can not support on such RDMA devices and SMC_CLC_DECL_INTERR fallback
happens because of the failure of create qp.

This patch introduce the support for SMC-R V2 on RDMA devices with
max_recv_sge equals to 1. Every recv wr has only one sge with individual
buffer whose size is SMC_WR_BUF_V2_SIZE once the RDMA device's max_recv_sge
equals to 1. It may use more memory, but it is better than
SMC_CLC_DECL_INTERR fallback.

Co-developed-by: Wen Gu <guwen@linux.alibaba.com>
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
---
 net/smc/smc_core.c |  5 +++++
 net/smc/smc_core.h | 11 ++++++++++-
 net/smc/smc_ib.c   |  3 +--
 net/smc/smc_llc.c  | 21 +++++++++++++++------
 net/smc/smc_wr.c   | 42 +++++++++++++++++++++---------------------
 5 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 500952c2e67b..ede4d5f3111b 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -795,9 +795,14 @@ int smcr_link_init(struct smc_link_group *lgr, struct smc_link *lnk,
 	if (lgr->smc_version == SMC_V2) {
 		lnk->smcibdev = ini->smcrv2.ib_dev_v2;
 		lnk->ibport = ini->smcrv2.ib_port_v2;
+		lnk->wr_rx_sge_cnt = lnk->smcibdev->ibdev->attrs.max_recv_sge < 2 ? 1 : 2;
+		lnk->wr_rx_buflen = smc_link_shared_v2_rxbuf(lnk) ?
+			SMC_WR_BUF_SIZE : SMC_WR_BUF_V2_SIZE;
 	} else {
 		lnk->smcibdev = ini->ib_dev;
 		lnk->ibport = ini->ib_port;
+		lnk->wr_rx_sge_cnt = 1;
+		lnk->wr_rx_buflen = SMC_WR_BUF_SIZE;
 	}
 	get_device(&lnk->smcibdev->ibdev->dev);
 	atomic_inc(&lnk->smcibdev->lnk_cnt);
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 69b54ecd6503..48a1b1dcb576 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -122,10 +122,14 @@ struct smc_link {
 	} ____cacheline_aligned_in_smp;
 	struct completion	tx_ref_comp;
 
-	struct smc_wr_buf	*wr_rx_bufs;	/* WR recv payload buffers */
+	u8			*wr_rx_bufs;	/* WR recv payload buffers */
 	struct ib_recv_wr	*wr_rx_ibs;	/* WR recv meta data */
 	struct ib_sge		*wr_rx_sges;	/* WR recv scatter meta data */
 	/* above three vectors have wr_rx_cnt elements and use the same index */
+	int			wr_rx_sge_cnt; /* rx sge, V1 is 1, V2 is either 2 or 1 */
+	int			wr_rx_buflen;	/* buffer len for the first sge, len for the
+						 * second sge is lgr shared if rx sge is 2.
+						 */
 	dma_addr_t		wr_rx_dma_addr;	/* DMA address of wr_rx_bufs */
 	dma_addr_t		wr_rx_v2_dma_addr; /* DMA address of v2 rx buf*/
 	u64			wr_rx_id;	/* seq # of last recv WR */
@@ -506,6 +510,11 @@ static inline bool smc_link_active(struct smc_link *lnk)
 	return lnk->state == SMC_LNK_ACTIVE;
 }
 
+static inline bool smc_link_shared_v2_rxbuf(struct smc_link *lnk)
+{
+	return lnk->wr_rx_sge_cnt > 1;
+}
+
 static inline void smc_gid_be16_convert(__u8 *buf, u8 *gid_raw)
 {
 	sprintf(buf, "%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x",
diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
index 9c563cdbea90..53828833a3f7 100644
--- a/net/smc/smc_ib.c
+++ b/net/smc/smc_ib.c
@@ -662,7 +662,6 @@ void smc_ib_destroy_queue_pair(struct smc_link *lnk)
 /* create a queue pair within the protection domain for a link */
 int smc_ib_create_queue_pair(struct smc_link *lnk)
 {
-	int sges_per_buf = (lnk->lgr->smc_version == SMC_V2) ? 2 : 1;
 	struct ib_qp_init_attr qp_attr = {
 		.event_handler = smc_ib_qp_event_handler,
 		.qp_context = lnk,
@@ -676,7 +675,7 @@ int smc_ib_create_queue_pair(struct smc_link *lnk)
 			.max_send_wr = SMC_WR_BUF_CNT * 3,
 			.max_recv_wr = SMC_WR_BUF_CNT * 3,
 			.max_send_sge = SMC_IB_MAX_SEND_SGE,
-			.max_recv_sge = sges_per_buf,
+			.max_recv_sge = lnk->wr_rx_sge_cnt,
 			.max_inline_data = 0,
 		},
 		.sq_sig_type = IB_SIGNAL_REQ_WR,
diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
index 018ce8133b02..f865c58c3aa7 100644
--- a/net/smc/smc_llc.c
+++ b/net/smc/smc_llc.c
@@ -997,13 +997,14 @@ static int smc_llc_cli_conf_link(struct smc_link *link,
 }
 
 static void smc_llc_save_add_link_rkeys(struct smc_link *link,
-					struct smc_link *link_new)
+					struct smc_link *link_new,
+					u8 *llc_msg)
 {
 	struct smc_llc_msg_add_link_v2_ext *ext;
 	struct smc_link_group *lgr = link->lgr;
 	int max, i;
 
-	ext = (struct smc_llc_msg_add_link_v2_ext *)((u8 *)lgr->wr_rx_buf_v2 +
+	ext = (struct smc_llc_msg_add_link_v2_ext *)(llc_msg +
 						     SMC_WR_TX_SIZE);
 	max = min_t(u8, ext->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
 	down_write(&lgr->rmbs_lock);
@@ -1098,7 +1099,9 @@ int smc_llc_cli_add_link(struct smc_link *link, struct smc_llc_qentry *qentry)
 	if (rc)
 		goto out_clear_lnk;
 	if (lgr->smc_version == SMC_V2) {
-		smc_llc_save_add_link_rkeys(link, lnk_new);
+		u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
+			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)llc;
+		smc_llc_save_add_link_rkeys(link, lnk_new, llc_msg);
 	} else {
 		rc = smc_llc_cli_rkey_exchange(link, lnk_new);
 		if (rc) {
@@ -1498,7 +1501,9 @@ int smc_llc_srv_add_link(struct smc_link *link,
 	if (rc)
 		goto out_err;
 	if (lgr->smc_version == SMC_V2) {
-		smc_llc_save_add_link_rkeys(link, link_new);
+		u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
+			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)add_llc;
+		smc_llc_save_add_link_rkeys(link, link_new, llc_msg);
 	} else {
 		rc = smc_llc_srv_rkey_exchange(link, link_new);
 		if (rc)
@@ -1807,8 +1812,12 @@ static void smc_llc_rmt_delete_rkey(struct smc_link_group *lgr)
 	if (lgr->smc_version == SMC_V2) {
 		struct smc_llc_msg_delete_rkey_v2 *llcv2;
 
-		memcpy(lgr->wr_rx_buf_v2, llc, sizeof(*llc));
-		llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;
+		if (smc_link_shared_v2_rxbuf(link)) {
+			memcpy(lgr->wr_rx_buf_v2, llc, sizeof(*llc));
+			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;
+		} else {
+			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)llc;
+		}
 		llcv2->num_inval_rkeys = 0;
 
 		max = min_t(u8, llcv2->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
index 994c0cd4fddb..b04a21b8c511 100644
--- a/net/smc/smc_wr.c
+++ b/net/smc/smc_wr.c
@@ -439,7 +439,7 @@ static inline void smc_wr_rx_demultiplex(struct ib_wc *wc)
 		return; /* short message */
 	temp_wr_id = wc->wr_id;
 	index = do_div(temp_wr_id, link->wr_rx_cnt);
-	wr_rx = (struct smc_wr_rx_hdr *)&link->wr_rx_bufs[index];
+	wr_rx = (struct smc_wr_rx_hdr *)(link->wr_rx_bufs + index * link->wr_rx_buflen);
 	hash_for_each_possible(smc_wr_rx_hash, handler, list, wr_rx->type) {
 		if (handler->type == wr_rx->type)
 			handler->handler(wc, wr_rx);
@@ -555,7 +555,6 @@ void smc_wr_remember_qp_attr(struct smc_link *lnk)
 
 static void smc_wr_init_sge(struct smc_link *lnk)
 {
-	int sges_per_buf = (lnk->lgr->smc_version == SMC_V2) ? 2 : 1;
 	bool send_inline = (lnk->qp_attr.cap.max_inline_data > SMC_WR_TX_SIZE);
 	u32 i;
 
@@ -608,13 +607,14 @@ static void smc_wr_init_sge(struct smc_link *lnk)
 	 * the larger spillover buffer, allowing easy data mapping.
 	 */
 	for (i = 0; i < lnk->wr_rx_cnt; i++) {
-		int x = i * sges_per_buf;
+		int x = i * lnk->wr_rx_sge_cnt;
 
 		lnk->wr_rx_sges[x].addr =
-			lnk->wr_rx_dma_addr + i * SMC_WR_BUF_SIZE;
-		lnk->wr_rx_sges[x].length = SMC_WR_TX_SIZE;
+			lnk->wr_rx_dma_addr + i * lnk->wr_rx_buflen;
+		lnk->wr_rx_sges[x].length = smc_link_shared_v2_rxbuf(lnk) ?
+			SMC_WR_TX_SIZE : lnk->wr_rx_buflen;
 		lnk->wr_rx_sges[x].lkey = lnk->roce_pd->local_dma_lkey;
-		if (lnk->lgr->smc_version == SMC_V2) {
+		if (lnk->lgr->smc_version == SMC_V2 && smc_link_shared_v2_rxbuf(lnk)) {
 			lnk->wr_rx_sges[x + 1].addr =
 					lnk->wr_rx_v2_dma_addr + SMC_WR_TX_SIZE;
 			lnk->wr_rx_sges[x + 1].length =
@@ -624,7 +624,7 @@ static void smc_wr_init_sge(struct smc_link *lnk)
 		}
 		lnk->wr_rx_ibs[i].next = NULL;
 		lnk->wr_rx_ibs[i].sg_list = &lnk->wr_rx_sges[x];
-		lnk->wr_rx_ibs[i].num_sge = sges_per_buf;
+		lnk->wr_rx_ibs[i].num_sge = lnk->wr_rx_sge_cnt;
 	}
 	lnk->wr_reg.wr.next = NULL;
 	lnk->wr_reg.wr.num_sge = 0;
@@ -655,7 +655,7 @@ void smc_wr_free_link(struct smc_link *lnk)
 
 	if (lnk->wr_rx_dma_addr) {
 		ib_dma_unmap_single(ibdev, lnk->wr_rx_dma_addr,
-				    SMC_WR_BUF_SIZE * lnk->wr_rx_cnt,
+				    lnk->wr_rx_buflen * lnk->wr_rx_cnt,
 				    DMA_FROM_DEVICE);
 		lnk->wr_rx_dma_addr = 0;
 	}
@@ -740,13 +740,11 @@ int smc_wr_alloc_lgr_mem(struct smc_link_group *lgr)
 
 int smc_wr_alloc_link_mem(struct smc_link *link)
 {
-	int sges_per_buf = link->lgr->smc_version == SMC_V2 ? 2 : 1;
-
 	/* allocate link related memory */
 	link->wr_tx_bufs = kcalloc(SMC_WR_BUF_CNT, SMC_WR_BUF_SIZE, GFP_KERNEL);
 	if (!link->wr_tx_bufs)
 		goto no_mem;
-	link->wr_rx_bufs = kcalloc(SMC_WR_BUF_CNT * 3, SMC_WR_BUF_SIZE,
+	link->wr_rx_bufs = kcalloc(SMC_WR_BUF_CNT * 3, link->wr_rx_buflen,
 				   GFP_KERNEL);
 	if (!link->wr_rx_bufs)
 		goto no_mem_wr_tx_bufs;
@@ -774,7 +772,7 @@ int smc_wr_alloc_link_mem(struct smc_link *link)
 	if (!link->wr_tx_sges)
 		goto no_mem_wr_tx_rdma_sges;
 	link->wr_rx_sges = kcalloc(SMC_WR_BUF_CNT * 3,
-				   sizeof(link->wr_rx_sges[0]) * sges_per_buf,
+				   sizeof(link->wr_rx_sges[0]) * link->wr_rx_sge_cnt,
 				   GFP_KERNEL);
 	if (!link->wr_rx_sges)
 		goto no_mem_wr_tx_sges;
@@ -872,7 +870,7 @@ int smc_wr_create_link(struct smc_link *lnk)
 	smc_wr_tx_set_wr_id(&lnk->wr_tx_id, 0);
 	lnk->wr_rx_id = 0;
 	lnk->wr_rx_dma_addr = ib_dma_map_single(
-		ibdev, lnk->wr_rx_bufs,	SMC_WR_BUF_SIZE * lnk->wr_rx_cnt,
+		ibdev, lnk->wr_rx_bufs,	lnk->wr_rx_buflen * lnk->wr_rx_cnt,
 		DMA_FROM_DEVICE);
 	if (ib_dma_mapping_error(ibdev, lnk->wr_rx_dma_addr)) {
 		lnk->wr_rx_dma_addr = 0;
@@ -880,13 +878,15 @@ int smc_wr_create_link(struct smc_link *lnk)
 		goto out;
 	}
 	if (lnk->lgr->smc_version == SMC_V2) {
-		lnk->wr_rx_v2_dma_addr = ib_dma_map_single(ibdev,
-			lnk->lgr->wr_rx_buf_v2, SMC_WR_BUF_V2_SIZE,
-			DMA_FROM_DEVICE);
-		if (ib_dma_mapping_error(ibdev, lnk->wr_rx_v2_dma_addr)) {
-			lnk->wr_rx_v2_dma_addr = 0;
-			rc = -EIO;
-			goto dma_unmap;
+		if (smc_link_shared_v2_rxbuf(lnk)) {
+			lnk->wr_rx_v2_dma_addr =
+				ib_dma_map_single(ibdev, lnk->lgr->wr_rx_buf_v2,
+						  SMC_WR_BUF_V2_SIZE, DMA_FROM_DEVICE);
+			if (ib_dma_mapping_error(ibdev, lnk->wr_rx_v2_dma_addr)) {
+				lnk->wr_rx_v2_dma_addr = 0;
+				rc = -EIO;
+				goto dma_unmap;
+			}
 		}
 		lnk->wr_tx_v2_dma_addr = ib_dma_map_single(ibdev,
 			lnk->lgr->wr_tx_buf_v2, SMC_WR_BUF_V2_SIZE,
@@ -935,7 +935,7 @@ int smc_wr_create_link(struct smc_link *lnk)
 		lnk->wr_tx_v2_dma_addr = 0;
 	}
 	ib_dma_unmap_single(ibdev, lnk->wr_rx_dma_addr,
-			    SMC_WR_BUF_SIZE * lnk->wr_rx_cnt,
+			    lnk->wr_rx_buflen * lnk->wr_rx_cnt,
 			    DMA_FROM_DEVICE);
 	lnk->wr_rx_dma_addr = 0;
 out:
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net-next v2 1/2] net/smc: support SMC-R V2 for rdma devices with max_recv_sge equals to 1

[Wenjia Zhang]

On 02.12.24 13:52, Guangguan Wang wrote:
> For SMC-R V2, llc msg can be larger than SMC_WR_BUF_SIZE, thus every
> recv wr has 2 sges, the first sge with length SMC_WR_BUF_SIZE is for
> V1/V2 compatible llc/cdc msg, and the second sge with length
> SMC_WR_BUF_V2_SIZE-SMC_WR_TX_SIZE is for V2 specific llc msg, like
> SMC_LLC_DELETE_RKEY and SMC_LLC_ADD_LINK for SMC-R V2. The memory
> buffer in the second sge is shared by all recv wr in one link and
> all link in one lgr for saving memory usage purpose.
> 
> But not all RDMA devices with max_recv_sge greater than 1. Thus SMC-R
> V2 can not support on such RDMA devices and SMC_CLC_DECL_INTERR fallback
> happens because of the failure of create qp.
> 
> This patch introduce the support for SMC-R V2 on RDMA devices with
> max_recv_sge equals to 1. Every recv wr has only one sge with individual
> buffer whose size is SMC_WR_BUF_V2_SIZE once the RDMA device's max_recv_sge
> equals to 1. It may use more memory, but it is better than
> SMC_CLC_DECL_INTERR fallback.
> 
find good!
> Co-developed-by: Wen Gu <guwen@linux.alibaba.com>
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> ---
>   net/smc/smc_core.c |  5 +++++
>   net/smc/smc_core.h | 11 ++++++++++-
>   net/smc/smc_ib.c   |  3 +--
>   net/smc/smc_llc.c  | 21 +++++++++++++++------
>   net/smc/smc_wr.c   | 42 +++++++++++++++++++++---------------------
>   5 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 500952c2e67b..ede4d5f3111b 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -795,9 +795,14 @@ int smcr_link_init(struct smc_link_group *lgr, struct smc_link *lnk,
>   	if (lgr->smc_version == SMC_V2) {
>   		lnk->smcibdev = ini->smcrv2.ib_dev_v2;
>   		lnk->ibport = ini->smcrv2.ib_port_v2;
> +		lnk->wr_rx_sge_cnt = lnk->smcibdev->ibdev->attrs.max_recv_sge < 2 ? 1 : 2;
> +		lnk->wr_rx_buflen = smc_link_shared_v2_rxbuf(lnk) ?
> +			SMC_WR_BUF_SIZE : SMC_WR_BUF_V2_SIZE;
>   	} else {
>   		lnk->smcibdev = ini->ib_dev;
>   		lnk->ibport = ini->ib_port;
> +		lnk->wr_rx_sge_cnt = 1;
> +		lnk->wr_rx_buflen = SMC_WR_BUF_SIZE;
>   	}
>   	get_device(&lnk->smcibdev->ibdev->dev);
>   	atomic_inc(&lnk->smcibdev->lnk_cnt);
> diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
> index 69b54ecd6503..48a1b1dcb576 100644
> --- a/net/smc/smc_core.h
> +++ b/net/smc/smc_core.h
> @@ -122,10 +122,14 @@ struct smc_link {
>   	} ____cacheline_aligned_in_smp;
>   	struct completion	tx_ref_comp;
>   
> -	struct smc_wr_buf	*wr_rx_bufs;	/* WR recv payload buffers */
> +	u8			*wr_rx_bufs;	/* WR recv payload buffers */
>   	struct ib_recv_wr	*wr_rx_ibs;	/* WR recv meta data */
>   	struct ib_sge		*wr_rx_sges;	/* WR recv scatter meta data */
>   	/* above three vectors have wr_rx_cnt elements and use the same index */
> +	int			wr_rx_sge_cnt; /* rx sge, V1 is 1, V2 is either 2 or 1 */
> +	int			wr_rx_buflen;	/* buffer len for the first sge, len for the
> +						 * second sge is lgr shared if rx sge is 2.
> +						 */
>   	dma_addr_t		wr_rx_dma_addr;	/* DMA address of wr_rx_bufs */
>   	dma_addr_t		wr_rx_v2_dma_addr; /* DMA address of v2 rx buf*/
>   	u64			wr_rx_id;	/* seq # of last recv WR */
> @@ -506,6 +510,11 @@ static inline bool smc_link_active(struct smc_link *lnk)
>   	return lnk->state == SMC_LNK_ACTIVE;
>   }
>   
> +static inline bool smc_link_shared_v2_rxbuf(struct smc_link *lnk)
> +{
> +	return lnk->wr_rx_sge_cnt > 1;
> +}
> +
>   static inline void smc_gid_be16_convert(__u8 *buf, u8 *gid_raw)
>   {
>   	sprintf(buf, "%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x",
> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9c563cdbea90..53828833a3f7 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
> @@ -662,7 +662,6 @@ void smc_ib_destroy_queue_pair(struct smc_link *lnk)
>   /* create a queue pair within the protection domain for a link */
>   int smc_ib_create_queue_pair(struct smc_link *lnk)
>   {
> -	int sges_per_buf = (lnk->lgr->smc_version == SMC_V2) ? 2 : 1;
>   	struct ib_qp_init_attr qp_attr = {
>   		.event_handler = smc_ib_qp_event_handler,
>   		.qp_context = lnk,
> @@ -676,7 +675,7 @@ int smc_ib_create_queue_pair(struct smc_link *lnk)
>   			.max_send_wr = SMC_WR_BUF_CNT * 3,
>   			.max_recv_wr = SMC_WR_BUF_CNT * 3,
>   			.max_send_sge = SMC_IB_MAX_SEND_SGE,
> -			.max_recv_sge = sges_per_buf,
> +			.max_recv_sge = lnk->wr_rx_sge_cnt,
>   			.max_inline_data = 0,
>   		},
>   		.sq_sig_type = IB_SIGNAL_REQ_WR,
> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 018ce8133b02..f865c58c3aa7 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -997,13 +997,14 @@ static int smc_llc_cli_conf_link(struct smc_link *link,
>   }
>   
>   static void smc_llc_save_add_link_rkeys(struct smc_link *link,
> -					struct smc_link *link_new)
> +					struct smc_link *link_new,
> +					u8 *llc_msg)
>   {
>   	struct smc_llc_msg_add_link_v2_ext *ext;
>   	struct smc_link_group *lgr = link->lgr;
>   	int max, i;
>   
> -	ext = (struct smc_llc_msg_add_link_v2_ext *)((u8 *)lgr->wr_rx_buf_v2 +
> +	ext = (struct smc_llc_msg_add_link_v2_ext *)(llc_msg +
>   						     SMC_WR_TX_SIZE);
>   	max = min_t(u8, ext->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
>   	down_write(&lgr->rmbs_lock);
> @@ -1098,7 +1099,9 @@ int smc_llc_cli_add_link(struct smc_link *link, struct smc_llc_qentry *qentry)
>   	if (rc)
>   		goto out_clear_lnk;
>   	if (lgr->smc_version == SMC_V2) {
> -		smc_llc_save_add_link_rkeys(link, lnk_new);
> +		u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
> +			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)llc;
> +		smc_llc_save_add_link_rkeys(link, lnk_new, llc_msg);
>   	} else {
>   		rc = smc_llc_cli_rkey_exchange(link, lnk_new);
>   		if (rc) {
> @@ -1498,7 +1501,9 @@ int smc_llc_srv_add_link(struct smc_link *link,
>   	if (rc)
>   		goto out_err;
>   	if (lgr->smc_version == SMC_V2) {
> -		smc_llc_save_add_link_rkeys(link, link_new);
> +		u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
> +			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)add_llc;
> +		smc_llc_save_add_link_rkeys(link, link_new, llc_msg);
>   	} else {
>   		rc = smc_llc_srv_rkey_exchange(link, link_new);
>   		if (rc)
> @@ -1807,8 +1812,12 @@ static void smc_llc_rmt_delete_rkey(struct smc_link_group *lgr)
>   	if (lgr->smc_version == SMC_V2) {
>   		struct smc_llc_msg_delete_rkey_v2 *llcv2;
>   
> -		memcpy(lgr->wr_rx_buf_v2, llc, sizeof(*llc));
> -		llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;
> +		if (smc_link_shared_v2_rxbuf(link)) {
> +			memcpy(lgr->wr_rx_buf_v2, llc, sizeof(*llc));
> +			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;
> +		} else {
> +			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)llc;
> +		}
>   		llcv2->num_inval_rkeys = 0;
>   
>   		max = min_t(u8, llcv2->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> index 994c0cd4fddb..b04a21b8c511 100644
> --- a/net/smc/smc_wr.c
> +++ b/net/smc/smc_wr.c
> @@ -439,7 +439,7 @@ static inline void smc_wr_rx_demultiplex(struct ib_wc *wc)
>   		return; /* short message */
>   	temp_wr_id = wc->wr_id;
>   	index = do_div(temp_wr_id, link->wr_rx_cnt);
> -	wr_rx = (struct smc_wr_rx_hdr *)&link->wr_rx_bufs[index];
> +	wr_rx = (struct smc_wr_rx_hdr *)(link->wr_rx_bufs + index * link->wr_rx_buflen);
>   	hash_for_each_possible(smc_wr_rx_hash, handler, list, wr_rx->type) {
>   		if (handler->type == wr_rx->type)
>   			handler->handler(wc, wr_rx);
> @@ -555,7 +555,6 @@ void smc_wr_remember_qp_attr(struct smc_link *lnk)
>   
>   static void smc_wr_init_sge(struct smc_link *lnk)
>   {
> -	int sges_per_buf = (lnk->lgr->smc_version == SMC_V2) ? 2 : 1;
>   	bool send_inline = (lnk->qp_attr.cap.max_inline_data > SMC_WR_TX_SIZE);
>   	u32 i;
>   
> @@ -608,13 +607,14 @@ static void smc_wr_init_sge(struct smc_link *lnk)
>   	 * the larger spillover buffer, allowing easy data mapping.
>   	 */
>   	for (i = 0; i < lnk->wr_rx_cnt; i++) {
> -		int x = i * sges_per_buf;
> +		int x = i * lnk->wr_rx_sge_cnt;
>   
>   		lnk->wr_rx_sges[x].addr =
> -			lnk->wr_rx_dma_addr + i * SMC_WR_BUF_SIZE;
> -		lnk->wr_rx_sges[x].length = SMC_WR_TX_SIZE;
> +			lnk->wr_rx_dma_addr + i * lnk->wr_rx_buflen;
> +		lnk->wr_rx_sges[x].length = smc_link_shared_v2_rxbuf(lnk) ?
> +			SMC_WR_TX_SIZE : lnk->wr_rx_buflen;
>   		lnk->wr_rx_sges[x].lkey = lnk->roce_pd->local_dma_lkey;
> -		if (lnk->lgr->smc_version == SMC_V2) {
> +		if (lnk->lgr->smc_version == SMC_V2 && smc_link_shared_v2_rxbuf(lnk)) {
>   			lnk->wr_rx_sges[x + 1].addr =
>   					lnk->wr_rx_v2_dma_addr + SMC_WR_TX_SIZE;
>   			lnk->wr_rx_sges[x + 1].length =
> @@ -624,7 +624,7 @@ static void smc_wr_init_sge(struct smc_link *lnk)
>   		}
>   		lnk->wr_rx_ibs[i].next = NULL;
>   		lnk->wr_rx_ibs[i].sg_list = &lnk->wr_rx_sges[x];
> -		lnk->wr_rx_ibs[i].num_sge = sges_per_buf;
> +		lnk->wr_rx_ibs[i].num_sge = lnk->wr_rx_sge_cnt;
>   	}
>   	lnk->wr_reg.wr.next = NULL;
>   	lnk->wr_reg.wr.num_sge = 0;
> @@ -655,7 +655,7 @@ void smc_wr_free_link(struct smc_link *lnk)
>   
>   	if (lnk->wr_rx_dma_addr) {
>   		ib_dma_unmap_single(ibdev, lnk->wr_rx_dma_addr,
> -				    SMC_WR_BUF_SIZE * lnk->wr_rx_cnt,
> +				    lnk->wr_rx_buflen * lnk->wr_rx_cnt,
>   				    DMA_FROM_DEVICE);
>   		lnk->wr_rx_dma_addr = 0;
>   	}
> @@ -740,13 +740,11 @@ int smc_wr_alloc_lgr_mem(struct smc_link_group *lgr)
>   
>   int smc_wr_alloc_link_mem(struct smc_link *link)
>   {
> -	int sges_per_buf = link->lgr->smc_version == SMC_V2 ? 2 : 1;
> -
>   	/* allocate link related memory */
>   	link->wr_tx_bufs = kcalloc(SMC_WR_BUF_CNT, SMC_WR_BUF_SIZE, GFP_KERNEL);
>   	if (!link->wr_tx_bufs)
>   		goto no_mem;
> -	link->wr_rx_bufs = kcalloc(SMC_WR_BUF_CNT * 3, SMC_WR_BUF_SIZE,
> +	link->wr_rx_bufs = kcalloc(SMC_WR_BUF_CNT * 3, link->wr_rx_buflen,
>   				   GFP_KERNEL);
>   	if (!link->wr_rx_bufs)
>   		goto no_mem_wr_tx_bufs;
> @@ -774,7 +772,7 @@ int smc_wr_alloc_link_mem(struct smc_link *link)
>   	if (!link->wr_tx_sges)
>   		goto no_mem_wr_tx_rdma_sges;
>   	link->wr_rx_sges = kcalloc(SMC_WR_BUF_CNT * 3,
> -				   sizeof(link->wr_rx_sges[0]) * sges_per_buf,
> +				   sizeof(link->wr_rx_sges[0]) * link->wr_rx_sge_cnt,
>   				   GFP_KERNEL);
>   	if (!link->wr_rx_sges)
>   		goto no_mem_wr_tx_sges;
> @@ -872,7 +870,7 @@ int smc_wr_create_link(struct smc_link *lnk)
>   	smc_wr_tx_set_wr_id(&lnk->wr_tx_id, 0);
>   	lnk->wr_rx_id = 0;
>   	lnk->wr_rx_dma_addr = ib_dma_map_single(
> -		ibdev, lnk->wr_rx_bufs,	SMC_WR_BUF_SIZE * lnk->wr_rx_cnt,
> +		ibdev, lnk->wr_rx_bufs,	lnk->wr_rx_buflen * lnk->wr_rx_cnt,
>   		DMA_FROM_DEVICE);
>   	if (ib_dma_mapping_error(ibdev, lnk->wr_rx_dma_addr)) {
>   		lnk->wr_rx_dma_addr = 0;
> @@ -880,13 +878,15 @@ int smc_wr_create_link(struct smc_link *lnk)
>   		goto out;
>   	}
>   	if (lnk->lgr->smc_version == SMC_V2) {
> -		lnk->wr_rx_v2_dma_addr = ib_dma_map_single(ibdev,
> -			lnk->lgr->wr_rx_buf_v2, SMC_WR_BUF_V2_SIZE,
> -			DMA_FROM_DEVICE);
> -		if (ib_dma_mapping_error(ibdev, lnk->wr_rx_v2_dma_addr)) {
> -			lnk->wr_rx_v2_dma_addr = 0;
> -			rc = -EIO;
> -			goto dma_unmap;
> +		if (smc_link_shared_v2_rxbuf(lnk)) {
> +			lnk->wr_rx_v2_dma_addr =
> +				ib_dma_map_single(ibdev, lnk->lgr->wr_rx_buf_v2,
> +						  SMC_WR_BUF_V2_SIZE, DMA_FROM_DEVICE);
> +			if (ib_dma_mapping_error(ibdev, lnk->wr_rx_v2_dma_addr)) {
> +				lnk->wr_rx_v2_dma_addr = 0;
> +				rc = -EIO;
> +				goto dma_unmap;
> +			}
>   		}
>   		lnk->wr_tx_v2_dma_addr = ib_dma_map_single(ibdev,
>   			lnk->lgr->wr_tx_buf_v2, SMC_WR_BUF_V2_SIZE,
> @@ -935,7 +935,7 @@ int smc_wr_create_link(struct smc_link *lnk)
>   		lnk->wr_tx_v2_dma_addr = 0;
>   	}
>   	ib_dma_unmap_single(ibdev, lnk->wr_rx_dma_addr,
> -			    SMC_WR_BUF_SIZE * lnk->wr_rx_cnt,
> +			    lnk->wr_rx_buflen * lnk->wr_rx_cnt,
>   			    DMA_FROM_DEVICE);
>   	lnk->wr_rx_dma_addr = 0;
>   out:
It looks good for me!

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

Thanks,
Wenjia

[PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Guangguan Wang]

AF_INET6 is not supported for smc-r v2 client before, event if the
ipv6 addr is ipv4 mapped. Thus, when using AF_INET6, smc-r connection
will fallback to tcp, especially for java applications running smc-r.
This patch support ipv4 mapped ipv6 addr client for smc-r v2. Clients
using real global ipv6 addr is still not supported yet.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/af_smc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 9d76e902fd77..5b13dd759766 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct smc_sock *smc,
 	ini->check_smcrv2 = true;
 	ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
 	if (!(ini->smcr_version & SMC_V2) ||
+#if IS_ENABLED(CONFIG_IPV6)
+	    (smc->clcsock->sk->sk_family != AF_INET &&
+	     !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
+#else
 	    smc->clcsock->sk->sk_family != AF_INET ||
+#endif
 	    !smc_clc_ueid_count() ||
 	    smc_find_rdma_device(smc, ini))
 		ini->smcr_version &= ~SMC_V2;
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Wenjia Zhang]

On 02.12.24 13:52, Guangguan Wang wrote:
> AF_INET6 is not supported for smc-r v2 client before, event if the
%s/event/even/g

> ipv6 addr is ipv4 mapped. Thus, when using AF_INET6, smc-r connection
> will fallback to tcp, especially for java applications running smc-r.
> This patch support ipv4 mapped ipv6 addr client for smc-r v2. Clients
> using real global ipv6 addr is still not supported yet.
> 
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
> Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
> Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
> Reviewed-by: D. Wythe <alibuda@linux.alibaba.com>
> ---
>   net/smc/af_smc.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 9d76e902fd77..5b13dd759766 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct smc_sock *smc,
>   	ini->check_smcrv2 = true;
>   	ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>   	if (!(ini->smcr_version & SMC_V2) ||
> +#if IS_ENABLED(CONFIG_IPV6)
> +	    (smc->clcsock->sk->sk_family != AF_INET &&
> +	     !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET && 
ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If it 
is, the negativ form of the logical operation (a&&b) is (!a)||(!b), i.e. 
here should be:
（smc->clcsock->sk->sk_family != AF_INET）|| 
（!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）

> +#else
>   	    smc->clcsock->sk->sk_family != AF_INET ||
> +#endif
>   	    !smc_clc_ueid_count() ||
>   	    smc_find_rdma_device(smc, ini))
>   		ini->smcr_version &= ~SMC_V2;

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Guangguan Wang]

On 2024/12/5 18:16, Wenjia Zhang wrote:
> 
> 
> On 02.12.24 13:52, Guangguan Wang wrote:
>> AF_INET6 is not supported for smc-r v2 client before, event if the
> %s/event/even/g
> 

I will fix it in the next version.

>> ipv6 addr is ipv4 mapped. Thus, when using AF_INET6, smc-r connection
>> will fallback to tcp, especially for java applications running smc-r.
>> This patch support ipv4 mapped ipv6 addr client for smc-r v2. Clients
>> using real global ipv6 addr is still not supported yet.
>>
>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
>> Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
>> Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
>> Reviewed-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>>   net/smc/af_smc.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 9d76e902fd77..5b13dd759766 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct smc_sock *smc,
>>       ini->check_smcrv2 = true;
>>       ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>       if (!(ini->smcr_version & SMC_V2) ||
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +        (smc->clcsock->sk->sk_family != AF_INET &&
>> +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If it is, the negativ form of the logical operation (a&&b) is (!a)||(!b), i.e. here should be:
> （smc->clcsock->sk->sk_family != AF_INET）|| （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）
> 

(smc->clcsock->sk->sk_family != AF_INET && !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) not equls to !(smc->clcsock->sk->sk_family == AF_INET && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr))

Following your logic, here also can be: 
!(smc->clcsock->sk->sk_family == AF_INET || ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr))

I think both is acceptable. But in order to keep consistent with the code when IPV6 is not enabled, I prefer (smc->clcsock->sk->sk_family != AF_INET && !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)).

>> +#else
>>           smc->clcsock->sk->sk_family != AF_INET ||
>> +#endif
>>           !smc_clc_ueid_count() ||
>>           smc_find_rdma_device(smc, ini))
>>           ini->smcr_version &= ~SMC_V2;

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Halil Pasic]

On Thu, 5 Dec 2024 11:16:27 +0100
Wenjia Zhang <wenjia@linux.ibm.com> wrote:

> > --- a/net/smc/af_smc.c
> > +++ b/net/smc/af_smc.c
> > @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct
> > smc_sock *smc, ini->check_smcrv2 = true;
> >   	ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
> >   	if (!(ini->smcr_version & SMC_V2) ||
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +	    (smc->clcsock->sk->sk_family != AF_INET &&
> > +
> > !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||  
> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET
> && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If
> it is, the negativ form of the logical operation (a&&b) is (!a)||(!b),
> i.e. here should be:
> （smc->clcsock->sk->sk_family != AF_INET）|| 
> （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）

Wenjia, I think you happen to confuse something here. The condition
of this if statement is supposed to evaluate as true iff we don't want
to propose SMCRv2 because the situation is such that SMCRv2 is not
supported.

We have a bunch of conditions we need to meet for SMCRv2 so
logically we have (A && B && C && D). Now since the if is
about when SMCRv2 is not supported we have a super structure
that looks like !A || !B || !C || !D. With this patch, if
CONFIG_IPV6 is not enabled, the sub-condition remains the same:
if smc->clcsock->sk->sk_family is something else that AF_INET
the we do not do SMCRv2!

But when we do have CONFIG_IPV6 then we want to do SMCRv2 for
AF_INET6 sockets too if the addresses used are actually
v4 mapped addresses.

Now this is where the cognitive dissonance starts on my end. I
think the author assumes sk_family == AF_INET || sk_family == AF_INET6
is a tautology in this context. That may be a reasonable thing to
assume. Under that assumption 
sk_family != AF_INET &&	!ipv6_addr_v4mapped(addr) (shortened for
convenience)
becomes equivalent to
sk_family == AF_INET6 && !ipv6_addr_v4mapped(addr)
which means in words if the socket is an IPv6 sockeet and the addr is not
a v4 mapped v6 address then we *can not* do SMCRv2. And the condition
when we can is sk_family != AF_INET6 || ipv6_addr_v4mapped(addr) which
is equivalen to sk_family == AF_INET || ipv6_addr_v4mapped(addr) under
the aforementioned assumption.

But if we assume sk_family == AF_INET || sk_family == AF_INET6 then
the #else does not make any sense, because I guess with IPv6 not
available AF_INET6 is not available ant thus the else is always
guaranteed to evaluate to false under the assumption made.

Thus I conclude, that I am certainly missing something here. Guangguan,
do you care to explain?

Regards,
Halil

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Guangguan Wang]

On 2024/12/5 20:58, Halil Pasic wrote:
> On Thu, 5 Dec 2024 11:16:27 +0100
> Wenjia Zhang <wenjia@linux.ibm.com> wrote:
> 
>>> --- a/net/smc/af_smc.c
>>> +++ b/net/smc/af_smc.c
>>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct
>>> smc_sock *smc, ini->check_smcrv2 = true;
>>>   	ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>>   	if (!(ini->smcr_version & SMC_V2) ||
>>> +#if IS_ENABLED(CONFIG_IPV6)
>>> +	    (smc->clcsock->sk->sk_family != AF_INET &&
>>> +
>>> !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||  
>> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET
>> && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If
>> it is, the negativ form of the logical operation (a&&b) is (!a)||(!b),
>> i.e. here should be:
>> （smc->clcsock->sk->sk_family != AF_INET）|| 
>> （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）
> 
> Wenjia, I think you happen to confuse something here. The condition
> of this if statement is supposed to evaluate as true iff we don't want
> to propose SMCRv2 because the situation is such that SMCRv2 is not
> supported.
> 
> We have a bunch of conditions we need to meet for SMCRv2 so
> logically we have (A && B && C && D). Now since the if is
> about when SMCRv2 is not supported we have a super structure
> that looks like !A || !B || !C || !D. With this patch, if
> CONFIG_IPV6 is not enabled, the sub-condition remains the same:
> if smc->clcsock->sk->sk_family is something else that AF_INET
> the we do not do SMCRv2!
> 
> But when we do have CONFIG_IPV6 then we want to do SMCRv2 for
> AF_INET6 sockets too if the addresses used are actually
> v4 mapped addresses.
> 
> Now this is where the cognitive dissonance starts on my end. I
> think the author assumes sk_family == AF_INET || sk_family == AF_INET6
> is a tautology in this context. That may be a reasonable thing to
> assume. Under that assumption 
> sk_family != AF_INET &&	!ipv6_addr_v4mapped(addr) (shortened for
> convenience)
> becomes equivalent to
> sk_family == AF_INET6 && !ipv6_addr_v4mapped(addr)
> which means in words if the socket is an IPv6 sockeet and the addr is not
> a v4 mapped v6 address then we *can not* do SMCRv2. And the condition
> when we can is sk_family != AF_INET6 || ipv6_addr_v4mapped(addr) which
> is equivalen to sk_family == AF_INET || ipv6_addr_v4mapped(addr) under
> the aforementioned assumption.

Hi, Halil

Thank you for such a detailed derivation. 

Yes, here assume that sk_family == AF_INET || sk_family == AF_INET6. Indeed,
many codes in SMC have already made this assumption, for example,
static int __smc_create(struct net *net, struct socket *sock, int protocol,
			int kern, struct socket *clcsock)
{
	int family = (protocol == SMCPROTO_SMC6) ? PF_INET6 : PF_INET;
	...
}
And I also believe it is reasonable.

Before this patch, for SMCR client, only an IPV4 socket can do SMCRv2. This patch
introduce an IPV6 socket with v4 mapped v6 address for SMCRv2. It is equivalen
to sk_family == AF_INET || ipv6_addr_v4mapped(addr) as you described.

> 
> But if we assume sk_family == AF_INET || sk_family == AF_INET6 then
> the #else does not make any sense, because I guess with IPv6 not
> available AF_INET6 is not available ant thus the else is always
> guaranteed to evaluate to false under the assumption made.
> 
You are right. The #else here does not make any sense. It's my mistake.

The condition is easier to understand and read should be like this:
 	if (!(ini->smcr_version & SMC_V2) ||
+#if IS_ENABLED(CONFIG_IPV6)
+	    (smc->clcsock->sk->sk_family == AF_INET6 &&
+	     !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
+#endif
 	    !smc_clc_ueid_count() ||
 	    smc_find_rdma_device(smc, ini))
 		ini->smcr_version &= ~SMC_V2;

Thanks,
Guangguan Wang


> Thus I conclude, that I am certainly missing something here. Guangguan,
> do you care to explain?
> 
> Regards,
> Halil
>  

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Wenjia Zhang]

On 06.12.24 07:06, Guangguan Wang wrote:
> 
> 
> On 2024/12/5 20:58, Halil Pasic wrote:
>> On Thu, 5 Dec 2024 11:16:27 +0100
>> Wenjia Zhang <wenjia@linux.ibm.com> wrote:
>>
>>>> --- a/net/smc/af_smc.c
>>>> +++ b/net/smc/af_smc.c
>>>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct
>>>> smc_sock *smc, ini->check_smcrv2 = true;
>>>>    	ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>>>    	if (!(ini->smcr_version & SMC_V2) ||
>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>> +	    (smc->clcsock->sk->sk_family != AF_INET &&
>>>> +
>>>> !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET
>>> && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If
>>> it is, the negativ form of the logical operation (a&&b) is (!a)||(!b),
>>> i.e. here should be:
>>> （smc->clcsock->sk->sk_family != AF_INET）||
>>> （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）
>>
>> Wenjia, I think you happen to confuse something here. The condition
>> of this if statement is supposed to evaluate as true iff we don't want
>> to propose SMCRv2 because the situation is such that SMCRv2 is not
>> supported.
>>
>> We have a bunch of conditions we need to meet for SMCRv2 so
>> logically we have (A && B && C && D). Now since the if is
>> about when SMCRv2 is not supported we have a super structure
>> that looks like !A || !B || !C || !D. With this patch, if
>> CONFIG_IPV6 is not enabled, the sub-condition remains the same:
>> if smc->clcsock->sk->sk_family is something else that AF_INET
>> the we do not do SMCRv2!
>>
>> But when we do have CONFIG_IPV6 then we want to do SMCRv2 for
>> AF_INET6 sockets too if the addresses used are actually
>> v4 mapped addresses.
>>
>> Now this is where the cognitive dissonance starts on my end. I
>> think the author assumes sk_family == AF_INET || sk_family == AF_INET6
>> is a tautology in this context. That may be a reasonable thing to
>> assume. Under that assumption
>> sk_family != AF_INET &&	!ipv6_addr_v4mapped(addr) (shortened for
>> convenience)
>> becomes equivalent to
>> sk_family == AF_INET6 && !ipv6_addr_v4mapped(addr)
>> which means in words if the socket is an IPv6 sockeet and the addr is not
>> a v4 mapped v6 address then we *can not* do SMCRv2. And the condition
>> when we can is sk_family != AF_INET6 || ipv6_addr_v4mapped(addr) which
>> is equivalen to sk_family == AF_INET || ipv6_addr_v4mapped(addr) under
>> the aforementioned assumption.
> 
> Hi, Halil
> 
> Thank you for such a detailed derivation.
> 
> Yes, here assume that sk_family == AF_INET || sk_family == AF_INET6. Indeed,
> many codes in SMC have already made this assumption, for example,
> static int __smc_create(struct net *net, struct socket *sock, int protocol,
> 			int kern, struct socket *clcsock)
> {
> 	int family = (protocol == SMCPROTO_SMC6) ? PF_INET6 : PF_INET;
> 	...
> }
> And I also believe it is reasonable.
> 
> Before this patch, for SMCR client, only an IPV4 socket can do SMCRv2. This patch
> introduce an IPV6 socket with v4 mapped v6 address for SMCRv2. It is equivalen
> to sk_family == AF_INET || ipv6_addr_v4mapped(addr) as you described.
> 
>>
>> But if we assume sk_family == AF_INET || sk_family == AF_INET6 then
>> the #else does not make any sense, because I guess with IPv6 not
>> available AF_INET6 is not available ant thus the else is always
>> guaranteed to evaluate to false under the assumption made.
>>
> You are right. The #else here does not make any sense. It's my mistake.
> 
> The condition is easier to understand and read should be like this:
>   	if (!(ini->smcr_version & SMC_V2) ||
> +#if IS_ENABLED(CONFIG_IPV6)
> +	    (smc->clcsock->sk->sk_family == AF_INET6 &&
> +	     !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
> +#endif
>   	    !smc_clc_ueid_count() ||
>   	    smc_find_rdma_device(smc, ini))
>   		ini->smcr_version &= ~SMC_V2;
> 

sorry, I still don't agree on this version. You removed the condition
"
smc->clcsock->sk->sk_family != AF_INET ||
"
completely. What about the socket with neither AF_INET nor AF_INET6 family?

Thanks,
Wenjia

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Wenjia Zhang]

On 06.12.24 11:51, Wenjia Zhang wrote:
> 
> 
> On 06.12.24 07:06, Guangguan Wang wrote:
>>
>>
>> On 2024/12/5 20:58, Halil Pasic wrote:
>>> On Thu, 5 Dec 2024 11:16:27 +0100
>>> Wenjia Zhang <wenjia@linux.ibm.com> wrote:
>>>
>>>>> --- a/net/smc/af_smc.c
>>>>> +++ b/net/smc/af_smc.c
>>>>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct
>>>>> smc_sock *smc, ini->check_smcrv2 = true;
>>>>>        ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>>>>        if (!(ini->smcr_version & SMC_V2) ||
>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>> +        (smc->clcsock->sk->sk_family != AF_INET &&
>>>>> +
>>>>> !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>>> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET
>>>> && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If
>>>> it is, the negativ form of the logical operation (a&&b) is (!a)||(!b),
>>>> i.e. here should be:
>>>> （smc->clcsock->sk->sk_family != AF_INET）||
>>>> （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）
>>>
>>> Wenjia, I think you happen to confuse something here. The condition
>>> of this if statement is supposed to evaluate as true iff we don't want
>>> to propose SMCRv2 because the situation is such that SMCRv2 is not
>>> supported.
>>>
>>> We have a bunch of conditions we need to meet for SMCRv2 so
>>> logically we have (A && B && C && D). Now since the if is
>>> about when SMCRv2 is not supported we have a super structure
>>> that looks like !A || !B || !C || !D. With this patch, if
>>> CONFIG_IPV6 is not enabled, the sub-condition remains the same:
>>> if smc->clcsock->sk->sk_family is something else that AF_INET
>>> the we do not do SMCRv2!
>>>
>>> But when we do have CONFIG_IPV6 then we want to do SMCRv2 for
>>> AF_INET6 sockets too if the addresses used are actually
>>> v4 mapped addresses.
>>>
>>> Now this is where the cognitive dissonance starts on my end. I
>>> think the author assumes sk_family == AF_INET || sk_family == AF_INET6
>>> is a tautology in this context. That may be a reasonable thing to
>>> assume. Under that assumption
>>> sk_family != AF_INET &&    !ipv6_addr_v4mapped(addr) (shortened for
>>> convenience)
>>> becomes equivalent to
>>> sk_family == AF_INET6 && !ipv6_addr_v4mapped(addr)
>>> which means in words if the socket is an IPv6 sockeet and the addr is 
>>> not
>>> a v4 mapped v6 address then we *can not* do SMCRv2. And the condition
>>> when we can is sk_family != AF_INET6 || ipv6_addr_v4mapped(addr) which
>>> is equivalen to sk_family == AF_INET || ipv6_addr_v4mapped(addr) under
>>> the aforementioned assumption.
>>
>> Hi, Halil
>>
>> Thank you for such a detailed derivation.
>>
>> Yes, here assume that sk_family == AF_INET || sk_family == AF_INET6. 
>> Indeed,
>> many codes in SMC have already made this assumption, for example,
>> static int __smc_create(struct net *net, struct socket *sock, int 
>> protocol,
>>             int kern, struct socket *clcsock)
>> {
>>     int family = (protocol == SMCPROTO_SMC6) ? PF_INET6 : PF_INET;
>>     ...
>> }
>> And I also believe it is reasonable.
>>
>> Before this patch, for SMCR client, only an IPV4 socket can do SMCRv2. 
>> This patch
>> introduce an IPV6 socket with v4 mapped v6 address for SMCRv2. It is 
>> equivalen
>> to sk_family == AF_INET || ipv6_addr_v4mapped(addr) as you described.
>>
>>>
>>> But if we assume sk_family == AF_INET || sk_family == AF_INET6 then
>>> the #else does not make any sense, because I guess with IPv6 not
>>> available AF_INET6 is not available ant thus the else is always
>>> guaranteed to evaluate to false under the assumption made.
>>>
>> You are right. The #else here does not make any sense. It's my mistake.
>>
>> The condition is easier to understand and read should be like this:
>>       if (!(ini->smcr_version & SMC_V2) ||
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +        (smc->clcsock->sk->sk_family == AF_INET6 &&
>> +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>> +#endif
>>           !smc_clc_ueid_count() ||
>>           smc_find_rdma_device(smc, ini))
>>           ini->smcr_version &= ~SMC_V2;
>>
> 
> sorry, I still don't agree on this version. You removed the condition
> "
> smc->clcsock->sk->sk_family != AF_INET ||
> "
> completely. What about the socket with neither AF_INET nor AF_INET6 family?
> 
> Thanks,
> Wenjia
> 
I think the main problem in the original version was that
(sk_family != AF_INET) is not equivalent to (sk_family == AF_INET6).
Since you already in the new version above used sk_family == AF_INET6,
the else condition could stay as it is. My suggestion:

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 8e3093938cd2..5f205a41fc48 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct 
smc_sock *smc,
         ini->check_smcrv2 = true;
         ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
         if (!(ini->smcr_version & SMC_V2) ||
+#if IS_ENABLED(CONFIG_IPV6)
+           (smc->clcsock->sk->sk_family == AF_INET6 &&
+            !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
+#else
             smc->clcsock->sk->sk_family != AF_INET ||
+#endif
             !smc_clc_ueid_count() ||
             smc_find_rdma_device(smc, ini))
                 ini->smcr_version &= ~SMC_V2;

Thanks,
Wenjia

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Guangguan Wang]

On 2024/12/7 03:49, Wenjia Zhang wrote:
> 
> 
> On 06.12.24 11:51, Wenjia Zhang wrote:
>>
>>
>> On 06.12.24 07:06, Guangguan Wang wrote:
>>>
>>>
>>> On 2024/12/5 20:58, Halil Pasic wrote:
>>>> On Thu, 5 Dec 2024 11:16:27 +0100
>>>> Wenjia Zhang <wenjia@linux.ibm.com> wrote:
>>>>
>>>>>> --- a/net/smc/af_smc.c
>>>>>> +++ b/net/smc/af_smc.c
>>>>>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct
>>>>>> smc_sock *smc, ini->check_smcrv2 = true;
>>>>>>        ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>>>>>        if (!(ini->smcr_version & SMC_V2) ||
>>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>>> +        (smc->clcsock->sk->sk_family != AF_INET &&
>>>>>> +
>>>>>> !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>>>> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET
>>>>> && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If
>>>>> it is, the negativ form of the logical operation (a&&b) is (!a)||(!b),
>>>>> i.e. here should be:
>>>>> （smc->clcsock->sk->sk_family != AF_INET）||
>>>>> （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）
>>>>
>>>> Wenjia, I think you happen to confuse something here. The condition
>>>> of this if statement is supposed to evaluate as true iff we don't want
>>>> to propose SMCRv2 because the situation is such that SMCRv2 is not
>>>> supported.
>>>>
>>>> We have a bunch of conditions we need to meet for SMCRv2 so
>>>> logically we have (A && B && C && D). Now since the if is
>>>> about when SMCRv2 is not supported we have a super structure
>>>> that looks like !A || !B || !C || !D. With this patch, if
>>>> CONFIG_IPV6 is not enabled, the sub-condition remains the same:
>>>> if smc->clcsock->sk->sk_family is something else that AF_INET
>>>> the we do not do SMCRv2!
>>>>
>>>> But when we do have CONFIG_IPV6 then we want to do SMCRv2 for
>>>> AF_INET6 sockets too if the addresses used are actually
>>>> v4 mapped addresses.
>>>>
>>>> Now this is where the cognitive dissonance starts on my end. I
>>>> think the author assumes sk_family == AF_INET || sk_family == AF_INET6
>>>> is a tautology in this context. That may be a reasonable thing to
>>>> assume. Under that assumption
>>>> sk_family != AF_INET &&    !ipv6_addr_v4mapped(addr) (shortened for
>>>> convenience)
>>>> becomes equivalent to
>>>> sk_family == AF_INET6 && !ipv6_addr_v4mapped(addr)
>>>> which means in words if the socket is an IPv6 sockeet and the addr is not
>>>> a v4 mapped v6 address then we *can not* do SMCRv2. And the condition
>>>> when we can is sk_family != AF_INET6 || ipv6_addr_v4mapped(addr) which
>>>> is equivalen to sk_family == AF_INET || ipv6_addr_v4mapped(addr) under
>>>> the aforementioned assumption.
>>>
>>> Hi, Halil
>>>
>>> Thank you for such a detailed derivation.
>>>
>>> Yes, here assume that sk_family == AF_INET || sk_family == AF_INET6. Indeed,
>>> many codes in SMC have already made this assumption, for example,
>>> static int __smc_create(struct net *net, struct socket *sock, int protocol,
>>>             int kern, struct socket *clcsock)
>>> {
>>>     int family = (protocol == SMCPROTO_SMC6) ? PF_INET6 : PF_INET;
>>>     ...
>>> }
>>> And I also believe it is reasonable.
>>>
>>> Before this patch, for SMCR client, only an IPV4 socket can do SMCRv2. This patch
>>> introduce an IPV6 socket with v4 mapped v6 address for SMCRv2. It is equivalen
>>> to sk_family == AF_INET || ipv6_addr_v4mapped(addr) as you described.
>>>
>>>>
>>>> But if we assume sk_family == AF_INET || sk_family == AF_INET6 then
>>>> the #else does not make any sense, because I guess with IPv6 not
>>>> available AF_INET6 is not available ant thus the else is always
>>>> guaranteed to evaluate to false under the assumption made.
>>>>
>>> You are right. The #else here does not make any sense. It's my mistake.
>>>
>>> The condition is easier to understand and read should be like this:
>>>       if (!(ini->smcr_version & SMC_V2) ||
>>> +#if IS_ENABLED(CONFIG_IPV6)
>>> +        (smc->clcsock->sk->sk_family == AF_INET6 &&
>>> +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>> +#endif
>>>           !smc_clc_ueid_count() ||
>>>           smc_find_rdma_device(smc, ini))
>>>           ini->smcr_version &= ~SMC_V2;
>>>
>>
>> sorry, I still don't agree on this version. You removed the condition
>> "
>> smc->clcsock->sk->sk_family != AF_INET ||
>> "
>> completely. What about the socket with neither AF_INET nor AF_INET6 family?
>>
>> Thanks,
>> Wenjia
>>
> I think the main problem in the original version was that
> (sk_family != AF_INET) is not equivalent to (sk_family == AF_INET6).
> Since you already in the new version above used sk_family == AF_INET6,
> the else condition could stay as it is. My suggestion:
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 8e3093938cd2..5f205a41fc48 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct smc_sock *smc,
>         ini->check_smcrv2 = true;
>         ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>         if (!(ini->smcr_version & SMC_V2) ||
> +#if IS_ENABLED(CONFIG_IPV6)
> +           (smc->clcsock->sk->sk_family == AF_INET6 &&
> +            !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
> +#else
>             smc->clcsock->sk->sk_family != AF_INET ||
> +#endif
>             !smc_clc_ueid_count() ||
>             smc_find_rdma_device(smc, ini))
>                 ini->smcr_version &= ~SMC_V2;
> 
> Thanks,
> Wenjia

The RFC7609 have confined SMC to socket applications using stream (i.e., TCP) sockets over IPv4 or IPv6.
https://datatracker.ietf.org/doc/html/rfc7609#page-26:~:text=It%20is%20confined%20to%20socket%20applications%20using%20stream%0A%20%20%20(i.e.%2C%20TCP)%20sockets%20over%20IPv4%20or%20IPv6

Both in the smc-tools and in smc kernel module, we can see codes that the sk_family is either AF_INET or AF_INET6.
The codes here:
https://raw.githubusercontent.com/ibm-s390-linux/smc-tools/refs/heads/main/smc-preload.c#:~:text=if%20((domain%20%3D%3D%20AF_INET%20%7C%7C%20domain%20%3D%3D%20AF_INET6)%20%26%26
and
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=(sk%2D%3Esk_family%20!%3D%20AF_INET%20%26%26%20sk%2D%3Esk_family%20!%3D%20AF_INET6))
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=int%20family%20%3D%20(protocol%20%3D%3D%20SMCPROTO_SMC6)%20%3F%20PF_INET6%20%3A%20PF_INET%3B 
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=%2D%3Esin_family%20!%3D-,AF_INET,-%26%26%0A%09%20%20%20%20addr%2D%3E
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=%2D%3Esa_family%20!%3D-,AF_INET6,-)%0A%09%09goto%20out_err
...

I wonder if SMC-R can support other address famliy rather than AF_INET AF_INET6 in design？
And IBM has any plan to support other address family in future?  Wenjia, can you help explain
this?

If the answer is positive, the code should be like this:
        if (!(ini->smcr_version & SMC_V2) ||
+#if IS_ENABLED(CONFIG_IPV6)
+           !(smc->clcsock->sk->sk_family == AF_INET || (smc->clcsock->sk->sk_family == AF_INET6 &&
+            ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr))) ||
+#else
             smc->clcsock->sk->sk_family != AF_INET ||
+#endif
             !smc_clc_ueid_count() ||
             smc_find_rdma_device(smc, ini))
                 ini->smcr_version &= ~SMC_V2;

Otherwise, the code below is reasonable.
      if (!(ini->smcr_version & SMC_V2) ||
+#if IS_ENABLED(CONFIG_IPV6)
+        (smc->clcsock->sk->sk_family == AF_INET6 &&
+         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
+#endif
          !smc_clc_ueid_count() ||
          smc_find_rdma_device(smc, ini))
          ini->smcr_version &= ~SMC_V2;

Thanks,
Guangguan Wang

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Wenjia Zhang]

On 09.12.24 07:04, Guangguan Wang wrote:
> 
> 
> On 2024/12/7 03:49, Wenjia Zhang wrote:
>>
>>
>> On 06.12.24 11:51, Wenjia Zhang wrote:
>>>
>>>
>>> On 06.12.24 07:06, Guangguan Wang wrote:
>>>>
>>>>
>>>> On 2024/12/5 20:58, Halil Pasic wrote:
>>>>> On Thu, 5 Dec 2024 11:16:27 +0100
>>>>> Wenjia Zhang <wenjia@linux.ibm.com> wrote:
>>>>>
>>>>>>> --- a/net/smc/af_smc.c
>>>>>>> +++ b/net/smc/af_smc.c
>>>>>>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct
>>>>>>> smc_sock *smc, ini->check_smcrv2 = true;
>>>>>>>         ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>>>>>>         if (!(ini->smcr_version & SMC_V2) ||
>>>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>>>> +        (smc->clcsock->sk->sk_family != AF_INET &&
>>>>>>> +
>>>>>>> !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>>>>> I think here you want to say !(smc->clcsock->sk->sk_family == AF_INET
>>>>>> && ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)), right? If
>>>>>> it is, the negativ form of the logical operation (a&&b) is (!a)||(!b),
>>>>>> i.e. here should be:
>>>>>> （smc->clcsock->sk->sk_family != AF_INET）||
>>>>>> （!ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)）
>>>>>
>>>>> Wenjia, I think you happen to confuse something here. The condition
>>>>> of this if statement is supposed to evaluate as true iff we don't want
>>>>> to propose SMCRv2 because the situation is such that SMCRv2 is not
>>>>> supported.
>>>>>
>>>>> We have a bunch of conditions we need to meet for SMCRv2 so
>>>>> logically we have (A && B && C && D). Now since the if is
>>>>> about when SMCRv2 is not supported we have a super structure
>>>>> that looks like !A || !B || !C || !D. With this patch, if
>>>>> CONFIG_IPV6 is not enabled, the sub-condition remains the same:
>>>>> if smc->clcsock->sk->sk_family is something else that AF_INET
>>>>> the we do not do SMCRv2!
>>>>>
>>>>> But when we do have CONFIG_IPV6 then we want to do SMCRv2 for
>>>>> AF_INET6 sockets too if the addresses used are actually
>>>>> v4 mapped addresses.
>>>>>
>>>>> Now this is where the cognitive dissonance starts on my end. I
>>>>> think the author assumes sk_family == AF_INET || sk_family == AF_INET6
>>>>> is a tautology in this context. That may be a reasonable thing to
>>>>> assume. Under that assumption
>>>>> sk_family != AF_INET &&    !ipv6_addr_v4mapped(addr) (shortened for
>>>>> convenience)
>>>>> becomes equivalent to
>>>>> sk_family == AF_INET6 && !ipv6_addr_v4mapped(addr)
>>>>> which means in words if the socket is an IPv6 sockeet and the addr is not
>>>>> a v4 mapped v6 address then we *can not* do SMCRv2. And the condition
>>>>> when we can is sk_family != AF_INET6 || ipv6_addr_v4mapped(addr) which
>>>>> is equivalen to sk_family == AF_INET || ipv6_addr_v4mapped(addr) under
>>>>> the aforementioned assumption.
>>>>
>>>> Hi, Halil
>>>>
>>>> Thank you for such a detailed derivation.
>>>>
>>>> Yes, here assume that sk_family == AF_INET || sk_family == AF_INET6. Indeed,
>>>> many codes in SMC have already made this assumption, for example,
>>>> static int __smc_create(struct net *net, struct socket *sock, int protocol,
>>>>              int kern, struct socket *clcsock)
>>>> {
>>>>      int family = (protocol == SMCPROTO_SMC6) ? PF_INET6 : PF_INET;
>>>>      ...
>>>> }
>>>> And I also believe it is reasonable.
>>>>
>>>> Before this patch, for SMCR client, only an IPV4 socket can do SMCRv2. This patch
>>>> introduce an IPV6 socket with v4 mapped v6 address for SMCRv2. It is equivalen
>>>> to sk_family == AF_INET || ipv6_addr_v4mapped(addr) as you described.
>>>>
>>>>>
>>>>> But if we assume sk_family == AF_INET || sk_family == AF_INET6 then
>>>>> the #else does not make any sense, because I guess with IPv6 not
>>>>> available AF_INET6 is not available ant thus the else is always
>>>>> guaranteed to evaluate to false under the assumption made.
>>>>>
>>>> You are right. The #else here does not make any sense. It's my mistake.
>>>>
>>>> The condition is easier to understand and read should be like this:
>>>>        if (!(ini->smcr_version & SMC_V2) ||
>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>> +        (smc->clcsock->sk->sk_family == AF_INET6 &&
>>>> +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>>> +#endif
>>>>            !smc_clc_ueid_count() ||
>>>>            smc_find_rdma_device(smc, ini))
>>>>            ini->smcr_version &= ~SMC_V2;
>>>>
>>>
>>> sorry, I still don't agree on this version. You removed the condition
>>> "
>>> smc->clcsock->sk->sk_family != AF_INET ||
>>> "
>>> completely. What about the socket with neither AF_INET nor AF_INET6 family?
>>>
>>> Thanks,
>>> Wenjia
>>>
>> I think the main problem in the original version was that
>> (sk_family != AF_INET) is not equivalent to (sk_family == AF_INET6).
>> Since you already in the new version above used sk_family == AF_INET6,
>> the else condition could stay as it is. My suggestion:
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 8e3093938cd2..5f205a41fc48 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -1116,7 +1116,12 @@ static int smc_find_proposal_devices(struct smc_sock *smc,
>>          ini->check_smcrv2 = true;
>>          ini->smcrv2.saddr = smc->clcsock->sk->sk_rcv_saddr;
>>          if (!(ini->smcr_version & SMC_V2) ||
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +           (smc->clcsock->sk->sk_family == AF_INET6 &&
>> +            !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>> +#else
>>              smc->clcsock->sk->sk_family != AF_INET ||
>> +#endif
>>              !smc_clc_ueid_count() ||
>>              smc_find_rdma_device(smc, ini))
>>                  ini->smcr_version &= ~SMC_V2;
>>
>> Thanks,
>> Wenjia
> 
> The RFC7609 have confined SMC to socket applications using stream (i.e., TCP) sockets over IPv4 or IPv6.
> https://datatracker.ietf.org/doc/html/rfc7609#page-26:~:text=It%20is%20confined%20to%20socket%20applications%20using%20stream%0A%20%20%20(i.e.%2C%20TCP)%20sockets%20over%20IPv4%20or%20IPv6
> 
> Both in the smc-tools and in smc kernel module, we can see codes that the sk_family is either AF_INET or AF_INET6.
> The codes here:
> https://raw.githubusercontent.com/ibm-s390-linux/smc-tools/refs/heads/main/smc-preload.c#:~:text=if%20((domain%20%3D%3D%20AF_INET%20%7C%7C%20domain%20%3D%3D%20AF_INET6)%20%26%26
> and
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=(sk%2D%3Esk_family%20!%3D%20AF_INET%20%26%26%20sk%2D%3Esk_family%20!%3D%20AF_INET6))
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=int%20family%20%3D%20(protocol%20%3D%3D%20SMCPROTO_SMC6)%20%3F%20PF_INET6%20%3A%20PF_INET%3B
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=%2D%3Esin_family%20!%3D-,AF_INET,-%26%26%0A%09%20%20%20%20addr%2D%3E
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/smc/af_smc.c#:~:text=%2D%3Esa_family%20!%3D-,AF_INET6,-)%0A%09%09goto%20out_err
> ...
> 
> I wonder if SMC-R can support other address famliy rather than AF_INET AF_INET6 in design？
> And IBM has any plan to support other address family in future?  Wenjia, can you help explain
> this?
> 
The answer is no, at least in the near future. As you might be already 
aware, it depends on the implementation on z/OS.

> If the answer is positive, the code should be like this:
>          if (!(ini->smcr_version & SMC_V2) ||
> +#if IS_ENABLED(CONFIG_IPV6)
> +           !(smc->clcsock->sk->sk_family == AF_INET || (smc->clcsock->sk->sk_family == AF_INET6 &&
> +            ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr))) ||
> +#else
>               smc->clcsock->sk->sk_family != AF_INET ||
> +#endif
>               !smc_clc_ueid_count() ||
>               smc_find_rdma_device(smc, ini))
>                   ini->smcr_version &= ~SMC_V2;
> 
> Otherwise, the code below is reasonable.
>        if (!(ini->smcr_version & SMC_V2) ||
> +#if IS_ENABLED(CONFIG_IPV6)
> +        (smc->clcsock->sk->sk_family == AF_INET6 &&
> +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
> +#endif
>            !smc_clc_ueid_count() ||
>            smc_find_rdma_device(smc, ini))
>            ini->smcr_version &= ~SMC_V2;
> 
Ok, I got your point, a socket with an address family other than AF_INET 
and AF_INET6 is already pre-filtered, so that such extra condition 
checking for the smc->clcsock->sk->sk_family != AF_INET is not 
necessary, right?

Would you like to send a new version? And feel free to use this in the 
new version:

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

Thanks,
Wenjia

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Halil Pasic]

On Mon, 9 Dec 2024 09:49:23 +0100
Wenjia Zhang <wenjia@linux.ibm.com> wrote:

> > Otherwise, the code below is reasonable.
> >        if (!(ini->smcr_version & SMC_V2) ||
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +        (smc->clcsock->sk->sk_family == AF_INET6 &&
> > +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
> > +#endif
> >            !smc_clc_ueid_count() ||
> >            smc_find_rdma_device(smc, ini))
> >            ini->smcr_version &= ~SMC_V2;
> >   
> Ok, I got your point, a socket with an address family other than AF_INET 
> and AF_INET6 is already pre-filtered, so that such extra condition 
> checking for the smc->clcsock->sk->sk_family != AF_INET is not 
> necessary, right?
> 
> Would you like to send a new version? And feel free to use this in the 
> new version:
> 
> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>

I believe we would like to have a v3 here. Also I'm not sure
checking on saddr is sufficient, but I didn't do my research on
that question yet.

Regards,
Halil

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Guangguan Wang]

On 2024/12/9 17:46, Halil Pasic wrote:
> On Mon, 9 Dec 2024 09:49:23 +0100
> Wenjia Zhang <wenjia@linux.ibm.com> wrote:
> 
>>> Otherwise, the code below is reasonable.
>>>        if (!(ini->smcr_version & SMC_V2) ||
>>> +#if IS_ENABLED(CONFIG_IPV6)
>>> +        (smc->clcsock->sk->sk_family == AF_INET6 &&
>>> +         !ipv6_addr_v4mapped(&smc->clcsock->sk->sk_v6_rcv_saddr)) ||
>>> +#endif
>>>            !smc_clc_ueid_count() ||
>>>            smc_find_rdma_device(smc, ini))
>>>            ini->smcr_version &= ~SMC_V2;
>>>   
>> Ok, I got your point, a socket with an address family other than AF_INET 
>> and AF_INET6 is already pre-filtered, so that such extra condition 
>> checking for the smc->clcsock->sk->sk_family != AF_INET is not 
>> necessary, right?
>>
>> Would you like to send a new version? And feel free to use this in the 
>> new version:
>>
>> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> 
> I believe we would like to have a v3 here. Also I'm not sure
> checking on saddr is sufficient, but I didn't do my research on
> that question yet.
> 
> Regards,
> Halil

Did you mean to research whether the daddr should be checked too?

Thanks,
Guangguan Wang

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Halil Pasic]

On Mon, 9 Dec 2024 20:36:45 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> > I believe we would like to have a v3 here. Also I'm not sure
> > checking on saddr is sufficient, but I didn't do my research on
> > that question yet.
> > 
> > Regards,
> > Halil  
> 
> Did you mean to research whether the daddr should be checked too?

Right! Or is it implied that if saddr is a ipv4 mapped ipv6 addr
then the daddr must be ipv4 mapped ipv6 addr as well?

Regards,
Halil

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Guangguan Wang]

On 2024/12/10 04:44, Halil Pasic wrote:
> On Mon, 9 Dec 2024 20:36:45 +0800
> Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
> 
>>> I believe we would like to have a v3 here. Also I'm not sure
>>> checking on saddr is sufficient, but I didn't do my research on
>>> that question yet.
>>>
>>> Regards,
>>> Halil  
>>
>> Did you mean to research whether the daddr should be checked too?
> 
> Right! Or is it implied that if saddr is a ipv4 mapped ipv6 addr
> then the daddr must be ipv4 mapped ipv6 addr as well?
> 
> Regards,
> Halil

I did a test by iperf3:
A server with IPV4 addr 11.213.5.33/24 and real IPV6 addr 2012::1/64.
A client with IPV4 addr 11.213.5.5/24 and real IPV6 addr 2012::2/64.
iperf3 fails to run when server listen on IPV6 addr and client connect
to server using IPV4 mapped IPV6 addr. commands show below:
server: smc_run iperf3 -s -6 -B 2012::1
client: smc_run iperf3 -t 10 -c 2012::1 -6 -B ::ffff:11.213.5.5

Failure happened due to the connect() function got the errno -EAFNOSUPPORT,
I also located the kernel code where the -EAFNOSUPPORT is returned
(https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ipv6/ip6_output.c#:~:text=err%20%3D%20%2DEAFNOSUPPORT%3B).
The call stack is:
ip6_dst_lookup_tail+1
ip6_dst_lookup_flow+55
tcp_v6_connect+743
__inet_stream_connect+181
inet_stream_connect+59
kernel_connect+109
smc_connect+239
__sys_connect+179
__x64_sys_connect+26
do_syscall_64+112
entry_SYSCALL_64_after_hwframe+118

The kernel code mentioned above restricts that when the saddr is ipv4
mapped ipv6 addr, the daddr should be either ipv4 mapped ipv6 addr or
ipv6_addr_any(::). As far as I know, the ipv6_addr_any daddr is used
to connect to a server listen on ipv6_addr_any(::) by loopback connection.

Thus, based on the test and the code, I think it has the restrict that for
SMC-Rv2 if saddr is a ipv4 mapped ipv6 addr then the daddr must be
ipv4 mapped ipv6 addr as well.

Thanks,
Guangguan Wang

Re: [PATCH net-next v2 2/2] net/smc: support ipv4 mapped ipv6 addr client for smc-r v2

[Halil Pasic]

On Tue, 10 Dec 2024 15:07:04 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> On 2024/12/10 04:44, Halil Pasic wrote:
> > On Mon, 9 Dec 2024 20:36:45 +0800
> > Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
> >   
> >>> I believe we would like to have a v3 here. Also I'm not sure
> >>> checking on saddr is sufficient, but I didn't do my research on
> >>> that question yet.
> >>>
> >>> Regards,
> >>> Halil    
> >>
> >> Did you mean to research whether the daddr should be checked too?  
> > 
> > Right! Or is it implied that if saddr is a ipv4 mapped ipv6 addr
> > then the daddr must be ipv4 mapped ipv6 addr as well?
> > 
> > Regards,
> > Halil  
> 
> I did a test by iperf3:
> A server with IPV4 addr 11.213.5.33/24 and real IPV6 addr 2012::1/64.
> A client with IPV4 addr 11.213.5.5/24 and real IPV6 addr 2012::2/64.
> iperf3 fails to run when server listen on IPV6 addr and client connect
> to server using IPV4 mapped IPV6 addr. commands show below:
> server: smc_run iperf3 -s -6 -B 2012::1
> client: smc_run iperf3 -t 10 -c 2012::1 -6 -B ::ffff:11.213.5.5
> 
> Failure happened due to the connect() function got the errno -EAFNOSUPPORT,
> I also located the kernel code where the -EAFNOSUPPORT is returned
> (https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ipv6/ip6_output.c#:~:text=err%20%3D%20%2DEAFNOSUPPORT%3B).

Thanks! That is enough for me!

Re: [PATCH net-next v2 0/2] net/smc: Two features for smc-r

[Jakub Kicinski]

On Mon,  2 Dec 2024 20:52:01 +0800 Guangguan Wang wrote:
> net/smc: Two features for smc-r

changes look relatively straightforward, IBM folks do you need more
time to review?

Re: [PATCH net-next v2 0/2] net/smc: Two features for smc-r

[Wenjia Zhang]

On 05.12.24 03:39, Jakub Kicinski wrote:
> On Mon,  2 Dec 2024 20:52:01 +0800 Guangguan Wang wrote:
>> net/smc: Two features for smc-r
> 
> changes look relatively straightforward, IBM folks do you need more
> time to review?

Hi Jakub,

Thank you for the reminder! Yes, we still need a bit more time.

Thanks,
Wenjia