Re: [Qemu-devel] [PATCH 07/10] Simplify rdma_delete_block and remove it's dependence on the hash

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

On 04/20/2015 10:57 AM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> rdma_delete_block is currently very general, but it's only used
> in cleanup at the end.   Simplify it and remove it's dependence
> on the hash table and remove all of the hash-table regeneration
> designed to handle the (unused) case of deleting an arbitrary block.

Can we not delete this? I have a patch to QEMU that allows
us to perform RDMA transfers of arbitrary regions of memory
in QEMU at anytime in the migration process (you might guess
for the purposes of fault tolerance). This function will in fact
get called more often in the future ---- particularly if we want
to allow other subsystems, such as storage to register regions
of memory to be transferred.

> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   migration/rdma.c | 57 +++++++++-----------------------------------------------
>   trace-events     |  2 +-
>   2 files changed, 10 insertions(+), 49 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 4f7dd0d..fe3b76e 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -617,16 +617,11 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
>       return 0;
>   }
>
> -static int rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
> +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;
>
> @@ -656,51 +651,15 @@ static int rdma_delete_block(RDMAContext *rdma, ram_addr_t block_offset)
>       g_free(block->remote_keys);
>       block->remote_keys = NULL;
>
> -    for (x = 0; x < local->nb_blocks; x++) {
> -        g_hash_table_remove(rdma->blockmap, (void *)(uintptr_t)old[x].offset);
> -    }
> -
>       g_free(block->block_name);
>       block->block_name = NULL;
>
> -    if (local->nb_blocks > 1) {
> -
> -        local->block = g_malloc0(sizeof(RDMALocalBlock) *
> -                                    (local->nb_blocks - 1));
> -
> -        if (block->index) {
> -            memcpy(local->block, old, sizeof(RDMALocalBlock) * block->index);
> -        }
> -
> -        if (block->index < (local->nb_blocks - 1)) {
> -            memcpy(local->block + block->index, old + (block->index + 1),
> -                sizeof(RDMALocalBlock) *
> -                    (local->nb_blocks - (block->index + 1)));
> -        }
> -    } else {
> -        assert(block == local->block);
> -        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) *
>                                  sizeof(unsigned long) * 8, block->nb_chunks);
>
> -    g_free(old);
> -
> -    local->nb_blocks--;
> -
> -    if (local->nb_blocks) {
> -        for (x = 0; x < local->nb_blocks; x++) {
> -            g_hash_table_insert(rdma->blockmap,
> -                                (void *)(uintptr_t)local->block[x].offset,
> -                                &local->block[x]);
> -        }
> -    }
> -
>       return 0;
>   }
>
> @@ -2213,9 +2172,11 @@ 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);
> +        for (idx = 0; idx < rdma->local_ram_blocks.nb_blocks; idx++) {
> +            rdma_delete_block(rdma, &rdma->local_ram_blocks.block[idx]);
>           }
> +        g_free(rdma->local_ram_blocks.block);
> +        rdma->local_ram_blocks.block = NULL;
>       }
>
>       if (rdma->qp) {
> diff --git a/trace-events b/trace-events
> index baf8647..bdb0868 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1436,7 +1436,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) ""