Comments inline - tell me what you think.......
On 03/18/2013 05:09 AM, Paolo Bonzini wrote:
+typedef struct QEMUFileRDMA
+{
+ void *rdma;
This is an RDMAData *. Please avoid using void * as much as possible.
Acknowledged - forgot to move this to rdma.c, so it doesn't have to be
void anymore.
*/
+ return qemu_rdma_fill(r->rdma, buf, size);
+}
Please move these functions closer to qemu_fopen_rdma (or better, to an
RDMA-specific file altogether). Also, using qemu_rdma_fill introduces a
dependency of savevm.c on migration-rdma.c. There should be no such
dependency; migration-rdma.c should be used only by migration.c.
Acknowledged......
+void * migrate_use_rdma(QEMUFile *f)
+{
+ QEMUFileRDMA *r = f->opaque;
+
+ return qemu_rdma_enabled(r->rdma) ? r->rdma : NULL;
You cannot be sure that f->opaque->rdma is a valid pointer. For
example, the first field in a socket QEMUFile's is a file descriptor.
Instead, you could use a qemu_file_ops_are(const QEMUFile *, const
QEMUFileOps *) function that checks if the file uses the given ops.
Then, migrate_use_rdma can simply check if the QEMUFile is using the
RDMA ops structure.
With this change, the "enabled" field of RDMAData should go.
Great - I like that...... will do....
+/*
+ * Called only for RDMA right now at the end
+ * of each live iteration of memory.
+ *
+ * 'drain' from a QEMUFile perspective means
+ * to flush the outbound send buffer
+ * (if one exists).
+ *
+ * For RDMA, this means to make sure we've
+ * received completion queue (CQ) messages
+ * successfully for all of the RDMA writes
+ * that we requested.
+ */
+int qemu_drain(QEMUFile *f)
+{
+ return f->ops->drain ? f->ops->drain(f->opaque) : 0;
+}
Hmm, this is very similar to qemu_fflush, but not quite. :/
Why exactly is this needed?
Good idea - I'll replace drain with flush once I added
the "qemu_file_ops_are(const QEMUFile *, const QEMUFileOps *) "
that you recommended......
/** Flushes QEMUFile buffer
*
*/
@@ -723,6 +867,8 @@ int qemu_get_byte(QEMUFile *f)
int64_t qemu_ftell(QEMUFile *f)
{
qemu_fflush(f);
+ if(migrate_use_rdma(f))
+ return delta_norm_mig_bytes_transferred();
Not needed, and another undesirable dependency (savevm.c ->
arch_init.c). Just update f->pos in save_rdma_page.
f->pos isn't good enough because save_rdma_page does not
go through QEMUFile directly - only non-live state goes
through QEMUFile ....... pc.ram uses direct RDMA writes.
As a result, the position pointer does not get updated
and the accounting is missed........
- Michael
