From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42709) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHgkf-000687-Vr for qemu-devel@nongnu.org; Mon, 18 Mar 2013 16:33:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UHgkc-0004rT-43 for qemu-devel@nongnu.org; Mon, 18 Mar 2013 16:33:49 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:53992) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHgkb-0004rN-Vn for qemu-devel@nongnu.org; Mon, 18 Mar 2013 16:33:46 -0400 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 18 Mar 2013 16:33:45 -0400 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 97C57C90052 for ; Mon, 18 Mar 2013 16:33:43 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2IKXhaD23593096 for ; Mon, 18 Mar 2013 16:33:43 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2IKXgNO017105 for ; Mon, 18 Mar 2013 17:33:42 -0300 Message-ID: <51477A26.8090600@linux.vnet.ibm.com> Date: Mon, 18 Mar 2013 16:33:42 -0400 From: "Michael R. Hines" MIME-Version: 1.0 References: <1363576743-6146-1-git-send-email-mrhines@linux.vnet.ibm.com> <1363576743-6146-9-git-send-email-mrhines@linux.vnet.ibm.com> <5146D9BF.3030407@redhat.com> In-Reply-To: <5146D9BF.3030407@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH RDMA support v4: 08/10] introduce QEMUFileRDMA List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aliguori@us.ibm.com, mst@redhat.com, qemu-devel@nongnu.org, owasserm@redhat.com, abali@us.ibm.com, mrhines@us.ibm.com, gokul@us.ibm.com Comments inline - tell me what you think....... On 03/18/2013 05:09 AM, Paolo Bonzini wrote: > +typedef struct QEMUFileRDMA > +{ > + void *rdma; > This is an RDMAData *. Please avoid using void * as much as possible. Acknowledged - forgot to move this to rdma.c, so it doesn't have to be void anymore. > */ > + return qemu_rdma_fill(r->rdma, buf, size); > +} > Please move these functions closer to qemu_fopen_rdma (or better, to an > RDMA-specific file altogether). Also, using qemu_rdma_fill introduces a > dependency of savevm.c on migration-rdma.c. There should be no such > dependency; migration-rdma.c should be used only by migration.c. Acknowledged...... > +void * migrate_use_rdma(QEMUFile *f) > +{ > + QEMUFileRDMA *r = f->opaque; > + > + return qemu_rdma_enabled(r->rdma) ? r->rdma : NULL; > You cannot be sure that f->opaque->rdma is a valid pointer. For > example, the first field in a socket QEMUFile's is a file descriptor. > > Instead, you could use a qemu_file_ops_are(const QEMUFile *, const > QEMUFileOps *) function that checks if the file uses the given ops. > Then, migrate_use_rdma can simply check if the QEMUFile is using the > RDMA ops structure. > > With this change, the "enabled" field of RDMAData should go. Great - I like that...... will do.... > +/* > + * Called only for RDMA right now at the end > + * of each live iteration of memory. > + * > + * 'drain' from a QEMUFile perspective means > + * to flush the outbound send buffer > + * (if one exists). > + * > + * For RDMA, this means to make sure we've > + * received completion queue (CQ) messages > + * successfully for all of the RDMA writes > + * that we requested. > + */ > +int qemu_drain(QEMUFile *f) > +{ > + return f->ops->drain ? f->ops->drain(f->opaque) : 0; > +} > Hmm, this is very similar to qemu_fflush, but not quite. :/ > > Why exactly is this needed? Good idea - I'll replace drain with flush once I added the "qemu_file_ops_are(const QEMUFile *, const QEMUFileOps *) " that you recommended...... >> /** Flushes QEMUFile buffer >> * >> */ >> @@ -723,6 +867,8 @@ int qemu_get_byte(QEMUFile *f) >> int64_t qemu_ftell(QEMUFile *f) >> { >> qemu_fflush(f); >> + if(migrate_use_rdma(f)) >> + return delta_norm_mig_bytes_transferred(); > Not needed, and another undesirable dependency (savevm.c -> > arch_init.c). Just update f->pos in save_rdma_page. f->pos isn't good enough because save_rdma_page does not go through QEMUFile directly - only non-live state goes through QEMUFile ....... pc.ram uses direct RDMA writes. As a result, the position pointer does not get updated and the accounting is missed........ - Michael