[PATCH v2] providers/rxe: Set the correct value of resid for inline data

[PATCH v2] providers/rxe: Set the correct value of resid for inline data

[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

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Zhu Yanjun]

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
>
>
>

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[yangx.jy]

[-- 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

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Bob Pearson]

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>

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Zhu Yanjun]

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

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Zhu Yanjun]

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
> >>
> >>
> >>
>

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Bob Pearson]

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.

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[yangx.jy]

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
>>>>
>>>>
>>>>

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[yangx.jy]

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.

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[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?

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
> >>>>
> >>>>
> >>>>

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[yangx.jy]

于 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
>>>>>>
>>>>>>
>>>>>>

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Leon Romanovsky]

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.

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[yangx.jy]

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.

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Bob Pearson]

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

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[yangx.jy]

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

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Zhu Yanjun]

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.

Re: [PATCH v2] providers/rxe: Set the correct value of resid for inline data

[Leon Romanovsky]

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.