[2/2] rdma: simplify qemu_rdma_register_and_get_keys()

Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
---
 migration-rdma.c |   23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

Isaku Yamahata <yamahata@private.email.ne.jp> wrote:
> Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>

I don't understand this bit:

> +        uint8_t *chunk_start = ram_chunk_start(block, chunk);
> +        uint8_t *chunk_end = ram_chunk_end(block, chunk);

Here we are getting just the length of a chunk


>      sge.length = length;
>  
>      chunk = ram_chunk_index(block->local_host_addr, (uint8_t *) sge.addr);
> -    chunk_start = ram_chunk_start(block, chunk);
>  
>      if (block->is_ram_block) {
>          chunks = length / (1UL << RDMA_REG_CHUNK_SHIFT);
> @@ -1884,8 +1883,6 @@ retry:
>      DDPRINTF("Writing %" PRIu64 " chunks, (%" PRIu64 " MB)\n",
>          chunks + 1, (chunks + 1) * (1UL << RDMA_REG_CHUNK_SHIFT) / 1024 / 1024);
>  
> -    chunk_end = ram_chunk_end(block, chunk + chunks);
> -

But here chuck_end was the length of a list of chucks.


> @@ -3076,11 +3069,9 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>                      host_addr = block->local_host_addr +
>                          (reg->key.chunk * (1UL << RDMA_REG_CHUNK_SHIFT));
>                  }
> -                chunk_start = ram_chunk_start(block, chunk);
> -                chunk_end = ram_chunk_end(block, chunk + reg->chunks);
>                  if (qemu_rdma_register_and_get_keys(rdma, block,
>                              (uint8_t *)host_addr, NULL, &reg_result->rkey,
> -                            chunk, chunk_start, chunk_end)) {
> +                            chunk)) {

Same here.


I am missing something obvious?

PD.  No, I don't claim to understand RDMA.

On 09/03/2013 10:32 PM, Isaku Yamahata wrote:
> Signed-off-by: Isaku Yamahata <yamahata@private.email.ne.jp>
> ---
>   migration-rdma.c |   23 +++++++----------------
>   1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/migration-rdma.c b/migration-rdma.c
> index db5a908..941c07e 100644
> --- a/migration-rdma.c
> +++ b/migration-rdma.c
> @@ -1128,8 +1128,7 @@ static int qemu_rdma_search_ram_block(RDMAContext *rdma,
>    */
>   static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
>           RDMALocalBlock *block, uint8_t *host_addr,
> -        uint32_t *lkey, uint32_t *rkey, int chunk,
> -        uint8_t *chunk_start, uint8_t *chunk_end)
> +        uint32_t *lkey, uint32_t *rkey, int chunk)
>   {
>       if (block->mr) {
>           if (lkey) {
> @@ -1155,6 +1154,8 @@ static int qemu_rdma_register_and_get_keys(RDMAContext *rdma,
>        * If 'lkey', then we're the source VM, so grant access only to ourselves.
>        */
>       if (!block->pmr[chunk]) {
> +        uint8_t *chunk_start = ram_chunk_start(block, chunk);
> +        uint8_t *chunk_end = ram_chunk_end(block, chunk);
>           uint64_t len = chunk_end - chunk_start;

NACK. chunk_end cannot be calculated like this for all calls to this 
function.

See below.......

>
>           DDPRINTF("Registering %" PRIu64 " bytes @ %p\n",
> @@ -1849,7 +1850,6 @@ static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
>       struct ibv_send_wr *bad_wr;
>       int reg_result_idx, ret, count = 0;
>       uint64_t chunk, chunks;
> -    uint8_t *chunk_start, *chunk_end;
>       RDMALocalBlock *block = &(rdma->local_ram_blocks.block[current_index]);
>       RDMARegister reg;
>       RDMARegisterResult *reg_result;
> @@ -1865,7 +1865,6 @@ retry:
>       sge.length = length;
>
>       chunk = ram_chunk_index(block->local_host_addr, (uint8_t *) sge.addr);
> -    chunk_start = ram_chunk_start(block, chunk);
>
>       if (block->is_ram_block) {
>           chunks = length / (1UL << RDMA_REG_CHUNK_SHIFT);
> @@ -1884,8 +1883,6 @@ retry:
>       DDPRINTF("Writing %" PRIu64 " chunks, (%" PRIu64 " MB)\n",
>           chunks + 1, (chunks + 1) * (1UL << RDMA_REG_CHUNK_SHIFT) / 1024 / 1024);
>
> -    chunk_end = ram_chunk_end(block, chunk + chunks);
> -

NACK.

The value of chunk_end changes based on whether or not the value of 
block->is_ram_block is true of false.

>       if (!rdma->pin_all) {
>   #ifdef RDMA_UNREGISTRATION_EXAMPLE
>           qemu_rdma_unregister_waiting(rdma);
> @@ -1974,8 +1971,7 @@ retry:
>               /* try to overlap this single registration with the one we sent. */
>               if (qemu_rdma_register_and_get_keys(rdma, block,
>                                                   (uint8_t *) sge.addr,
> -                                                &sge.lkey, NULL, chunk,
> -                                                chunk_start, chunk_end)) {
> +                                                &sge.lkey, NULL, chunk)) {
>                   fprintf(stderr, "cannot get lkey!\n");
>                   return -EINVAL;
>               }
> @@ -1995,8 +1991,7 @@ retry:
>               /* already registered before */
>               if (qemu_rdma_register_and_get_keys(rdma, block,
>                                                   (uint8_t *)sge.addr,
> -                                                &sge.lkey, NULL, chunk,
> -                                                chunk_start, chunk_end)) {
> +                                                &sge.lkey, NULL, chunk)) {
>                   fprintf(stderr, "cannot get lkey!\n");
>                   return -EINVAL;
>               }
> @@ -2007,8 +2002,7 @@ retry:
>           send_wr.wr.rdma.rkey = block->remote_rkey;
>
>           if (qemu_rdma_register_and_get_keys(rdma, block, (uint8_t *)sge.addr,
> -                                                     &sge.lkey, NULL, chunk,
> -                                                     chunk_start, chunk_end)) {
> +                                                     &sge.lkey, NULL, chunk)) {
>               fprintf(stderr, "cannot get lkey!\n");
>               return -EINVAL;
>           }
> @@ -3054,7 +3048,6 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>
>               for (count = 0; count < head.repeat; count++) {
>                   uint64_t chunk;
> -                uint8_t *chunk_start, *chunk_end;
>
>                   reg = &registers[count];
>                   network_to_register(reg);
> @@ -3076,11 +3069,9 @@ static int qemu_rdma_registration_handle(QEMUFile *f, void *opaque,
>                       host_addr = block->local_host_addr +
>                           (reg->key.chunk * (1UL << RDMA_REG_CHUNK_SHIFT));
>                   }
> -                chunk_start = ram_chunk_start(block, chunk);
> -                chunk_end = ram_chunk_end(block, chunk + reg->chunks);
>                   if (qemu_rdma_register_and_get_keys(rdma, block,
>                               (uint8_t *)host_addr, NULL, &reg_result->rkey,
> -                            chunk, chunk_start, chunk_end)) {
> +                            chunk)) {
>                       fprintf(stderr, "cannot get rkey!\n");
>                       ret = -EINVAL;
>                       goto out;

On Wed, Sep 18, 2013 at 11:01:16AM -0400, Michael R. Hines wrote:
> The value of chunk_end changes based on whether or not the value of  
> block->is_ram_block is true of false.

When is block->is_ram_block set to false?
Looking at __qemu_rdma_add_block() and qemu_rdma_init_ram_blocks(),
is_ram_block seems to be always set to true because local->init = false
when __qemu_rdma_add_block() is called.

On 09/20/2013 12:59 PM, Isaku Yamahata wrote:
> On Wed, Sep 18, 2013 at 11:01:16AM -0400, Michael R. Hines wrote:
>> The value of chunk_end changes based on whether or not the value of
>> block->is_ram_block is true of false.
> When is block->is_ram_block set to false?
> Looking at __qemu_rdma_add_block() and qemu_rdma_init_ram_blocks(),
> is_ram_block seems to be always set to true because local->init = false
> when __qemu_rdma_add_block() is called.

That's correct, but I have other patches (not yet posted)
that depend on block->is_ram_block.

Please don't delete it =)

- Michael