Re: [Qemu-devel] [RFC PATCH RDMA support v4: 08/10] introduce QEMUFileRDMA

[Michael R. Hines]

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