linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] providers/rxe: Set the correct value of resid for inline data
@ 2021-08-09 15:07 Xiao Yang
  2021-08-10  3:40 ` Zhu Yanjun
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Xiao Yang @ 2021-08-09 15:07 UTC (permalink / raw)
  To: linux-rdma, leon; +Cc: rpearsonhpe, zyjzyj2000, jgg, Xiao Yang

Resid indicates the residual length of transmitted bytes but current
rxe sets it to zero for inline data at the beginning.  In this case,
request will transmit zero byte to responder wrongly.

Resid should be set to the total length of transmitted bytes at the
beginning.

Note:
Just remove the useless setting of resid in init_send_wqe().

Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 providers/rxe/rxe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
index 3c3ea8bb..3533a325 100644
--- a/providers/rxe/rxe.c
+++ b/providers/rxe/rxe.c
@@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
 
 	memcpy(wqe->dma.inline_data, addr, length);
 	wqe->dma.length = length;
-	wqe->dma.resid = 0;
+	wqe->dma.resid = length;
 }
 
 static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
@@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
 	}
 
 	wqe->dma.length = tot_length;
+	wqe->dma.resid = tot_length;
 }
 
 static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
@@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
 	if (ibwr->send_flags & IBV_SEND_INLINE) {
 		uint8_t *inline_data = wqe->dma.inline_data;
 
-		wqe->dma.resid = 0;
-
 		for (i = 0; i < num_sge; i++) {
 			memcpy(inline_data,
 			       (uint8_t *)(long)ibwr->sg_list[i].addr,
-- 
2.25.1




^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-09 15:07 [PATCH v2] providers/rxe: Set the correct value of resid for inline data Xiao Yang
@ 2021-08-10  3:40 ` Zhu Yanjun
  2021-08-10 16:33   ` yangx.jy
  2021-08-13 22:11 ` Bob Pearson
  2021-09-26 10:51 ` Leon Romanovsky
  2 siblings, 1 reply; 17+ messages in thread
From: Zhu Yanjun @ 2021-08-10  3:40 UTC (permalink / raw)
  To: Xiao Yang
  Cc: RDMA mailing list, Leon Romanovsky, Bob Pearson, Jason Gunthorpe

On Mon, Aug 9, 2021 at 10:43 PM Xiao Yang <yangx.jy@fujitsu.com> wrote:
>
> Resid indicates the residual length of transmitted bytes but current
> rxe sets it to zero for inline data at the beginning.  In this case,
> request will transmit zero byte to responder wrongly.

What are the symptoms if resid is set to zero?

Thanks
Zhu Yanjun

>
> Resid should be set to the total length of transmitted bytes at the
> beginning.
>
> Note:
> Just remove the useless setting of resid in init_send_wqe().
>
> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  providers/rxe/rxe.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> index 3c3ea8bb..3533a325 100644
> --- a/providers/rxe/rxe.c
> +++ b/providers/rxe/rxe.c
> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>
>         memcpy(wqe->dma.inline_data, addr, length);
>         wqe->dma.length = length;
> -       wqe->dma.resid = 0;
> +       wqe->dma.resid = length;
>  }
>
>  static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>         }
>
>         wqe->dma.length = tot_length;
> +       wqe->dma.resid = tot_length;
>  }
>
>  static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>         if (ibwr->send_flags & IBV_SEND_INLINE) {
>                 uint8_t *inline_data = wqe->dma.inline_data;
>
> -               wqe->dma.resid = 0;
> -
>                 for (i = 0; i < num_sge; i++) {
>                         memcpy(inline_data,
>                                (uint8_t *)(long)ibwr->sg_list[i].addr,
> --
> 2.25.1
>
>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-10  3:40 ` Zhu Yanjun
@ 2021-08-10 16:33   ` yangx.jy
  2021-08-16  7:47     ` Zhu Yanjun
  0 siblings, 1 reply; 17+ messages in thread
From: yangx.jy @ 2021-08-10 16:33 UTC (permalink / raw)
  To: Zhu Yanjun
  Cc: RDMA mailing list, Leon Romanovsky, Bob Pearson, Jason Gunthorpe

