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

["Michael R. Hines" <mrhines@linux.vnet.ibm.com>]

On 06/27/2013 07:16 PM, Peter Maydell wrote:
> On 27 June 2013 23:44,  <mrhines@linux.vnet.ibm.com> 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
>