Re: [Qemu-devel] [PATCH v2 08/12] Allow rdma_delete_block to work without the hash

[Michael R. Hines]

On 06/11/2015 12:17 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> In the next patch we remove the hash on the destination,
> rdma_delete_block does two things with the hash which can be avoided:
>    a) The caller passes the offset and rdma_delete_block looks it up
>       in the hash; fixed by getting the caller to pass the block
>    b) The hash gets recreated after deletion; fixed by making that
>       conditional on the hash being initialised.
>
> While this function is currently only used during cleanup, Michael
> asked that we keep it general for future dynamic block registration
> work.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>   migration/rdma.c | 27 ++++++++++++++++-----------
>   trace-events     |  2 +-
>   2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 396329c..8d99378 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -617,16 +617,19 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
>       return 0;
>   }
>
> -static int rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
> +/*
> + * Note: If used outside of cleanup, the caller must ensure that the 
> destination
> + * block structures are also updated
> + */
> +static int rdma_delete_block(RDMAContext *rdma, RDMALocalBlock *block)
>   {
>       RDMALocalBlocks *local = &rdma->local_ram_blocks;
> -    RDMALocalBlock *block = g_hash_table_lookup(rdma->blockmap,
> -        (void *) block_offset);
>       RDMALocalBlock *old = local->block;
>       int x;
>
> -    assert(block);
> -
> +    if (rdma->blockmap) {
> +        g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)block->offset);
> +    }
>       if (block->pmr) {
>           int j;
>
> @@ -659,8 +662,11 @@ static int rdma_delete_block(RDMAContext *rdma, ram_addr_t 
> block_offset)
>       g_free(block->block_name);
>       block->block_name = NULL;
>
> -    for (x = 0; x < local->nb_blocks; x++) {
> -        g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)old[x].offset);
> +    if (rdma->blockmap) {
> +        for (x = 0; x < local->nb_blocks; x++) {
> +            g_hash_table_remove(rdma->blockmap,
> +                                (void *)(uintptr_t)old[x].offset);
> +        }
>       }
>
>       if (local->nb_blocks > 1) {
> @@ -682,8 +688,7 @@ static int rdma_delete_block(RDMAContext *rdma, ram_addr_t 
> block_offset)
>           local->block = NULL;
>       }
>
> -    trace_rdma_delete_block(local->nb_blocks,
> -                           (uintptr_t)block->local_host_addr,
> +    trace_rdma_delete_block(block, (uintptr_t)block->local_host_addr,
>                              block->offset, block->length,
>                               (uintptr_t)(block->local_host_addr + 
> block->length),
>                              BITS_TO_LONGS(block->nb_chunks) *
> @@ -693,7 +698,7 @@ static int rdma_delete_block(RDMAContext *rdma, ram_addr_t 
> block_offset)
>
>       local->nb_blocks--;
>
> -    if (local->nb_blocks) {
> +    if (local->nb_blocks && rdma->blockmap) {
>           for (x = 0; x < local->nb_blocks; x++) {
>               g_hash_table_insert(rdma->blockmap,
>                                   (void *)(uintptr_t)local->block[x].offset,
> @@ -2214,7 +2219,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>
>       if (rdma->local_ram_blocks.block) {
>           while (rdma->local_ram_blocks.nb_blocks) {
> -            rdma_delete_block(rdma, rdma->local_ram_blocks.block->offset);
> +            rdma_delete_block(rdma, &rdma->local_ram_blocks.block[0]);
>           }
>       }

Looks good overall. Maybe this is a silly question, but have you done
a few migrations over actual RDMA hardware yet?

Reviewed-by: Michael R. Hines <address@hidden>

> diff --git a/trace-events b/trace-events
> index 0f37a4b..7dff362 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1452,7 +1452,7 @@ qemu_rdma_write_one_sendreg(uint64_t chunk, int len, int 
> index, int64_t offset)
>   qemu_rdma_write_one_top(uint64_t chunks, uint64_t size) "Writing %" PRIu64 " chunks, 
> (%" PRIu64 " MB)"
>   qemu_rdma_write_one_zero(uint64_t chunk, int len, int index, int64_t offset) "Entire chunk 
> is zero, sending compress: %" PRIu64 " for %d bytes, index: %d, offset: %" PRId64
>   rdma_add_block(const char *block_name, int block, uint64_t addr, uint64_t offset, uint64_t len, uint64_t end, uint64_t bits, int chunks) 
> "Added Block: '%s':%d, addr: %" PRIu64 ", offset: %" PRIu64 " length: %" PRIu64 " end: %" PRIu64 
> " bits %" PRIu64 " chunks %d"
> -rdma_delete_block(int block, uint64_t addr, uint64_t offset, uint64_t len, uint64_t end, uint64_t bits, int chunks) "Deleted Block: 
> %d, addr: %" PRIu64 ", offset: %" PRIu64 " length: %" PRIu64 " end: %" PRIu64 " bits %" PRIu64 
> " chunks %d"
> +rdma_delete_block(void *block, uint64_t addr, uint64_t offset, uint64_t len, uint64_t end, uint64_t bits, int chunks) "Deleted Block: 
> %p, addr: %" PRIu64 ", offset: %" PRIu64 " length: %" PRIu64 " end: %" PRIu64 " bits %" PRIu64 
> " chunks %d"
>   rdma_start_incoming_migration(void) ""
>   rdma_start_incoming_migration_after_dest_init(void) ""
>   rdma_start_incoming_migration_after_rdma_listen(void) ""
These messages are also empty? What happened to them? =)