Re: [Qemu-devel] [PATCH 04/10] Translate offsets to destination address space

[Dr. David Alan Gilbert]

* Michael R. Hines (address@hidden) wrote:
> On 04/20/2015 10:57 AM, Dr. David Alan Gilbert (git) wrote:
> >From: "Dr. David Alan Gilbert" <address@hidden>
> >
> >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.
> 
> I don't understand fully: If the offsets are not the same, then
> why would the RAMBlocks be the same? If a RAMBlock
> is hot-plugged on one side, shouldn't an identical one be
> hotplugged on the other side, including the offset into ram_addr_t?

If a RAMBlock is hotplugged on the source, it's normally passed in on
the command line at startup on the destination, not hotplugged.

This difference in order of allocation of the RAMBlocks can cause
the allocation of space in ram_addr_t to be different.

In addition changes between qemu versions as to the order in which
devices are initialised can cause differences in the allocation in ram_addr_t.

Indeed the allocation algorithm isn't very deterministic, I think it looks
for the smallest gap that will fit for the allocation.
Also, lets say that you hot plugged 6 devices, and hot unplugged 5,
then migrated;  on the destination you only see one of them, not the history
of the other allocations that had to be performed.

> (Even if the base address of the ram_addr_t space is different
> between source and destination, then at least the offsets
> and list of blocks should be the same, no?)

Neither the offsets nor the order is deterministic (as mentioned above);
only the naming.

> Is hotplugging an asynchronous operation for post-copy or
> something?

Postcopy doesn't change that; just don't try hotplugging during the
migration.

Dave



> 
> >
> >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 <address@hidden>
> >---
> >  migration/rdma.c | 31 ++++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> >diff --git a/migration/rdma.c b/migration/rdma.c
> >index c3814c5..2c0d11b 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;
> 
> The below seems OK, but I would prefer not to do this translation here.
> Can the source and destination apply the offset calculations outside
> of the byte-order functions? Like, before register_to_network, the
> source removes the offset, and then when the message is received,
> the destination then again re-applies the correct offset?
> 
> >-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);
> 
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK