Re: [Qemu-devel] [PATCH v2 4/5] migration: implement bi-directional RDMA QIOChannel

[Daniel P . Berrangé]

On Sat, Apr 28, 2018 at 12:16:36PM +0800, 858585 jemmy wrote:
> On Fri, Apr 27, 2018 at 5:16 PM, Daniel P. Berrangé <address@hidden> wrote:
> so I prefer to add a lock for QEMUFile struct, like this:
> 
> int qemu_fclose(QEMUFile *f)
> {
>     int ret;
>     qemu_fflush(f);
>     ret = qemu_file_get_error(f);
> 
>     if (f->ops->close) {
> +        qemu_mutex_lock(&f->lock);
>         int ret2 = f->ops->close(f->opaque);
> +       qemu_mutex_unlock(&f->lock);
>         if (ret >= 0) {
>             ret = ret2;
>         }
>     }

That looks ok


> >> > But is there nothing else to make the QIOChannel bidirectional?
> >> >
> >> > Also, a lot seems dependent on listen_id, can you explain how that's
> >> > being used.
> >>
> >> The destination qemu is server side, so listen_id is not zero. the
> >> source qemu is client side,
> >> the listen_id is zero.
> >> I use listen_id to determine whether qemu is destination or source.
> >>
> >> for the destination qemu, if write data to source, it need use the
> >> return_path rdma, like this:
> >>     if (rdma->listen_id) {
> >>         rdma = rdma->return_path;
> >>     }
> >>
> >> for the source qemu, if read data from destination, it also need use
> >> the return_path rdma.
> >>     if (!rdma->listen_id) {
> >>         rdma = rdma->return_path;
> >>     }
> >
> > This feels uncessarily complex to me. Why not just change QIOCHannelRMDA
> > struct, so that it has 2 RDMA context pointers eg
> >
> > struct QIOChannelRDMA {
> >     QIOChannel parent;
> >     RDMAContext *rdmain;
> >     RDMAContext *rdmaout;
> >     QEMUFile *file;
> >     bool blocking; /* XXX we don't actually honour this yet */
> > };
> 
> The reason is the parameter of some function is RDMAContext.
> like qemu_rdma_accept, rdma_start_outgoing_migration.
> It's easier to implement return path.

I don't see that as an issue - qemu_fopen_rdma() function should be able
to setup a pair of RDMAContext pointers when it creates the QIOChannelRDMA
struct. It just has to request the return path from the single RDMAContext
it gets passed, and then set that on the QIOChannelRDMA struct.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|