[Qemu-devel] [PATCH 0/3] rdma fixes and clean ups

[Qemu-devel] [PATCH 0/3] rdma fixes and clean ups

[Isaku Yamahata]

Small fixes/clean up to rdma. found through code review.

Isaku Yamahata (3):
  rdma: don't use negative index to array
  rdma: qemu_rdma_post_send_control uses wrongly RDMA_WRID_MAX
  rdma: use RDMA_WRID_READY

 migration-rdma.c |   43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/3] rdma: don't use negative index to array

[Isaku Yamahata]

Cc: Michael R. Hines <mrhines@us.ibm.com>
Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
---
 migration-rdma.c |   27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/migration-rdma.c b/migration-rdma.c
index 4828738..edbae9f 100644
--- a/migration-rdma.c
+++ b/migration-rdma.c
@@ -1931,10 +1931,21 @@ static int qemu_rdma_write_flush(QEMUFile *f, RDMAContext *rdma)
 static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
                     uint64_t offset, uint64_t len)
 {
-    RDMALocalBlock *block =
-        &(rdma->local_ram_blocks.block[rdma->current_index]);
-    uint8_t *host_addr = block->local_host_addr + (offset - block->offset);
-    uint8_t *chunk_end = ram_chunk_end(block, rdma->current_chunk);
+    RDMALocalBlock *block;
+    uint8_t *host_addr;
+    uint8_t *chunk_end;
+
+    if (rdma->current_index < 0) {
+        return 0;
+    }
+
+    if (rdma->current_chunk < 0) {
+        return 0;
+    }
+
+    block = &(rdma->local_ram_blocks.block[rdma->current_index]);
+    host_addr = block->local_host_addr + (offset - block->offset);
+    chunk_end = ram_chunk_end(block, rdma->current_chunk);
 
     if (rdma->current_length == 0) {
         return 0;
@@ -1947,10 +1958,6 @@ static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
         return 0;
     }
 
-    if (rdma->current_index < 0) {
-        return 0;
-    }
-
     if (offset < block->offset) {
         return 0;
     }
@@ -1959,10 +1966,6 @@ static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
         return 0;
     }
 
-    if (rdma->current_chunk < 0) {
-        return 0;
-    }
-
     if ((host_addr + len) > chunk_end) {
         return 0;
     }
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/3] rdma: qemu_rdma_post_send_control uses wrongly RDMA_WRID_MAX

[Isaku Yamahata]

RDMA_WRID_CONTROL should be used. And remove related work around.

Cc: Michael R. Hines <mrhines@us.ibm.com>
Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
---
 migration-rdma.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration-rdma.c b/migration-rdma.c
