Re: [Qemu-devel] [PATCH 2/2] rdma: simplify qemu_rdma_register_and_get_keys()

[Michael R. Hines]

On 09/03/2013 10:32 PM, Isaku Yamahata wrote:
> Signed-off-by: Isaku Yamahata <address@hidden>
> ---
>   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;