Re: [Qemu-devel] [PATCH v3 resend 4/8] rdma: core logic

[Marcel Apfelbaum]

On Tue, 2013-07-16 at 12:48 -0400, address@hidden wrote:
> From: "Michael R. Hines" <address@hidden>
> 
> Code that does need to be visible is kept
> well contained inside this file and this is the only
> new additional file to the entire patch.
> 
> This file includes the entire protocol and interfaces
> required to perform RDMA migration.
> 
> Also, the configure and Makefile modifications to link
> this file are included.
> 
> Full documentation is in docs/rdma.txt
> 

This patch is too big (in my opinion).
I would split it into at least 3 patches:
1. Generic RDMA code (this part can be reused by everyone who will need RDMA in 
the future) 
2. RDMA transfer protocol (separating this will give us possibility for 
optimization without touching the rest of the code)
3. Migration related code



> + */
> +#define RDMA_WRID_TYPE_SHIFT  0UL
> +#define RDMA_WRID_BLOCK_SHIFT 16UL
> +#define RDMA_WRID_CHUNK_SHIFT 30UL

If I understand correctly each RDMA write is exactly 1MB.
I think that it is crucial from the performance point of view to make this 
configurable.
Sometimes the time to register the MR(and unregister) on both machines may be 
the same as transmission of 1MB.
Bottom line, this cannot be hard-coded because it depends on the connection 
speed.


> +/*
> + * RDMA requires memory registration (mlock/pinning), but this is not good 
> for
> + * overcommitment.
> + *
> + * In preparation for the future where LRU information or workload-specific
> + * writable writable working set memory access behavior is available to QEMU
> + * it would be nice to have in place the ability to UN-register/UN-pin
> + * particular memory regions from the RDMA hardware when it is determine that
> + * those regions of memory will likely not be accessed again in the near 
> future.
> + *
> + * While we do not yet have such information right now, the following
> + * compile-time option allows us to perform a non-optimized version of this
> + * behavior.
> + *
> + * By uncommenting this option, you will cause *all* RDMA transfers to be
> + * unregistered immediately after the transfer completes on both sides of the
> + * connection. This has no effect in 'rdma-pin-all' mode, only regular mode.
> + *
> + * This will have a terrible impact on migration performance, so until future
> + * workload information or LRU information is available, do not attempt to 
> use
> + * this feature except for basic testing.
> + */
> +//#define RDMA_UNREGISTRATION_EXAMPLE
> +
> +/*
> + * Perform a non-optimized memory unregistration after every transfer
> + * for demonsration purposes, only if pin-all is not requested.
> + *
> + * Potential optimizations:
> + * 1. Start a new thread to run this function continuously
> +        - for bit clearing
> +        - and for receipt of unregister messages
> + * 2. Use an LRU.
> + * 3. Use workload hints.
> + */

I think that if we work with chunks large enough, in such way that the time to 
pass 
the pages between hosts is much bigger the MR registration/unregistration, it 
may solve
the issue as least partially (it will not be such an impact).
After that, optimization is a good idea.


Nice work!
Marcel