Re: [Qemu-devel] [PATCH 3/6] rdma: core logic

[Michael R. Hines]

On 06/27/2013 07:16 PM, Peter Maydell wrote:
> On 27 June 2013 23:44,  <address@hidden> wrote:
> > +if test "$rdma" != "no" ; then
> > +  cat > $TMPC <<EOF
> > +#include <rdma/rdma_cma.h>
> > +int main(void) { return 0; }
> > +EOF
> > +  rdma_libs="-lrdmacm -libverbs"
> > +  if compile_prog "-Werror" "$rdma_libs" ; then
> Do you really need -Werror in your test's CFLAGS?
> If so, you need a comment explaining why (as with the
> couple of other tests which do this). If not, just use
> "" like the majority of our tests.
>
> Also, configure's not totally consistent about it but
> (especially given where in the file you've put this) you
> could add in a comment header like the one you can just see
> for VNC at the bottom of the context:

Will drop the -Werror, no problem. (I think I copied and pasted 99% of 
the configure changes),
can't even remember why I added it in the first place. Perhaps I was 
debugging something
and forgot to remove it....

> > +    rdma="yes"
> > +    libs_softmmu="$libs_softmmu $rdma_libs"
> > +  else
> > +    if test "$rdma" = "yes" ; then
> > +      feature_not_found "rdma"
> > +    fi
> > +    rdma="no"
> > +  fi
> > +fi
> > +
> >   ##########################################
> >   # VNC TLS/WS detection
> >   if test "$vnc" = "yes" -a \( "$vnc_tls" != "no" -o "$vnc_ws" != "no" \) ; then
> ...there.

Acknowledged.

> > @@ -3525,6 +3549,7 @@ echo "Linux AIO support $linux_aio"
> >   echo "ATTR/XATTR support $attr"
> >   echo "Install blobs     $blobs"
> >   echo "KVM support       $kvm"
> > +echo "RDMA support      $rdma"
> >   echo "TCG interpreter   $tcg_interpreter"
> >   echo "fdt support       $fdt"
> >   echo "preadv support    $preadv"
> > @@ -4464,6 +4489,10 @@ if [ "$pixman" = "internal" ]; then
> >     echo "config-host.h: subdir-pixman" >> $config_host_mak
> >   fi
> >
> > +if test "$rdma" = "yes" ; then
> > +echo "CONFIG_RDMA=y" >> $config_host_mak
> > +fi
> > +
> This is definitely completely the wrong place for this hunk:
> you've put it between the two "here's a submodule directory"
> stanzas. Put it further up with all the other simple "if
> thing then echo to $config_host_mak" checks (VNC and so on).
>
> Also, you need to indent the echo line.

Acknowledged.

> >   if [ "$dtc_internal" = "yes" ]; then
> >     echo "config-host.h: subdir-dtc" >> $config_host_mak
> >   fi
> > +const char *wrid_desc[] = {
> > +        [RDMA_WRID_NONE] = "NONE",
> > +        [RDMA_WRID_RDMA_WRITE] = "WRITE RDMA",
> > +        [RDMA_WRID_SEND_CONTROL] = "CONTROL SEND",
> > +        [RDMA_WRID_RECV_CONTROL] = "CONTROL RECV",
> > +};
> Weird indent (here and elsewhere).

Acknowledged.

> > +/*
> > + * Also represents a RAMblock, but only on the dest.
> > + * This gets transmitted by the dest during connection-time
> > + * to the source VM and then is used to populate the
> > + * corresponding RDMALocalBlock with
> > + * the information needed to perform the actual RDMA.
> > + */
> > +typedef struct QEMU_PACKED RDMARemoteBlock {
> > +    uint64_t remote_host_addr;
> > +    uint64_t offset;
> > +    uint64_t length;
> > +    uint32_t remote_rkey;
> > +    uint32_t padding;
> > +} QEMU_PACKED RDMARemoteBlock;
> I assume from the PACKED annotations (do we really need both,
> incidentally) that this is shared with either the guest or
> with another instance of QEMU. Are there definitely no
> endianness issues to deal with here?

I have ntohl()/htonl() on the protocol headers, but I did not
add them for the data portions of the protocol.


Is endianess for the data a big issue when you are assume the migration
is happening across identical CPU architectures?


> > +                DDPRINTF("Ungegistration request (%d): "
> Typo.

I have ntohl()/htonl() on the protocol headers, but I did not
add them for the data portions of the protocol.


> > --- a/migration.c
> > +++ b/migration.c
> > @@ -78,6 +78,10 @@ void qemu_start_incoming_migration(const char *uri, Error 
> > **errp)
> >
> >       if (strstart(uri, "tcp:", &p))
> >           tcp_start_incoming_migration(p, errp);
> > +#ifdef CONFIG_RDMA
> > +    else if (strstart(uri, "x-rdma:", &p))
> > +        rdma_start_incoming_migration(p, errp);
> > +#endif
> >   #if !defined(WIN32)
> >       else if (strstart(uri, "exec:", &p))
> >           exec_start_incoming_migration(p, errp);
> > @@ -407,6 +411,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >
> >       if (strstart(uri, "tcp:", &p)) {
> >           tcp_start_outgoing_migration(s, p, &local_err);
> > +#ifdef CONFIG_RDMA
> > +    } else if (strstart(uri, "x-rdma:", &p)) {
> > +        rdma_start_outgoing_migration(s, p, &local_err);
> > +#endif
> >   #if !defined(WIN32)
> >       } else if (strstart(uri, "exec:", &p)) {
> >           exec_start_outgoing_migration(s, p, &local_err);
> This code is begging to be refactored into something
> table driven.
>
> -- PMM