[-- Attachment #1: Type: text/plain, Size: 3041 bytes --]

On 2021/8/10 11:40, Zhu Yanjun wrote:
> On Mon, Aug 9, 2021 at 10:43 PM Xiao Yang <yangx.jy@fujitsu.com> wrote:
>> Resid indicates the residual length of transmitted bytes but current
>> rxe sets it to zero for inline data at the beginning.  In this case,
>> request will transmit zero byte to responder wrongly.
> What are the symptoms if resid is set to zero?
Hi Yanjun,

You can construct code by the attached patch and then run
rdma_client/server to reproduce the issue.

If resid is set to zero, running rdma_client/server:
--------------------------------
# ./rdma_client -s 10.167.220.99 -p 8765
rdma_client: start
rdma_client: end 0

# ./rdma_server -s 10.167.220.99 -p 8765
rdma_server: start
wc.byte_len 0 recv_msg bbbbbbbbbbbbbbbb
rdma_server: end 0
--------------------------------

rdma_client sends zero byte instead of 16 btyes to rdma_server.
rdma_server receives zero byte instead of 16 btyes from rdma_client.

Please also see the logic about resid in kernel, for example:
--------------------------------
int rxe_requester(void *arg)
{
...
payload = (mask & RXE_WRITE_OR_SEND) ? wqe->dma.resid : 0;
...
skb = init_req_packet(qp, wqe, opcode, payload, &pkt);
...
}
--------------------------------

Best Regards,
Xiao Yang
> Thanks
> Zhu Yanjun
>
>> Resid should be set to the total length of transmitted bytes at the
>> beginning.
>>
>> Note:
>> Just remove the useless setting of resid in init_send_wqe().
>>
>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
>> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
>> ---
>>  providers/rxe/rxe.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>> index 3c3ea8bb..3533a325 100644
>> --- a/providers/rxe/rxe.c
>> +++ b/providers/rxe/rxe.c
>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>>
>>         memcpy(wqe->dma.inline_data, addr, length);
>>         wqe->dma.length = length;
>> -       wqe->dma.resid = 0;
>> +       wqe->dma.resid = length;
>>  }
>>
>>  static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>         }
>>
>>         wqe->dma.length = tot_length;
>> +       wqe->dma.resid = tot_length;
>>  }
>>
>>  static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>         if (ibwr->send_flags & IBV_SEND_INLINE) {
>>                 uint8_t *inline_data = wqe->dma.inline_data;
>>
>> -               wqe->dma.resid = 0;
>> -
>>                 for (i = 0; i < num_sge; i++) {
>>                         memcpy(inline_data,
>>                                (uint8_t *)(long)ibwr->sg_list[i].addr,
>> --
>> 2.25.1
>>
>>
>>


[-- Attachment #2: 0001-rxe-construct-code-and-reproduce-the-issue-by-rdma_c.patch --]
[-- Type: text/plain, Size: 1968 bytes --]

From 5f2ff0e6f5ee3c4431a27e6bfba0868ad59ef56f Mon Sep 17 00:00:00 2001
From: Xiao Yang <yangx.jy@fujitsu.com>
Date: Wed, 11 Aug 2021 00:46:31 +0800
Subject: [PATCH] rxe: construct code and reproduce the issue by
 rdma_client/server

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 librdmacm/examples/rdma_client.c | 2 ++
 librdmacm/examples/rdma_server.c | 4 ++++
 providers/rxe/rxe.c              | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/librdmacm/examples/rdma_client.c b/librdmacm/examples/rdma_client.c
index c27047c5..6e568110 100644
--- a/librdmacm/examples/rdma_client.c
+++ b/librdmacm/examples/rdma_client.c
@@ -52,6 +52,8 @@ static int run(void)
 	struct ibv_wc wc;
 	int ret;
 
+	memset(send_msg, 'a', 16);
+
 	memset(&hints, 0, sizeof hints);
 	hints.ai_port_space = RDMA_PS_TCP;
 	ret = rdma_getaddrinfo(server, port, &hints, &res);
diff --git a/librdmacm/examples/rdma_server.c b/librdmacm/examples/rdma_server.c
index f9c766b2..ffb440c6 100644
--- a/librdmacm/examples/rdma_server.c
+++ b/librdmacm/examples/rdma_server.c
@@ -53,6 +53,8 @@ static int run(void)
 	struct ibv_wc wc;
 	int ret;
 
+	memset(recv_msg, 'b', 16);
+
 	memset(&hints, 0, sizeof hints);
 	hints.ai_flags = RAI_PASSIVE;
 	hints.ai_port_space = RDMA_PS_TCP;
@@ -132,6 +134,8 @@ static int run(void)
 		goto out_disconnect;
 	}
 
+	printf("wc.byte_len %u recv_msg %s\n", wc.byte_len, recv_msg);
+
 	ret = rdma_post_send(id, NULL, send_msg, 16, send_mr, send_flags);
 	if (ret) {
 		perror("rdma_post_send");
diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
index 3c3ea8bb..ed5479fe 100644
--- a/providers/rxe/rxe.c
+++ b/providers/rxe/rxe.c
@@ -1492,7 +1492,7 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
 		wqe->iova	= ibwr->wr.rdma.remote_addr;
 
 	wqe->dma.length		= length;
-	wqe->dma.resid		= length;
+	wqe->dma.resid		= 0;
 	wqe->dma.num_sge	= num_sge;
 	wqe->dma.cur_sge	= 0;
 	wqe->dma.sge_offset	= 0;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-09 15:07 [PATCH v2] providers/rxe: Set the correct value of resid for inline data Xiao Yang
  2021-08-10  3:40 ` Zhu Yanjun
@ 2021-08-13 22:11 ` Bob Pearson
  2021-08-16  3:55   ` Zhu Yanjun
  2021-09-26 10:51 ` Leon Romanovsky
  2 siblings, 1 reply; 17+ messages in thread
From: Bob Pearson @ 2021-08-13 22:11 UTC (permalink / raw)
  To: Xiao Yang, linux-rdma, leon; +Cc: zyjzyj2000, jgg

On 8/9/21 10:07 AM, Xiao Yang wrote:
> Resid indicates the residual length of transmitted bytes but current
> rxe sets it to zero for inline data at the beginning.  In this case,
> request will transmit zero byte to responder wrongly.
> 
> Resid should be set to the total length of transmitted bytes at the
> beginning.
> 
> Note:
> Just remove the useless setting of resid in init_send_wqe().
> 
> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  providers/rxe/rxe.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> index 3c3ea8bb..3533a325 100644
> --- a/providers/rxe/rxe.c
> +++ b/providers/rxe/rxe.c
> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>  
>  	memcpy(wqe->dma.inline_data, addr, length);
>  	wqe->dma.length = length;
> -	wqe->dma.resid = 0;
> +	wqe->dma.resid = length;
>  }
>  
>  static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>  	}
>  
>  	wqe->dma.length = tot_length;
> +	wqe->dma.resid = tot_length;
>  }
>  
>  static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>  	if (ibwr->send_flags & IBV_SEND_INLINE) {
>  		uint8_t *inline_data = wqe->dma.inline_data;
>  
> -		wqe->dma.resid = 0;
> -
>  		for (i = 0; i < num_sge; i++) {
>  			memcpy(inline_data,
>  			       (uint8_t *)(long)ibwr->sg_list[i].addr,
> 

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-13 22:11 ` Bob Pearson
@ 2021-08-16  3:55   ` Zhu Yanjun
  2021-08-16 18:52     ` Bob Pearson
  0 siblings, 1 reply; 17+ messages in thread
From: Zhu Yanjun @ 2021-08-16  3:55 UTC (permalink / raw)
  To: Bob Pearson
  Cc: Xiao Yang, RDMA mailing list, Leon Romanovsky, Jason Gunthorpe

On Sat, Aug 14, 2021 at 6:11 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 8/9/21 10:07 AM, Xiao Yang wrote:
> > Resid indicates the residual length of transmitted bytes but current
> > rxe sets it to zero for inline data at the beginning.  In this case,
> > request will transmit zero byte to responder wrongly.
> >
> > Resid should be set to the total length of transmitted bytes at the
> > beginning.
> >
> > Note:
> > Just remove the useless setting of resid in init_send_wqe().
> >
> > Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> > Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> > ---
> >  providers/rxe/rxe.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> > index 3c3ea8bb..3533a325 100644
> > --- a/providers/rxe/rxe.c
> > +++ b/providers/rxe/rxe.c
> > @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
> >
> >       memcpy(wqe->dma.inline_data, addr, length);
> >       wqe->dma.length = length;
> > -     wqe->dma.resid = 0;
> > +     wqe->dma.resid = length;
> >  }
> >
> >  static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> > @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >       }
> >
> >       wqe->dma.length = tot_length;
> > +     wqe->dma.resid = tot_length;
> >  }
> >
> >  static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
> > @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
> >       if (ibwr->send_flags & IBV_SEND_INLINE) {
> >               uint8_t *inline_data = wqe->dma.inline_data;
> >
> > -             wqe->dma.resid = 0;
> > -
> >               for (i = 0; i < num_sge; i++) {
> >                       memcpy(inline_data,
> >                              (uint8_t *)(long)ibwr->sg_list[i].addr,
> >
>
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>

The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch’s delivery
path.

Zhu Yanjun

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-10 16:33   ` yangx.jy
@ 2021-08-16  7:47     ` Zhu Yanjun
  2021-08-17  0:54       ` yangx.jy
  0 siblings, 1 reply; 17+ messages in thread
From: Zhu Yanjun @ 2021-08-16  7:47 UTC (permalink / raw)
  To: yangx.jy; +Cc: RDMA mailing list, Leon Romanovsky, Bob Pearson, Jason Gunthorpe

On Wed, Aug 11, 2021 at 12:33 AM yangx.jy@fujitsu.com
<yangx.jy@fujitsu.com> wrote:
>
> On 2021/8/10 11:40, Zhu Yanjun wrote:
> > On Mon, Aug 9, 2021 at 10:43 PM Xiao Yang <yangx.jy@fujitsu.com> wrote:
> >> Resid indicates the residual length of transmitted bytes but current
> >> rxe sets it to zero for inline data at the beginning.  In this case,
> >> request will transmit zero byte to responder wrongly.
> > What are the symptoms if resid is set to zero?
> Hi Yanjun,
>
> You can construct code by the attached patch and then run
> rdma_client/server to reproduce the issue.
>
> If resid is set to zero, running rdma_client/server:
> --------------------------------
> # ./rdma_client -s 10.167.220.99 -p 8765
> rdma_client: start
> rdma_client: end 0
>
> # ./rdma_server -s 10.167.220.99 -p 8765
> rdma_server: start
> wc.byte_len 0 recv_msg bbbbbbbbbbbbbbbb
> rdma_server: end 0
> --------------------------------
>
> rdma_client sends zero byte instead of 16 btyes to rdma_server.
> rdma_server receives zero byte instead of 16 btyes from rdma_client.
>
> Please also see the logic about resid in kernel, for example:
> --------------------------------
> int rxe_requester(void *arg)
> {
> ...
> payload = (mask & RXE_WRITE_OR_SEND) ? wqe->dma.resid : 0;
> ...
> skb = init_req_packet(qp, wqe, opcode, payload, &pkt);
> ...
> }
> --------------------------------
>
> Best Regards,
> Xiao Yang

Follow your steps on the latest rdma-core and linux upstream,
make tests with the followings:
"
diff --git a/librdmacm/examples/rdma_client.c b/librdmacm/examples/rdma_client.c
index c27047c5..6734757b 100644
--- a/librdmacm/examples/rdma_client.c
+++ b/librdmacm/examples/rdma_client.c
@@ -52,6 +52,7 @@ static int run(void)
        struct ibv_wc wc;
        int ret;

+       memset(send_msg, 'a', 16);
        memset(&hints, 0, sizeof hints);
        hints.ai_port_space = RDMA_PS_TCP;
        ret = rdma_getaddrinfo(server, port, &hints, &res);
diff --git a/librdmacm/examples/rdma_server.c b/librdmacm/examples/rdma_server.c
index f9c766b2..afa20996 100644
--- a/librdmacm/examples/rdma_server.c
+++ b/librdmacm/examples/rdma_server.c
@@ -132,6 +132,7 @@ static int run(void)
                goto out_disconnect;
        }

+       printf("wc.byte_len %u recv_msg %s\n", wc.byte_len, recv_msg);
        ret = rdma_post_send(id, NULL, send_msg, 16, send_mr, send_flags);
        if (ret) {
                perror("rdma_post_send");
"
The followings are results:

# ./build/bin/rdma_server -s 10.238.154.61 -p 5486&
[1] 10812
# rdma_server: start

# ./build/bin/rdma_client -s 10.238.154.61 -p 5486&
[2] 10815
# rdma_client: start
wc.byte_len 16 recv_msg aaaaaaaaaaaaaaaa    <--------------it seems
that inline is 16?
rdma_server: end 0
rdma_client: end 0
[1]-  Done                    ./build/bin/rdma_server -s 10.238.154.61 -p 5486
[2]+  Done                    ./build/bin/rdma_client -s 10.238.154.61 -p 5486

What does your commit fix?

Zhu Yanjun

> > Thanks
> > Zhu Yanjun
> >
> >> Resid should be set to the total length of transmitted bytes at the
> >> beginning.
> >>
> >> Note:
> >> Just remove the useless setting of resid in init_send_wqe().
> >>
> >> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> >> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> >> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> >> ---
> >>  providers/rxe/rxe.c | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> >> index 3c3ea8bb..3533a325 100644
> >> --- a/providers/rxe/rxe.c
> >> +++ b/providers/rxe/rxe.c
> >> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
> >>
> >>         memcpy(wqe->dma.inline_data, addr, length);
> >>         wqe->dma.length = length;
> >> -       wqe->dma.resid = 0;
> >> +       wqe->dma.resid = length;
> >>  }
> >>
> >>  static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >>         }
> >>
> >>         wqe->dma.length = tot_length;
> >> +       wqe->dma.resid = tot_length;
> >>  }
> >>
> >>  static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
> >> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
> >>         if (ibwr->send_flags & IBV_SEND_INLINE) {
> >>                 uint8_t *inline_data = wqe->dma.inline_data;
> >>
> >> -               wqe->dma.resid = 0;
> >> -
> >>                 for (i = 0; i < num_sge; i++) {
> >>                         memcpy(inline_data,
> >>                                (uint8_t *)(long)ibwr->sg_list[i].addr,
> >> --
> >> 2.25.1
> >>
> >>
> >>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-16  3:55   ` Zhu Yanjun
@ 2021-08-16 18:52     ` Bob Pearson
  2021-08-23  1:33       ` yangx.jy
  0 siblings, 1 reply; 17+ messages in thread
From: Bob Pearson @ 2021-08-16 18:52 UTC (permalink / raw)
  To: Zhu Yanjun; +Cc: Xiao Yang, RDMA mailing list, Leon Romanovsky, Jason Gunthorpe

On 8/15/21 10:55 PM, Zhu Yanjun wrote:
> On Sat, Aug 14, 2021 at 6:11 AM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>
>> On 8/9/21 10:07 AM, Xiao Yang wrote:
>>> Resid indicates the residual length of transmitted bytes but current
>>> rxe sets it to zero for inline data at the beginning.  In this case,
>>> request will transmit zero byte to responder wrongly.
>>>
>>> Resid should be set to the total length of transmitted bytes at the
>>> beginning.
>>>
>>> Note:
>>> Just remove the useless setting of resid in init_send_wqe().
>>>
>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
>>> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
>>> ---
>>>  providers/rxe/rxe.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>>> index 3c3ea8bb..3533a325 100644
>>> --- a/providers/rxe/rxe.c
>>> +++ b/providers/rxe/rxe.c
>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>>>
>>>       memcpy(wqe->dma.inline_data, addr, length);
>>>       wqe->dma.length = length;
>>> -     wqe->dma.resid = 0;
>>> +     wqe->dma.resid = length;
>>>  }
>>>
>>>  static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>       }
>>>
>>>       wqe->dma.length = tot_length;
>>> +     wqe->dma.resid = tot_length;
>>>  }
>>>
>>>  static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>>       if (ibwr->send_flags & IBV_SEND_INLINE) {
>>>               uint8_t *inline_data = wqe->dma.inline_data;
>>>
>>> -             wqe->dma.resid = 0;
>>> -
>>>               for (i = 0; i < num_sge; i++) {
>>>                       memcpy(inline_data,
>>>                              (uint8_t *)(long)ibwr->sg_list[i].addr,
>>>
>>
>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> 
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch’s delivery
> path.
> 
> Zhu Yanjun
> 

Sorry, my misunderstanding. Then I want to say

Reviewed-by: Bob Pearson <rpearsonhpe@gmail.com>

The patch looks correct to me.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-16  7:47     ` Zhu Yanjun
@ 2021-08-17  0:54       ` yangx.jy
  2021-08-23  2:44         ` Zhu Yanjun
  0 siblings, 1 reply; 17+ messages in thread
From: yangx.jy @ 2021-08-17  0:54 UTC (permalink / raw)
  To: Zhu Yanjun
  Cc: RDMA mailing list, Leon Romanovsky, Bob Pearson, Jason Gunthorpe

On 2021/8/16 15:47, Zhu Yanjun wrote:
> On Wed, Aug 11, 2021 at 12:33 AM yangx.jy@fujitsu.com
> <yangx.jy@fujitsu.com>  wrote:
>> On 2021/8/10 11:40, Zhu Yanjun wrote:
>>> On Mon, Aug 9, 2021 at 10:43 PM Xiao Yang<yangx.jy@fujitsu.com>  wrote:
>>>> Resid indicates the residual length of transmitted bytes but current
>>>> rxe sets it to zero for inline data at the beginning.  In this case,
>>>> request will transmit zero byte to responder wrongly.
>>> What are the symptoms if resid is set to zero?
>> Hi Yanjun,
>>
>> You can construct code by the attached patch and then run
>> rdma_client/server to reproduce the issue.
>>
>> If resid is set to zero, running rdma_client/server:
>> --------------------------------
>> # ./rdma_client -s 10.167.220.99 -p 8765
>> rdma_client: start
>> rdma_client: end 0
>>
>> # ./rdma_server -s 10.167.220.99 -p 8765
>> rdma_server: start
>> wc.byte_len 0 recv_msg bbbbbbbbbbbbbbbb
>> rdma_server: end 0
>> --------------------------------
>>
>> rdma_client sends zero byte instead of 16 btyes to rdma_server.
>> rdma_server receives zero byte instead of 16 btyes from rdma_client.
>>
>> Please also see the logic about resid in kernel, for example:
>> --------------------------------
>> int rxe_requester(void *arg)
>> {
>> ...
>> payload = (mask&  RXE_WRITE_OR_SEND) ? wqe->dma.resid : 0;
>> ...
>> skb = init_req_packet(qp, wqe, opcode, payload,&pkt);
>> ...
>> }
>> --------------------------------
>>
>> Best Regards,
>> Xiao Yang
> Follow your steps on the latest rdma-core and linux upstream,
> make tests with the followings:
> "
> diff --git a/librdmacm/examples/rdma_client.c b/librdmacm/examples/rdma_client.c
> index c27047c5..6734757b 100644
> --- a/librdmacm/examples/rdma_client.c
> +++ b/librdmacm/examples/rdma_client.c
> @@ -52,6 +52,7 @@ static int run(void)
>          struct ibv_wc wc;
>          int ret;
>
> +       memset(send_msg, 'a', 16);
>          memset(&hints, 0, sizeof hints);
>          hints.ai_port_space = RDMA_PS_TCP;
>          ret = rdma_getaddrinfo(server, port,&hints,&res);
> diff --git a/librdmacm/examples/rdma_server.c b/librdmacm/examples/rdma_server.c
> index f9c766b2..afa20996 100644
> --- a/librdmacm/examples/rdma_server.c
> +++ b/librdmacm/examples/rdma_server.c
> @@ -132,6 +132,7 @@ static int run(void)
>                  goto out_disconnect;
>          }
>
> +       printf("wc.byte_len %u recv_msg %s\n", wc.byte_len, recv_msg);
>          ret = rdma_post_send(id, NULL, send_msg, 16, send_mr, send_flags);
>          if (ret) {
>                  perror("rdma_post_send");
> "
> The followings are results:
>
> # ./build/bin/rdma_server -s 10.238.154.61 -p 5486&
> [1] 10812
> # rdma_server: start
>
> # ./build/bin/rdma_client -s 10.238.154.61 -p 5486&
> [2] 10815
> # rdma_client: start
> wc.byte_len 16 recv_msg aaaaaaaaaaaaaaaa<--------------it seems
> that inline is 16?
> rdma_server: end 0
> rdma_client: end 0
> [1]-  Done                    ./build/bin/rdma_server -s 10.238.154.61 -p 5486
> [2]+  Done                    ./build/bin/rdma_client -s 10.238.154.61 -p 5486
>
> What does your commit fix?
Hi Yanjun,

You missed the change that sets resid to zero on purpose:
-------------------------------------------------------------

diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
index 3c3ea8bb..ed5479fe 100644
--- a/providers/rxe/rxe.c
+++ b/providers/rxe/rxe.c
@@ -1492,7 +1492,7 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
  		wqe->iova	= ibwr->wr.rdma.remote_addr;

  	wqe->dma.length		= length;
-	wqe->dma.resid		= length;
+	wqe->dma.resid		= 0;
  	wqe->dma.num_sge	= num_sge;
  	wqe->dma.cur_sge	= 0;
  	wqe->dma.sge_offset	= 0;

-------------------------------------------------------------
Note:
It is also ok to remove "wqe->dma.resid = length" here because resid has 
been set to zero before.

With the change, running rdma_client/server will show the impact.

I didn't use new API(e.g. ibv_wr_set_inline_data, ibv_wr_send) to create 
a new test
but it is enough to show the impact of resid == 0 by doing some changes 
on rxe and
rdma_client/server.

Best Regards,
Xiao Yang
> Zhu Yanjun
>
>>> Thanks
>>> Zhu Yanjun
>>>
>>>> Resid should be set to the total length of transmitted bytes at the
>>>> beginning.
>>>>
>>>> Note:
>>>> Just remove the useless setting of resid in init_send_wqe().
>>>>
>>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
>>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
>>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
>>>> ---
>>>>   providers/rxe/rxe.c | 5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>>>> index 3c3ea8bb..3533a325 100644
>>>> --- a/providers/rxe/rxe.c
>>>> +++ b/providers/rxe/rxe.c
>>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>>>>
>>>>          memcpy(wqe->dma.inline_data, addr, length);
>>>>          wqe->dma.length = length;
>>>> -       wqe->dma.resid = 0;
>>>> +       wqe->dma.resid = length;
>>>>   }
>>>>
>>>>   static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>          }
>>>>
>>>>          wqe->dma.length = tot_length;
>>>> +       wqe->dma.resid = tot_length;
>>>>   }
>>>>
>>>>   static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
>>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>>>          if (ibwr->send_flags&  IBV_SEND_INLINE) {
>>>>                  uint8_t *inline_data = wqe->dma.inline_data;
>>>>
>>>> -               wqe->dma.resid = 0;
>>>> -
>>>>                  for (i = 0; i<  num_sge; i++) {
>>>>                          memcpy(inline_data,
>>>>                                 (uint8_t *)(long)ibwr->sg_list[i].addr,
>>>> --
>>>> 2.25.1
>>>>
>>>>
>>>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-16 18:52     ` Bob Pearson
@ 2021-08-23  1:33       ` yangx.jy
  2021-08-25  9:44         ` Leon Romanovsky
  0 siblings, 1 reply; 17+ messages in thread
From: yangx.jy @ 2021-08-23  1:33 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Bob Pearson, Zhu Yanjun, RDMA mailing list, Jason Gunthorpe

Hi Leon,

Could you review the patch?

Best Regards,
Xiao Yang
On 2021/8/17 2:52, Bob Pearson wrote:
> On 8/15/21 10:55 PM, Zhu Yanjun wrote:
>> On Sat, Aug 14, 2021 at 6:11 AM Bob Pearson<rpearsonhpe@gmail.com>  wrote:
>>> On 8/9/21 10:07 AM, Xiao Yang wrote:
>>>> Resid indicates the residual length of transmitted bytes but current
>>>> rxe sets it to zero for inline data at the beginning.  In this case,
>>>> request will transmit zero byte to responder wrongly.
>>>>
>>>> Resid should be set to the total length of transmitted bytes at the
>>>> beginning.
>>>>
>>>> Note:
>>>> Just remove the useless setting of resid in init_send_wqe().
>>>>
>>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
>>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
>>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
>>>> ---
>>>>   providers/rxe/rxe.c | 5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>>>> index 3c3ea8bb..3533a325 100644
>>>> --- a/providers/rxe/rxe.c
>>>> +++ b/providers/rxe/rxe.c
>>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>>>>
>>>>        memcpy(wqe->dma.inline_data, addr, length);
>>>>        wqe->dma.length = length;
>>>> -     wqe->dma.resid = 0;
>>>> +     wqe->dma.resid = length;
>>>>   }
>>>>
>>>>   static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>        }
>>>>
>>>>        wqe->dma.length = tot_length;
>>>> +     wqe->dma.resid = tot_length;
>>>>   }
>>>>
>>>>   static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
>>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>>>        if (ibwr->send_flags&  IBV_SEND_INLINE) {
>>>>                uint8_t *inline_data = wqe->dma.inline_data;
>>>>
>>>> -             wqe->dma.resid = 0;
>>>> -
>>>>                for (i = 0; i<  num_sge; i++) {
>>>>                        memcpy(inline_data,
>>>>                               (uint8_t *)(long)ibwr->sg_list[i].addr,
>>>>
>>> Signed-off-by: Bob Pearson<rpearsonhpe@gmail.com>
>> The Signed-off-by: tag indicates that the signer was involved in the
>> development of the patch, or that he/she was in the patch’s delivery
>> path.
>>
>> Zhu Yanjun
>>
> Sorry, my misunderstanding. Then I want to say
>
> Reviewed-by: Bob Pearson<rpearsonhpe@gmail.com>
>
> The patch looks correct to me.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-17  0:54       ` yangx.jy
@ 2021-08-23  2:44         ` Zhu Yanjun
  2021-08-23  3:58           ` yangx.jy
  0 siblings, 1 reply; 17+ messages in thread
From: Zhu Yanjun @ 2021-08-23  2:44 UTC (permalink / raw)
  To: yangx.jy; +Cc: RDMA mailing list, Leon Romanovsky, Bob Pearson, Jason Gunthorpe

On Tue, Aug 17, 2021 at 8:54 AM yangx.jy@fujitsu.com
<yangx.jy@fujitsu.com> wrote:
>
> On 2021/8/16 15:47, Zhu Yanjun wrote:
> > On Wed, Aug 11, 2021 at 12:33 AM yangx.jy@fujitsu.com
> > <yangx.jy@fujitsu.com>  wrote:
> >> On 2021/8/10 11:40, Zhu Yanjun wrote:
> >>> On Mon, Aug 9, 2021 at 10:43 PM Xiao Yang<yangx.jy@fujitsu.com>  wrote:
> >>>> Resid indicates the residual length of transmitted bytes but current
> >>>> rxe sets it to zero for inline data at the beginning.  In this case,
> >>>> request will transmit zero byte to responder wrongly.
> >>> What are the symptoms if resid is set to zero?
> >> Hi Yanjun,
> >>
> >> You can construct code by the attached patch and then run
> >> rdma_client/server to reproduce the issue.
> >>
> >> If resid is set to zero, running rdma_client/server:
> >> --------------------------------
> >> # ./rdma_client -s 10.167.220.99 -p 8765
> >> rdma_client: start
> >> rdma_client: end 0
> >>
> >> # ./rdma_server -s 10.167.220.99 -p 8765
> >> rdma_server: start
> >> wc.byte_len 0 recv_msg bbbbbbbbbbbbbbbb
> >> rdma_server: end 0
> >> --------------------------------
> >>
> >> rdma_client sends zero byte instead of 16 btyes to rdma_server.
> >> rdma_server receives zero byte instead of 16 btyes from rdma_client.
> >>
> >> Please also see the logic about resid in kernel, for example:
> >> --------------------------------
> >> int rxe_requester(void *arg)
> >> {
> >> ...
> >> payload = (mask&  RXE_WRITE_OR_SEND) ? wqe->dma.resid : 0;
> >> ...
> >> skb = init_req_packet(qp, wqe, opcode, payload,&pkt);
> >> ...
> >> }
> >> --------------------------------
> >>
> >> Best Regards,
> >> Xiao Yang
> > Follow your steps on the latest rdma-core and linux upstream,
> > make tests with the followings:
> > "
> > diff --git a/librdmacm/examples/rdma_client.c b/librdmacm/examples/rdma_client.c
> > index c27047c5..6734757b 100644
> > --- a/librdmacm/examples/rdma_client.c
> > +++ b/librdmacm/examples/rdma_client.c
> > @@ -52,6 +52,7 @@ static int run(void)
> >          struct ibv_wc wc;
> >          int ret;
> >
> > +       memset(send_msg, 'a', 16);
> >          memset(&hints, 0, sizeof hints);
> >          hints.ai_port_space = RDMA_PS_TCP;
> >          ret = rdma_getaddrinfo(server, port,&hints,&res);
> > diff --git a/librdmacm/examples/rdma_server.c b/librdmacm/examples/rdma_server.c
> > index f9c766b2..afa20996 100644
> > --- a/librdmacm/examples/rdma_server.c
> > +++ b/librdmacm/examples/rdma_server.c
> > @@ -132,6 +132,7 @@ static int run(void)
> >                  goto out_disconnect;
> >          }
> >
> > +       printf("wc.byte_len %u recv_msg %s\n", wc.byte_len, recv_msg);
> >          ret = rdma_post_send(id, NULL, send_msg, 16, send_mr, send_flags);
> >          if (ret) {
> >                  perror("rdma_post_send");
> > "
> > The followings are results:
> >
> > # ./build/bin/rdma_server -s 10.238.154.61 -p 5486&
> > [1] 10812
> > # rdma_server: start
> >
> > # ./build/bin/rdma_client -s 10.238.154.61 -p 5486&
> > [2] 10815
> > # rdma_client: start
> > wc.byte_len 16 recv_msg aaaaaaaaaaaaaaaa<--------------it seems
> > that inline is 16?
> > rdma_server: end 0
> > rdma_client: end 0
> > [1]-  Done                    ./build/bin/rdma_server -s 10.238.154.61 -p 5486
> > [2]+  Done                    ./build/bin/rdma_client -s 10.238.154.61 -p 5486
> >
> > What does your commit fix?
> Hi Yanjun,
>
> You missed the change that sets resid to zero on purpose:
> -------------------------------------------------------------
>
> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> index 3c3ea8bb..ed5479fe 100644
> --- a/providers/rxe/rxe.c
> +++ b/providers/rxe/rxe.c
> @@ -1492,7 +1492,7 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>                 wqe->iova       = ibwr->wr.rdma.remote_addr;
>
>         wqe->dma.length         = length;
> -       wqe->dma.resid          = length;
> +       wqe->dma.resid          = 0;
>         wqe->dma.num_sge        = num_sge;
>         wqe->dma.cur_sge        = 0;
>         wqe->dma.sge_offset     = 0;
>
> -------------------------------------------------------------
> Note:
> It is also ok to remove "wqe->dma.resid = length" here because resid has
> been set to zero before.
>
> With the change, running rdma_client/server will show the impact.

With the original source code, the rdma_server/rdma_client can work well.

Why "wqe->dma.resid          = length;" is replaced by "wqe->dma.resid
         = 0;", then this problem appears?

Thanks
Zhu Yanjun


>
> I didn't use new API(e.g. ibv_wr_set_inline_data, ibv_wr_send) to create
> a new test
> but it is enough to show the impact of resid == 0 by doing some changes
> on rxe and
> rdma_client/server.
>
> Best Regards,
> Xiao Yang
> > Zhu Yanjun
> >
> >>> Thanks
> >>> Zhu Yanjun
> >>>
> >>>> Resid should be set to the total length of transmitted bytes at the
> >>>> beginning.
> >>>>
> >>>> Note:
> >>>> Just remove the useless setting of resid in init_send_wqe().
> >>>>
> >>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> >>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> >>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
> >>>> ---
> >>>>   providers/rxe/rxe.c | 5 ++---
> >>>>   1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> >>>> index 3c3ea8bb..3533a325 100644
> >>>> --- a/providers/rxe/rxe.c
> >>>> +++ b/providers/rxe/rxe.c
> >>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
> >>>>
> >>>>          memcpy(wqe->dma.inline_data, addr, length);
> >>>>          wqe->dma.length = length;
> >>>> -       wqe->dma.resid = 0;
> >>>> +       wqe->dma.resid = length;
> >>>>   }
> >>>>
> >>>>   static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >>>>          }
> >>>>
> >>>>          wqe->dma.length = tot_length;
> >>>> +       wqe->dma.resid = tot_length;
> >>>>   }
> >>>>
> >>>>   static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
> >>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
> >>>>          if (ibwr->send_flags&  IBV_SEND_INLINE) {
> >>>>                  uint8_t *inline_data = wqe->dma.inline_data;
> >>>>
> >>>> -               wqe->dma.resid = 0;
> >>>> -
> >>>>                  for (i = 0; i<  num_sge; i++) {
> >>>>                          memcpy(inline_data,
> >>>>                                 (uint8_t *)(long)ibwr->sg_list[i].addr,
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>>
> >>>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-23  2:44         ` Zhu Yanjun
@ 2021-08-23  3:58           ` yangx.jy
  0 siblings, 0 replies; 17+ messages in thread
From: yangx.jy @ 2021-08-23  3:58 UTC (permalink / raw)
  To: Zhu Yanjun
  Cc: RDMA mailing list, Leon Romanovsky, Bob Pearson, Jason Gunthorpe

于 2021/8/23 10:44, Zhu Yanjun 写道:
> On Tue, Aug 17, 2021 at 8:54 AM yangx.jy@fujitsu.com
> <yangx.jy@fujitsu.com>  wrote:
>> On 2021/8/16 15:47, Zhu Yanjun wrote:
>>> On Wed, Aug 11, 2021 at 12:33 AM yangx.jy@fujitsu.com
>>> <yangx.jy@fujitsu.com>   wrote:
>>>> On 2021/8/10 11:40, Zhu Yanjun wrote:
>>>>> On Mon, Aug 9, 2021 at 10:43 PM Xiao Yang<yangx.jy@fujitsu.com>   wrote:
>>>>>> Resid indicates the residual length of transmitted bytes but current
>>>>>> rxe sets it to zero for inline data at the beginning.  In this case,
>>>>>> request will transmit zero byte to responder wrongly.
>>>>> What are the symptoms if resid is set to zero?
>>>> Hi Yanjun,
>>>>
>>>> You can construct code by the attached patch and then run
>>>> rdma_client/server to reproduce the issue.
>>>>
>>>> If resid is set to zero, running rdma_client/server:
>>>> --------------------------------
>>>> # ./rdma_client -s 10.167.220.99 -p 8765
>>>> rdma_client: start
>>>> rdma_client: end 0
>>>>
>>>> # ./rdma_server -s 10.167.220.99 -p 8765
>>>> rdma_server: start
>>>> wc.byte_len 0 recv_msg bbbbbbbbbbbbbbbb
>>>> rdma_server: end 0
>>>> --------------------------------
>>>>
>>>> rdma_client sends zero byte instead of 16 btyes to rdma_server.
>>>> rdma_server receives zero byte instead of 16 btyes from rdma_client.
>>>>
>>>> Please also see the logic about resid in kernel, for example:
>>>> --------------------------------
>>>> int rxe_requester(void *arg)
>>>> {
>>>> ...
>>>> payload = (mask&   RXE_WRITE_OR_SEND) ? wqe->dma.resid : 0;
>>>> ...
>>>> skb = init_req_packet(qp, wqe, opcode, payload,&pkt);
>>>> ...
>>>> }
>>>> --------------------------------
>>>>
>>>> Best Regards,
>>>> Xiao Yang
>>> Follow your steps on the latest rdma-core and linux upstream,
>>> make tests with the followings:
>>> "
>>> diff --git a/librdmacm/examples/rdma_client.c b/librdmacm/examples/rdma_client.c
>>> index c27047c5..6734757b 100644
>>> --- a/librdmacm/examples/rdma_client.c
>>> +++ b/librdmacm/examples/rdma_client.c
>>> @@ -52,6 +52,7 @@ static int run(void)
>>>           struct ibv_wc wc;
>>>           int ret;
>>>
>>> +       memset(send_msg, 'a', 16);
>>>           memset(&hints, 0, sizeof hints);
>>>           hints.ai_port_space = RDMA_PS_TCP;
>>>           ret = rdma_getaddrinfo(server, port,&hints,&res);
>>> diff --git a/librdmacm/examples/rdma_server.c b/librdmacm/examples/rdma_server.c
>>> index f9c766b2..afa20996 100644
>>> --- a/librdmacm/examples/rdma_server.c
>>> +++ b/librdmacm/examples/rdma_server.c
>>> @@ -132,6 +132,7 @@ static int run(void)
>>>                   goto out_disconnect;
>>>           }
>>>
>>> +       printf("wc.byte_len %u recv_msg %s\n", wc.byte_len, recv_msg);
>>>           ret = rdma_post_send(id, NULL, send_msg, 16, send_mr, send_flags);
>>>           if (ret) {
>>>                   perror("rdma_post_send");
>>> "
>>> The followings are results:
>>>
>>> # ./build/bin/rdma_server -s 10.238.154.61 -p 5486&
>>> [1] 10812
>>> # rdma_server: start
>>>
>>> # ./build/bin/rdma_client -s 10.238.154.61 -p 5486&
>>> [2] 10815
>>> # rdma_client: start
>>> wc.byte_len 16 recv_msg aaaaaaaaaaaaaaaa<--------------it seems
>>> that inline is 16?
>>> rdma_server: end 0
>>> rdma_client: end 0
>>> [1]-  Done                    ./build/bin/rdma_server -s 10.238.154.61 -p 5486
>>> [2]+  Done                    ./build/bin/rdma_client -s 10.238.154.61 -p 5486
>>>
>>> What does your commit fix?
>> Hi Yanjun,
>>
>> You missed the change that sets resid to zero on purpose:
>> -------------------------------------------------------------
>>
>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>> index 3c3ea8bb..ed5479fe 100644
>> --- a/providers/rxe/rxe.c
>> +++ b/providers/rxe/rxe.c
>> @@ -1492,7 +1492,7 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>                  wqe->iova       = ibwr->wr.rdma.remote_addr;
>>
>>          wqe->dma.length         = length;
>> -       wqe->dma.resid          = length;
>> +       wqe->dma.resid          = 0;
>>          wqe->dma.num_sge        = num_sge;
>>          wqe->dma.cur_sge        = 0;
>>          wqe->dma.sge_offset     = 0;
>>
>> -------------------------------------------------------------
>> Note:
>> It is also ok to remove "wqe->dma.resid = length" here because resid has
>> been set to zero before.
>>
>> With the change, running rdma_client/server will show the impact.
> With the original source code, the rdma_server/rdma_client can work well.
>
> Why "wqe->dma.resid          = length;" is replaced by "wqe->dma.resid
>           = 0;", then this problem appears?
Hi Yanjun,

The original source code has the following logic:
-----------------------------
static int init_send_wqe(...)
{
     ...
     if (ibwr->send_flags & IBV_SEND_INLINE) {
         ...
          wqe->dma.resid = 0;
         ...
     }
     ...
     wqe->dma.resid = length;
     ...
}
-----------------------------
For inline data flag, resid is set to zero first and then replaced with 
length at subsequent steps. In this case, the issue doesn't appear.
My patch removes the useless "wqe->dma.resid = 0" and sets the correct 
"wqe->dma.resid = length" for new inline_data APIs.

Best Regards,
Xiao Yang
> Thanks
> Zhu Yanjun
>
>
>> I didn't use new API(e.g. ibv_wr_set_inline_data, ibv_wr_send) to create
>> a new test
>> but it is enough to show the impact of resid == 0 by doing some changes
>> on rxe and
>> rdma_client/server.
>>
>> Best Regards,
>> Xiao Yang
>>> Zhu Yanjun
>>>
>>>>> Thanks
>>>>> Zhu Yanjun
>>>>>
>>>>>> Resid should be set to the total length of transmitted bytes at the
>>>>>> beginning.
>>>>>>
>>>>>> Note:
>>>>>> Just remove the useless setting of resid in init_send_wqe().
>>>>>>
>>>>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
>>>>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
>>>>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
>>>>>> ---
>>>>>>    providers/rxe/rxe.c | 5 ++---
>>>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>>>>>> index 3c3ea8bb..3533a325 100644
>>>>>> --- a/providers/rxe/rxe.c
>>>>>> +++ b/providers/rxe/rxe.c
>>>>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>>>>>>
>>>>>>           memcpy(wqe->dma.inline_data, addr, length);
>>>>>>           wqe->dma.length = length;
>>>>>> -       wqe->dma.resid = 0;
>>>>>> +       wqe->dma.resid = length;
>>>>>>    }
>>>>>>
>>>>>>    static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>>>           }
>>>>>>
>>>>>>           wqe->dma.length = tot_length;
>>>>>> +       wqe->dma.resid = tot_length;
>>>>>>    }
>>>>>>
>>>>>>    static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
>>>>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>>>>>           if (ibwr->send_flags&   IBV_SEND_INLINE) {
>>>>>>                   uint8_t *inline_data = wqe->dma.inline_data;
>>>>>>
>>>>>> -               wqe->dma.resid = 0;
>>>>>> -
>>>>>>                   for (i = 0; i<   num_sge; i++) {
>>>>>>                           memcpy(inline_data,
>>>>>>                                  (uint8_t *)(long)ibwr->sg_list[i].addr,
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>>>
>>>>>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-23  1:33       ` yangx.jy
@ 2021-08-25  9:44         ` Leon Romanovsky
  2021-09-02  8:27           ` yangx.jy
  0 siblings, 1 reply; 17+ messages in thread
From: Leon Romanovsky @ 2021-08-25  9:44 UTC (permalink / raw)
  To: yangx.jy; +Cc: Bob Pearson, Zhu Yanjun, RDMA mailing list, Jason Gunthorpe

On Mon, Aug 23, 2021 at 01:33:24AM +0000, yangx.jy@fujitsu.com wrote:
> Hi Leon,
> 
> Could you review the patch?

There is no need, I trust to Zhu's and Bob's review.

Thanks

> 
> Best Regards,
> Xiao Yang
> On 2021/8/17 2:52, Bob Pearson wrote:
> > On 8/15/21 10:55 PM, Zhu Yanjun wrote:
> >> On Sat, Aug 14, 2021 at 6:11 AM Bob Pearson<rpearsonhpe@gmail.com>  wrote:
> >>> On 8/9/21 10:07 AM, Xiao Yang wrote:
> >>>> Resid indicates the residual length of transmitted bytes but current
> >>>> rxe sets it to zero for inline data at the beginning.  In this case,
> >>>> request will transmit zero byte to responder wrongly.
> >>>>
> >>>> Resid should be set to the total length of transmitted bytes at the
> >>>> beginning.
> >>>>
> >>>> Note:
> >>>> Just remove the useless setting of resid in init_send_wqe().
> >>>>
> >>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> >>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> >>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
> >>>> ---
> >>>>   providers/rxe/rxe.c | 5 ++---
> >>>>   1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> >>>> index 3c3ea8bb..3533a325 100644
> >>>> --- a/providers/rxe/rxe.c
> >>>> +++ b/providers/rxe/rxe.c
> >>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
> >>>>
> >>>>        memcpy(wqe->dma.inline_data, addr, length);
> >>>>        wqe->dma.length = length;
> >>>> -     wqe->dma.resid = 0;
> >>>> +     wqe->dma.resid = length;
> >>>>   }
> >>>>
> >>>>   static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >>>>        }
> >>>>
> >>>>        wqe->dma.length = tot_length;
> >>>> +     wqe->dma.resid = tot_length;
> >>>>   }
> >>>>
> >>>>   static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
> >>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
> >>>>        if (ibwr->send_flags&  IBV_SEND_INLINE) {
> >>>>                uint8_t *inline_data = wqe->dma.inline_data;
> >>>>
> >>>> -             wqe->dma.resid = 0;
> >>>> -
> >>>>                for (i = 0; i<  num_sge; i++) {
> >>>>                        memcpy(inline_data,
> >>>>                               (uint8_t *)(long)ibwr->sg_list[i].addr,
> >>>>
> >>> Signed-off-by: Bob Pearson<rpearsonhpe@gmail.com>
> >> The Signed-off-by: tag indicates that the signer was involved in the
> >> development of the patch, or that he/she was in the patch’s delivery
> >> path.
> >>
> >> Zhu Yanjun
> >>
> > Sorry, my misunderstanding. Then I want to say
> >
> > Reviewed-by: Bob Pearson<rpearsonhpe@gmail.com>
> >
> > The patch looks correct to me.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-25  9:44         ` Leon Romanovsky
@ 2021-09-02  8:27           ` yangx.jy
  2021-09-02 19:48             ` Bob Pearson
  2021-09-03  3:34             ` Zhu Yanjun
  0 siblings, 2 replies; 17+ messages in thread
From: yangx.jy @ 2021-09-02  8:27 UTC (permalink / raw)
  To: Bob Pearson, Zhu Yanjun
  Cc: Leon Romanovsky, RDMA mailing list, Jason Gunthorpe

Hi Yanjun, Bob

Ping. :-)

Best Regards,
Xiao Yang
On 2021/8/25 17:44, Leon Romanovsky wrote:
> On Mon, Aug 23, 2021 at 01:33:24AM +0000, yangx.jy@fujitsu.com wrote:
>> Hi Leon,
>>
>> Could you review the patch?
> There is no need, I trust to Zhu's and Bob's review.
>
> Thanks
>
>> Best Regards,
>> Xiao Yang
>> On 2021/8/17 2:52, Bob Pearson wrote:
>>> On 8/15/21 10:55 PM, Zhu Yanjun wrote:
>>>> On Sat, Aug 14, 2021 at 6:11 AM Bob Pearson<rpearsonhpe@gmail.com>   wrote:
>>>>> On 8/9/21 10:07 AM, Xiao Yang wrote:
>>>>>> Resid indicates the residual length of transmitted bytes but current
>>>>>> rxe sets it to zero for inline data at the beginning.  In this case,
>>>>>> request will transmit zero byte to responder wrongly.
>>>>>>
>>>>>> Resid should be set to the total length of transmitted bytes at the
>>>>>> beginning.
>>>>>>
>>>>>> Note:
>>>>>> Just remove the useless setting of resid in init_send_wqe().
>>>>>>
>>>>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
>>>>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
>>>>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
>>>>>> ---
>>>>>>    providers/rxe/rxe.c | 5 ++---
>>>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>>>>>> index 3c3ea8bb..3533a325 100644
>>>>>> --- a/providers/rxe/rxe.c
>>>>>> +++ b/providers/rxe/rxe.c
>>>>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>>>>>>
>>>>>>         memcpy(wqe->dma.inline_data, addr, length);
>>>>>>         wqe->dma.length = length;
>>>>>> -     wqe->dma.resid = 0;
>>>>>> +     wqe->dma.resid = length;
>>>>>>    }
>>>>>>
>>>>>>    static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>>>         }
>>>>>>
>>>>>>         wqe->dma.length = tot_length;
>>>>>> +     wqe->dma.resid = tot_length;
>>>>>>    }
>>>>>>
>>>>>>    static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
>>>>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>>>>>         if (ibwr->send_flags&   IBV_SEND_INLINE) {
>>>>>>                 uint8_t *inline_data = wqe->dma.inline_data;
>>>>>>
>>>>>> -             wqe->dma.resid = 0;
>>>>>> -
>>>>>>                 for (i = 0; i<   num_sge; i++) {
>>>>>>                         memcpy(inline_data,
>>>>>>                                (uint8_t *)(long)ibwr->sg_list[i].addr,
>>>>>>
>>>>> Signed-off-by: Bob Pearson<rpearsonhpe@gmail.com>
>>>> The Signed-off-by: tag indicates that the signer was involved in the
>>>> development of the patch, or that he/she was in the patch’s delivery
>>>> path.
>>>>
>>>> Zhu Yanjun
>>>>
>>> Sorry, my misunderstanding. Then I want to say
>>>
>>> Reviewed-by: Bob Pearson<rpearsonhpe@gmail.com>
>>>
>>> The patch looks correct to me.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-09-02  8:27           ` yangx.jy
@ 2021-09-02 19:48             ` Bob Pearson
  2021-09-03  1:39               ` yangx.jy
  2021-09-03  3:34             ` Zhu Yanjun
  1 sibling, 1 reply; 17+ messages in thread
From: Bob Pearson @ 2021-09-02 19:48 UTC (permalink / raw)
  To: yangx.jy, Zhu Yanjun, Jason Gunthorpe, linux-rdma; +Cc: Leon Romanovsky

On 9/2/21 3:27 AM, yangx.jy@fujitsu.com wrote:
> Hi Yanjun, Bob
> 
> Ping. :-)
> 
> Best Regards,
> Xiao Yang
> On 2021/8/25 17:44, Leon Romanovsky wrote:
>> On Mon, Aug 23, 2021 at 01:33:24AM +0000, yangx.jy@fujitsu.com wrote:
>>> Hi Leon,
>>>
>>> Could you review the patch?
>> There is no need, I trust to Zhu's and Bob's review.
>>
>> Thanks
>>
>>> Best Regards,
>>> Xiao Yang
>>> On 2021/8/17 2:52, Bob Pearson wrote:
>>>> On 8/15/21 10:55 PM, Zhu Yanjun wrote:
>>>>> On Sat, Aug 14, 2021 at 6:11 AM Bob Pearson<rpearsonhpe@gmail.com>   wrote:
>>>>>> On 8/9/21 10:07 AM, Xiao Yang wrote:
>>>>>>> Resid indicates the residual length of transmitted bytes but current
>>>>>>> rxe sets it to zero for inline data at the beginning.  In this case,
>>>>>>> request will transmit zero byte to responder wrongly.
>>>>>>>
>>>>>>> Resid should be set to the total length of transmitted bytes at the
>>>>>>> beginning.
>>>>>>>
>>>>>>> Note:
>>>>>>> Just remove the useless setting of resid in init_send_wqe().
>>>>>>>
>>>>>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
>>>>>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
>>>>>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
>>>>>>> ---
>>>>>>>    providers/rxe/rxe.c | 5 ++---
>>>>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
>>>>>>> index 3c3ea8bb..3533a325 100644
>>>>>>> --- a/providers/rxe/rxe.c
>>>>>>> +++ b/providers/rxe/rxe.c
>>>>>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
>>>>>>>
>>>>>>>         memcpy(wqe->dma.inline_data, addr, length);
>>>>>>>         wqe->dma.length = length;
>>>>>>> -     wqe->dma.resid = 0;
>>>>>>> +     wqe->dma.resid = length;
>>>>>>>    }
>>>>>>>
>>>>>>>    static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
>>>>>>>         }
>>>>>>>
>>>>>>>         wqe->dma.length = tot_length;
>>>>>>> +     wqe->dma.resid = tot_length;
>>>>>>>    }
>>>>>>>
>>>>>>>    static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
>>>>>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
>>>>>>>         if (ibwr->send_flags&   IBV_SEND_INLINE) {
>>>>>>>                 uint8_t *inline_data = wqe->dma.inline_data;
>>>>>>>
>>>>>>> -             wqe->dma.resid = 0;
>>>>>>> -
>>>>>>>                 for (i = 0; i<   num_sge; i++) {
>>>>>>>                         memcpy(inline_data,
>>>>>>>                                (uint8_t *)(long)ibwr->sg_list[i].addr,
>>>>>>>
>>>>>> Signed-off-by: Bob Pearson<rpearsonhpe@gmail.com>
>>>>> The Signed-off-by: tag indicates that the signer was involved in the
>>>>> development of the patch, or that he/she was in the patch’s delivery
>>>>> path.
>>>>>
>>>>> Zhu Yanjun
>>>>>
>>>> Sorry, my misunderstanding. Then I want to say
>>>>
>>>> Reviewed-by: Bob Pearson<rpearsonhpe@gmail.com>
>>>>
>>>> The patch looks correct to me.

Hi,

What are you looking for? I reviewed the patch (above) and agree with you that it makes sense.
But it's not up to me to accept it. That would be Jason or Zhu.

BTW until this is fixed I it looks like inline WRs are broken for the extended QP API.

Bob

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-09-02 19:48             ` Bob Pearson
@ 2021-09-03  1:39               ` yangx.jy
  0 siblings, 0 replies; 17+ messages in thread
From: yangx.jy @ 2021-09-03  1:39 UTC (permalink / raw)
  To: Bob Pearson; +Cc: Zhu Yanjun, Jason Gunthorpe, linux-rdma, Leon Romanovsky

On 2021/9/3 3:48, Bob Pearson wrote:
> Hi,
>
> What are you looking for? I reviewed the patch (above) and agree with you that it makes sense.
> But it's not up to me to accept it. That would be Jason or Zhu.
Hi Bob,

Sorry for missing your review.  Thanks for your remind.

> BTW until this is fixed I it looks like inline WRs are broken for the extended QP API.

I wonder if there is some commands or tests can verify new inline WRs in 
rdma-core?

Best Regards,
Xiao Yang
> Bob

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-09-02  8:27           ` yangx.jy
  2021-09-02 19:48             ` Bob Pearson
@ 2021-09-03  3:34             ` Zhu Yanjun
  1 sibling, 0 replies; 17+ messages in thread
From: Zhu Yanjun @ 2021-09-03  3:34 UTC (permalink / raw)
  To: yangx.jy; +Cc: Bob Pearson, Leon Romanovsky, RDMA mailing list, Jason Gunthorpe

On Thu, Sep 2, 2021 at 4:27 PM yangx.jy@fujitsu.com
<yangx.jy@fujitsu.com> wrote:
>
> Hi Yanjun, Bob
>
> Ping. :

Sorry. It is late to reply.
My concern is if this will introduce other risks or not.
If you can confirm that this will not introduce other risks,
I am fine with this commit.

Reviewed-by: Zhu Yanjun <zyjzyj2000@gmail.com>

>
> Best Regards,
> Xiao Yang
> On 2021/8/25 17:44, Leon Romanovsky wrote:
> > On Mon, Aug 23, 2021 at 01:33:24AM +0000, yangx.jy@fujitsu.com wrote:
> >> Hi Leon,
> >>
> >> Could you review the patch?
> > There is no need, I trust to Zhu's and Bob's review.
> >
> > Thanks
> >
> >> Best Regards,
> >> Xiao Yang
> >> On 2021/8/17 2:52, Bob Pearson wrote:
> >>> On 8/15/21 10:55 PM, Zhu Yanjun wrote:
> >>>> On Sat, Aug 14, 2021 at 6:11 AM Bob Pearson<rpearsonhpe@gmail.com>   wrote:
> >>>>> On 8/9/21 10:07 AM, Xiao Yang wrote:
> >>>>>> Resid indicates the residual length of transmitted bytes but current
> >>>>>> rxe sets it to zero for inline data at the beginning.  In this case,
> >>>>>> request will transmit zero byte to responder wrongly.
> >>>>>>
> >>>>>> Resid should be set to the total length of transmitted bytes at the
> >>>>>> beginning.
> >>>>>>
> >>>>>> Note:
> >>>>>> Just remove the useless setting of resid in init_send_wqe().
> >>>>>>
> >>>>>> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> >>>>>> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> >>>>>> Signed-off-by: Xiao Yang<yangx.jy@fujitsu.com>
> >>>>>> ---
> >>>>>>    providers/rxe/rxe.c | 5 ++---
> >>>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> >>>>>> index 3c3ea8bb..3533a325 100644
> >>>>>> --- a/providers/rxe/rxe.c
> >>>>>> +++ b/providers/rxe/rxe.c
> >>>>>> @@ -1004,7 +1004,7 @@ static void wr_set_inline_data(struct ibv_qp_ex *ibqp, void *addr,
> >>>>>>
> >>>>>>         memcpy(wqe->dma.inline_data, addr, length);
> >>>>>>         wqe->dma.length = length;
> >>>>>> -     wqe->dma.resid = 0;
> >>>>>> +     wqe->dma.resid = length;
> >>>>>>    }
> >>>>>>
> >>>>>>    static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >>>>>> @@ -1035,6 +1035,7 @@ static void wr_set_inline_data_list(struct ibv_qp_ex *ibqp, size_t num_buf,
> >>>>>>         }
> >>>>>>
> >>>>>>         wqe->dma.length = tot_length;
> >>>>>> +     wqe->dma.resid = tot_length;
> >>>>>>    }
> >>>>>>
> >>>>>>    static void wr_set_sge(struct ibv_qp_ex *ibqp, uint32_t lkey, uint64_t addr,
> >>>>>> @@ -1473,8 +1474,6 @@ static int init_send_wqe(struct rxe_qp *qp, struct rxe_wq *sq,
> >>>>>>         if (ibwr->send_flags&   IBV_SEND_INLINE) {
> >>>>>>                 uint8_t *inline_data = wqe->dma.inline_data;
> >>>>>>
> >>>>>> -             wqe->dma.resid = 0;
> >>>>>> -
> >>>>>>                 for (i = 0; i<   num_sge; i++) {
> >>>>>>                         memcpy(inline_data,
> >>>>>>                                (uint8_t *)(long)ibwr->sg_list[i].addr,
> >>>>>>
> >>>>> Signed-off-by: Bob Pearson<rpearsonhpe@gmail.com>
> >>>> The Signed-off-by: tag indicates that the signer was involved in the
> >>>> development of the patch, or that he/she was in the patch’s delivery
> >>>> path.
> >>>>
> >>>> Zhu Yanjun
> >>>>
> >>> Sorry, my misunderstanding. Then I want to say
> >>>
> >>> Reviewed-by: Bob Pearson<rpearsonhpe@gmail.com>
> >>>
> >>> The patch looks correct to me.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data
  2021-08-09 15:07 [PATCH v2] providers/rxe: Set the correct value of resid for inline data Xiao Yang
  2021-08-10  3:40 ` Zhu Yanjun
  2021-08-13 22:11 ` Bob Pearson
@ 2021-09-26 10:51 ` Leon Romanovsky
  2 siblings, 0 replies; 17+ messages in thread
From: Leon Romanovsky @ 2021-09-26 10:51 UTC (permalink / raw)
  To: Xiao Yang; +Cc: linux-rdma, rpearsonhpe, zyjzyj2000, jgg

On Mon, Aug 09, 2021 at 11:07:38PM +0800, Xiao Yang wrote:
> Resid indicates the residual length of transmitted bytes but current
> rxe sets it to zero for inline data at the beginning.  In this case,
> request will transmit zero byte to responder wrongly.
> 
> Resid should be set to the total length of transmitted bytes at the
> beginning.
> 
> Note:
> Just remove the useless setting of resid in init_send_wqe().
> 
> Fixes: 1a894ca10105 ("Providers/rxe: Implement ibv_create_qp_ex verb")
> Fixes: 8337db5df125 ("Providers/rxe: Implement memory windows")
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
>  providers/rxe/rxe.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Thanks, applied.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-09-26 10:51 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 15:07 [PATCH v2] providers/rxe: Set the correct value of resid for inline data Xiao Yang
2021-08-10  3:40 ` Zhu Yanjun
2021-08-10 16:33   ` yangx.jy
2021-08-16  7:47     ` Zhu Yanjun
2021-08-17  0:54       ` yangx.jy
2021-08-23  2:44         ` Zhu Yanjun
2021-08-23  3:58           ` yangx.jy
2021-08-13 22:11 ` Bob Pearson
2021-08-16  3:55   ` Zhu Yanjun
2021-08-16 18:52     ` Bob Pearson
2021-08-23  1:33       ` yangx.jy
2021-08-25  9:44         ` Leon Romanovsky
2021-09-02  8:27           ` yangx.jy
2021-09-02 19:48             ` Bob Pearson
2021-09-03  1:39               ` yangx.jy
2021-09-03  3:34             ` Zhu Yanjun
2021-09-26 10:51 ` Leon Romanovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).