From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsYeF-0006SI-6W for qemu-devel@nongnu.org; Fri, 28 Jun 2013 09:23:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UsYe3-00061n-GA for qemu-devel@nongnu.org; Fri, 28 Jun 2013 09:23:35 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:44986) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsYe3-00061Z-7W for qemu-devel@nongnu.org; Fri, 28 Jun 2013 09:23:23 -0400 Received: from /spool/local by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Jun 2013 07:23:22 -0600 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id B0A4BC90026 for ; Fri, 28 Jun 2013 09:23:17 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r5SDNIaK317834 for ; Fri, 28 Jun 2013 09:23:18 -0400 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r5SDNHAc025408 for ; Fri, 28 Jun 2013 10:23:18 -0300 Message-ID: <51CD8E44.2090201@linux.vnet.ibm.com> Date: Fri, 28 Jun 2013 09:23:16 -0400 From: "Michael R. Hines" MIME-Version: 1.0 References: <1372373098-5877-1-git-send-email-mrhines@linux.vnet.ibm.com> <1372373098-5877-4-git-send-email-mrhines@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/6] rdma: core logic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, owasserm@redhat.com, abali@us.ibm.com, mrhines@us.ibm.com, gokul@us.ibm.com, pbonzini@redhat.com, chegu_vinod@hp.com, knoel@redhat.com On 06/27/2013 07:16 PM, Peter Maydell wrote: > On 27 June 2013 23:44, wrote: >> +if test "$rdma" != "no" ; then >> + cat > $TMPC <> +#include >> +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 >