Re: [Qemu-devel] [PATCH v1 13/22] migration: convert RDMA to use QIOChannel interface

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Tue, Feb 02, 2016 at 08:01:36PM +0000, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > This converts the RDMA code to provide a subclass of
> > > QIOChannel that uses RDMA for the data transport.
> > > 
> > > The RDMA code would be much better off it it could
> > > be split up in a generic RDMA layer, a QIOChannel
> > > impl based on RMDA, and then the RMDA migration
> > > glue. This is left as a future exercise for the brave.
> > > 
> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > ---
> > >  migration/rdma.c | 260 ++++++++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 161 insertions(+), 99 deletions(-)
> > > 
> 
> > > +static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
> > > +                                      const struct iovec *iov,
> > > +                                      size_t niov,
> > > +                                      int **fds,
> > > +                                      size_t *nfds,
> > > +                                      Error **errp)
> > > +{
> > > +    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
> > > +    RDMAContext *rdma = rioc->rdma;
> > >      RDMAControlHeader head;
> > >      int ret = 0;
> > > +    ssize_t i;
> > > +    size_t done = 0;
> > >  
> > >      CHECK_ERROR_STATE();
> > >  
> > > -    /*
> > > -     * First, we hold on to the last SEND message we
> > > -     * were given and dish out the bytes until we run
> > > -     * out of bytes.
> > > -     */
> > > -    r->len = qemu_rdma_fill(r->rdma, buf, size, 0);
> > > -    if (r->len) {
> > > -        return r->len;
> > > -    }
> > > +    for (i = 0; i < niov; i++) {
> > > +        size_t want = iov[i].iov_len;
> > > +        uint8_t *data = (void *)iov[i].iov_base;
> > >  
> > > -    /*
> > > -     * Once we run out, we block and wait for another
> > > -     * SEND message to arrive.
> > > -     */
> > > -    ret = qemu_rdma_exchange_recv(rdma, &head, RDMA_CONTROL_QEMU_FILE);
> > > +        /*
> > > +         * First, we hold on to the last SEND message we
> > > +         * were given and dish out the bytes until we run
> > > +         * out of bytes.
> > > +         */
> > > +        ret = qemu_rdma_fill(rioc->rdma, data, want, 0);
> > > +        if (ret > 0) {
> > > +            done += ret;
> > > +            if (ret < want) {
> > > +                break;
> > > +            } else {
> > > +                continue;
> > > +            }
> > 
> > > +        }
> > >  
> > > -    if (ret < 0) {
> > > -        rdma->error_state = ret;
> > > -        return ret;
> > > -    }
> > > +        /*
> > > +         * Once we run out, we block and wait for another
> > > +         * SEND message to arrive.
> > > +         */
> > > +        ret = qemu_rdma_exchange_recv(rdma, &head, RDMA_CONTROL_QEMU_FILE);
> > >  
> > > -    /*
> > > -     * SEND was received with new bytes, now try again.
> > > -     */
> > > -    return qemu_rdma_fill(r->rdma, buf, size, 0);
> > > +        if (ret < 0) {
> > > +            rdma->error_state = ret;
> > > +            return ret;
> > > +        }
> > > +
> > > +        /*
> > > +         * SEND was received with new bytes, now try again.
> > > +         */
> > > +        ret = qemu_rdma_fill(rioc->rdma, data, want, 0);
> > > +        if (ret > 0) {
> > > +            done += ret;
> > > +            if (ret < want) {
> > > +                break;
> > > +            }
> > > +        }
> > 
> > I don't quite understand the behaviour of this loop.
> > If rdma_fill returns less than you wanted for the first iov we break.
> > If it returns 0 then we try and get some more.
> > The weird thing to me is if we have two iov entries; if the
> > amount returned by the qemu_rdma_fill happens to match the size of
> > the 1st iov then I think we end up doing the exchange_recv and
> > waiting for more.  Is that what we want? Why?
> 
> No, it isn't quite what we want. If we have successfully received
> some data in a preceeding iov, then we shouldn't wait for more data
> for any following iov. I'll rework this bit of code to work better
> 
> In fact technically, we should not block for data at all when the
> channel is in non-blocking mode. Fixing that would require some
> refactoring of qemu_rdma_block_for_wrid() so that we could tell
> it to only look for an already completed work request and not
> block

I wouldn't go changing qemu_rdma_block_for_wrid unless you need
to.

Dave

> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK