Re: [Qemu-devel] [PATCH v2 06/12] Translate offsets to destination address space

["Michael R. Hines" <mrhines@linux.vnet.ibm.com>]

On 06/11/2015 12:17 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The 'offset' field in RDMACompress and 'current_addr' field
> in RDMARegister are commented as being offsets within a particular
> RAMBlock, however they appear to actually be offsets within the
> ram_addr_t space.
>
> The code currently assumes that the offsets on the source/destination
> match, this change removes the need for the assumption for these
> structures by translating the addresses into the ram_addr_t space of
> the destination host.
>
> Note: An alternative would be to change the fields to actually
> take the data they're commented for; this would potentially be
> simpler but would break stream compatibility for those cases
> that currently work.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   migration/rdma.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 9532461..cb66721 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -411,7 +411,7 @@ static void network_to_control(RDMAControlHeader *control)
>    */
>   typedef struct QEMU_PACKED {
>       union QEMU_PACKED {
> -        uint64_t current_addr;  /* offset into the ramblock of the chunk */
> +        uint64_t current_addr;  /* offset into the ram_addr_t space */
>           uint64_t chunk;         /* chunk to lookup if unregistering */
>       } key;
>       uint32_t current_index; /* which ramblock the chunk belongs to */
> @@ -419,8 +419,19 @@ typedef struct QEMU_PACKED {
>       uint64_t chunks;            /* how many sequential chunks to register */
>   } RDMARegister;
>
> -static void register_to_network(RDMARegister *reg)
> +static void register_to_network(RDMAContext *rdma, RDMARegister *reg)
>   {
> +    RDMALocalBlock *local_block;
> +    local_block  = &rdma->local_ram_blocks.block[reg->current_index];
> +
> +    if (local_block->is_ram_block) {
> +        /*
> +         * current_addr as passed in is an address in the local ram_addr_t
> +         * space, we need to translate this for the destination
> +         */
> +        reg->key.current_addr -= local_block->offset;
> +        reg->key.current_addr += rdma->dest_blocks[reg->current_index].offset;
> +    }
>       reg->key.current_addr = htonll(reg->key.current_addr);
>       reg->current_index = htonl(reg->current_index);
>       reg->chunks = htonll(reg->chunks);
> @@ -436,13 +447,19 @@ static void network_to_register(RDMARegister *reg)
>   typedef struct QEMU_PACKED {
>       uint32_t value;     /* if zero, we will madvise() */
>       uint32_t block_idx; /* which ram block index */
> -    uint64_t offset;    /* where in the remote ramblock this chunk */
> +    uint64_t offset;    /* Address in remote ram_addr_t space */
>       uint64_t length;    /* length of the chunk */
>   } RDMACompress;
>
> -static void compress_to_network(RDMACompress *comp)
> +static void compress_to_network(RDMAContext *rdma, RDMACompress *comp)
>   {
>       comp->value = htonl(comp->value);
> +    /*
> +     * comp->offset as passed in is an address in the local ram_addr_t
> +     * space, we need to translate this for the destination
> +     */
> +    comp->offset -= rdma->local_ram_blocks.block[comp->block_idx].offset;
> +    comp->offset += rdma->dest_blocks[comp->block_idx].offset;
>       comp->block_idx = htonl(comp->block_idx);
>       comp->offset = htonll(comp->offset);
>       comp->length = htonll(comp->length);

So, why add the destination block's offset on the source side
just for it to be re-adjusted again when it gets to the destination side?

Can you just stop at this:

+        reg->key.current_addr -= local_block->offset;

Without this:

+        reg->key.current_addr += 
rdma->dest_blocks[reg->current_index].offset;

... on the source, followed by this on the destionation:

+    comp->offset -= rdma->local_ram_blocks.block[comp->block_idx].offset;

Without this:

+    comp->offset += rdma->dest_blocks[comp->block_idx].offset;

Did I follow correctly?

> @@ -1288,7 +1305,7 @@ static int qemu_rdma_unregister_waiting(RDMAContext *rdma)
>           rdma->total_registrations--;
>
>           reg.key.chunk = chunk;
> -        register_to_network(&reg);
> +        register_to_network(rdma, &reg);
>           ret = qemu_rdma_exchange_send(rdma, &head, (uint8_t *) &reg,
>                                   &resp, NULL, NULL);
>           if (ret < 0) {
> @@ -1909,7 +1926,7 @@ retry:
>                   trace_qemu_rdma_write_one_zero(chunk, sge.length,
>                                                  current_index, current_addr);
>
> -                compress_to_network(&comp);
> +                compress_to_network(rdma, &comp);
>                   ret = qemu_rdma_exchange_send(rdma, &head,
>                                   (uint8_t *) &comp, NULL, NULL, NULL);
>
> @@ -1936,7 +1953,7 @@ retry:
>               trace_qemu_rdma_write_one_sendreg(chunk, sge.length, current_index,
>                                                 current_addr);
>
> -            register_to_network(&reg);
> +            register_to_network(rdma, &reg);
>               ret = qemu_rdma_exchange_send(rdma, &head, (uint8_t *) &reg,
>                                       &resp, &reg_result_idx, NULL);
>               if (ret < 0) {