index edbae9f..67069d2 100644
--- a/migration-rdma.c
+++ b/migration-rdma.c
@@ -322,7 +322,7 @@ typedef struct RDMAContext {
     char *host;
     int port;
 
-    RDMAWorkRequestData wr_data[RDMA_WRID_MAX + 1];
+    RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
 
     /*
      * This is used by *_exchange_send() to figure out whether or not
@@ -1397,7 +1397,7 @@ static int qemu_rdma_post_send_control(RDMAContext *rdma, uint8_t *buf,
                                        RDMAControlHeader *head)
 {
     int ret = 0;
-    RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_MAX];
+    RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_CONTROL];
     struct ibv_send_wr *bad_wr;
     struct ibv_sge sge = {
                            .addr = (uint64_t)(wr->control),
@@ -2052,7 +2052,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
     g_free(rdma->block);
     rdma->block = NULL;
 
-    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
+    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
         if (rdma->wr_data[idx].control_mr) {
             rdma->total_registrations--;
             ibv_dereg_mr(rdma->wr_data[idx].control_mr);
@@ -2134,7 +2134,7 @@ static int qemu_rdma_source_init(RDMAContext *rdma, Error **errp, bool pin_all)
         goto err_rdma_source_init;
     }
 
-    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
+    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
         ret = qemu_rdma_reg_control(rdma, idx);
         if (ret) {
             ERROR(temp, "rdma migration: error registering %d control!\n",
@@ -2243,7 +2243,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
     struct rdma_cm_id *listen_id;
     char ip[40] = "unknown";
 
-    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
+    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
         rdma->wr_data[idx].control_len = 0;
         rdma->wr_data[idx].control_curr = NULL;
     }
@@ -2696,7 +2696,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
         goto err_rdma_dest_wait;
     }
 
-    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
+    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
         ret = qemu_rdma_reg_control(rdma, idx);
         if (ret) {
             fprintf(stderr, "rdma: error registering %d control!\n", idx);
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/3] rdma: use RDMA_WRID_READY

[Isaku Yamahata]

Cc: Michael R. Hines <mrhines@us.ibm.com>
Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
---
 migration-rdma.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration-rdma.c b/migration-rdma.c
index 67069d2..871f9cd 100644
--- a/migration-rdma.c
+++ b/migration-rdma.c
@@ -2221,7 +2221,7 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp)
 
     rdma_ack_cm_event(cm_event);
 
-    ret = qemu_rdma_post_recv_control(rdma, 0);
+    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
     if (ret) {
         ERROR(errp, "posting second control recv!\n");
         goto err_rdma_source_connect;
@@ -2726,7 +2726,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
 
     rdma_ack_cm_event(cm_event);
 
-    ret = qemu_rdma_post_recv_control(rdma, 0);
+    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
     if (ret) {
         fprintf(stderr, "rdma migration: error posting second control recv!\n");
         goto err_rdma_dest_wait;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/3] rdma: don't use negative index to array

[Michael R. Hines]

On 08/01/2013 11:56 PM, Isaku Yamahata wrote:
> Cc: Michael R. Hines <mrhines@us.ibm.com>
> Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
> ---
>   migration-rdma.c |   27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/migration-rdma.c b/migration-rdma.c
> index 4828738..edbae9f 100644
> --- a/migration-rdma.c
> +++ b/migration-rdma.c
> @@ -1931,10 +1931,21 @@ static int qemu_rdma_write_flush(QEMUFile *f, RDMAContext *rdma)
>   static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
>                       uint64_t offset, uint64_t len)
>   {
> -    RDMALocalBlock *block =
> -        &(rdma->local_ram_blocks.block[rdma->current_index]);
> -    uint8_t *host_addr = block->local_host_addr + (offset - block->offset);
> -    uint8_t *chunk_end = ram_chunk_end(block, rdma->current_chunk);
> +    RDMALocalBlock *block;
> +    uint8_t *host_addr;
> +    uint8_t *chunk_end;
> +
> +    if (rdma->current_index < 0) {
> +        return 0;
> +    }
> +
> +    if (rdma->current_chunk < 0) {
> +        return 0;
> +    }
> +
> +    block = &(rdma->local_ram_blocks.block[rdma->current_index]);
> +    host_addr = block->local_host_addr + (offset - block->offset);
> +    chunk_end = ram_chunk_end(block, rdma->current_chunk);
>
>       if (rdma->current_length == 0) {
>           return 0;
> @@ -1947,10 +1958,6 @@ static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
>           return 0;
>       }
>
> -    if (rdma->current_index < 0) {
> -        return 0;
> -    }
> -
>       if (offset < block->offset) {
>           return 0;
>       }
> @@ -1959,10 +1966,6 @@ static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
>           return 0;
>       }
>
> -    if (rdma->current_chunk < 0) {
> -        return 0;
> -    }
> -
>       if ((host_addr + len) > chunk_end) {
>           return 0;
>       }

Reviewed-by: Michael R. Hines <mrhines@us.ibm.com>

Re: [Qemu-devel] [PATCH 3/3] rdma: use RDMA_WRID_READY

[Michael R. Hines]

On 08/01/2013 11:56 PM, Isaku Yamahata wrote:
> Cc: Michael R. Hines <mrhines@us.ibm.com>
> Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
> ---
>   migration-rdma.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/migration-rdma.c b/migration-rdma.c
> index 67069d2..871f9cd 100644
> --- a/migration-rdma.c
> +++ b/migration-rdma.c
> @@ -2221,7 +2221,7 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp)
>
>       rdma_ack_cm_event(cm_event);
>
> -    ret = qemu_rdma_post_recv_control(rdma, 0);
> +    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
>       if (ret) {
>           ERROR(errp, "posting second control recv!\n");
>           goto err_rdma_source_connect;
> @@ -2726,7 +2726,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>
>       rdma_ack_cm_event(cm_event);
>
> -    ret = qemu_rdma_post_recv_control(rdma, 0);
> +    ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
>       if (ret) {
>           fprintf(stderr, "rdma migration: error posting second control recv!\n");
>           goto err_rdma_dest_wait;

Good catch too.

Reviewed-by: Michael R. Hines <mrhines@us.ibm.com>

Re: [Qemu-devel] [PATCH 2/3] rdma: qemu_rdma_post_send_control uses wrongly RDMA_WRID_MAX

[Michael R. Hines]

On 08/01/2013 11:56 PM, Isaku Yamahata wrote:
> RDMA_WRID_CONTROL should be used. And remove related work around.
>
> Cc: Michael R. Hines <mrhines@us.ibm.com>
> Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
> ---
>   migration-rdma.c |   12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/migration-rdma.c b/migration-rdma.c
> index edbae9f..67069d2 100644
> --- a/migration-rdma.c
> +++ b/migration-rdma.c
> @@ -322,7 +322,7 @@ typedef struct RDMAContext {
>       char *host;
>       int port;
>
> -    RDMAWorkRequestData wr_data[RDMA_WRID_MAX + 1];
> +    RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
>
>       /*
>        * This is used by *_exchange_send() to figure out whether or not
> @@ -1397,7 +1397,7 @@ static int qemu_rdma_post_send_control(RDMAContext *rdma, uint8_t *buf,
>                                          RDMAControlHeader *head)
>   {
>       int ret = 0;
> -    RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_MAX];
> +    RDMAWorkRequestData *wr = &rdma->wr_data[RDMA_WRID_CONTROL];
>       struct ibv_send_wr *bad_wr;
>       struct ibv_sge sge = {
>                              .addr = (uint64_t)(wr->control),
> @@ -2052,7 +2052,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>       g_free(rdma->block);
>       rdma->block = NULL;
>
> -    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
> +    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
>           if (rdma->wr_data[idx].control_mr) {
>               rdma->total_registrations--;
>               ibv_dereg_mr(rdma->wr_data[idx].control_mr);
> @@ -2134,7 +2134,7 @@ static int qemu_rdma_source_init(RDMAContext *rdma, Error **errp, bool pin_all)
>           goto err_rdma_source_init;
>       }
>
> -    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
> +    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
>           ret = qemu_rdma_reg_control(rdma, idx);
>           if (ret) {
>               ERROR(temp, "rdma migration: error registering %d control!\n",
> @@ -2243,7 +2243,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>       struct rdma_cm_id *listen_id;
>       char ip[40] = "unknown";
>
> -    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
> +    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
>           rdma->wr_data[idx].control_len = 0;
>           rdma->wr_data[idx].control_curr = NULL;
>       }
> @@ -2696,7 +2696,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>           goto err_rdma_dest_wait;
>       }
>
> -    for (idx = 0; idx <= RDMA_WRID_MAX; idx++) {
> +    for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
>           ret = qemu_rdma_reg_control(rdma, idx);
>           if (ret) {
>               fprintf(stderr, "rdma: error registering %d control!\n", idx);

Good eyes ...... I totally missed that =)

Reviewed-by: Michael R. Hines <mrhines@us.ibm.com>

Re: [Qemu-devel] [PATCH 0/3] rdma fixes and clean ups

[Michael R. Hines]

On 08/01/2013 11:56 PM, Isaku Yamahata wrote:
> Small fixes/clean up to rdma. found through code review.
>
> Isaku Yamahata (3):
>    rdma: don't use negative index to array
>    rdma: qemu_rdma_post_send_control uses wrongly RDMA_WRID_MAX
>    rdma: use RDMA_WRID_READY
>
>   migration-rdma.c |   43 +++++++++++++++++++++++--------------------
>   1 file changed, 23 insertions(+), 20 deletions(-)
>

Would you like me to apply this series to my tree and re-send our 
bugfixes together
to the maintainer for 1.6?

- Michael

Re: [Qemu-devel] [PATCH 0/3] rdma fixes and clean ups

[Isaku Yamahata]

On Fri, Aug 02, 2013 at 09:40:58AM -0400,
"Michael R. Hines" <mrhines@linux.vnet.ibm.com> wrote:

> On 08/01/2013 11:56 PM, Isaku Yamahata wrote:
> >Small fixes/clean up to rdma. found through code review.
> >
> >Isaku Yamahata (3):
> >   rdma: don't use negative index to array
> >   rdma: qemu_rdma_post_send_control uses wrongly RDMA_WRID_MAX
> >   rdma: use RDMA_WRID_READY
> >
> >  migration-rdma.c |   43 +++++++++++++++++++++++--------------------
> >  1 file changed, 23 insertions(+), 20 deletions(-)
> >
> 
> Would you like me to apply this series to my tree and re-send our
> bugfixes together to the maintainer for 1.6?

Yes. With another patch I sent just a moment ago.

Re: [Qemu-devel] [PATCH 0/3] rdma fixes and clean ups

[Michael R. Hines]

On 08/03/2013 09:24 PM, Isaku Yamahata wrote:
> On Fri, Aug 02, 2013 at 09:40:58AM -0400,
> "Michael R. Hines" <mrhines@linux.vnet.ibm.com> wrote:
>
>> On 08/01/2013 11:56 PM, Isaku Yamahata wrote:
>>> Small fixes/clean up to rdma. found through code review.
>>>
>>> Isaku Yamahata (3):
>>>    rdma: don't use negative index to array
>>>    rdma: qemu_rdma_post_send_control uses wrongly RDMA_WRID_MAX
>>>    rdma: use RDMA_WRID_READY
>>>
>>>   migration-rdma.c |   43 +++++++++++++++++++++++--------------------
>>>   1 file changed, 23 insertions(+), 20 deletions(-)
>>>
>> Would you like me to apply this series to my tree and re-send our
>> bugfixes together to the maintainer for 1.6?
> Yes. With another patch I sent just a moment ago.
>

Thank you - just sent a unified "For-1.6" patch for both of us.

- Michael

Re: [Qemu-devel] [PATCH 0/3] rdma fixes and clean ups

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori