From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UPuEv-0007i9-31 for qemu-devel@nongnu.org; Wed, 10 Apr 2013 08:35:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UPuEo-0004jx-FO for qemu-devel@nongnu.org; Wed, 10 Apr 2013 08:35:01 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:55398) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UPuEo-0004jl-BT for qemu-devel@nongnu.org; Wed, 10 Apr 2013 08:34:54 -0400 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 10 Apr 2013 08:34:53 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id A014B6E8048 for ; Wed, 10 Apr 2013 08:34:46 -0400 (EDT) Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3ACYmni326770 for ; Wed, 10 Apr 2013 08:34:49 -0400 Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3ACYmww012007 for ; Wed, 10 Apr 2013 08:34:48 -0400 Message-ID: <51655C62.2030508@linux.vnet.ibm.com> Date: Wed, 10 Apr 2013 08:34:42 -0400 From: "Michael R. Hines" MIME-Version: 1.0 References: <1365568180-19593-1-git-send-email-mrhines@linux.vnet.ibm.com> <1365568180-19593-5-git-send-email-mrhines@linux.vnet.ibm.com> <516519CA.4090809@redhat.com> In-Reply-To: <516519CA.4090809@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH RDMA support v6: 4/7] Introduce two new capabilities 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 On 04/10/2013 03:50 AM, Paolo Bonzini wrote: > Il 10/04/2013 06:29, mrhines@linux.vnet.ibm.com ha scritto: >> From: "Michael R. Hines" >> >> RDMA performs very slowly with zero-page checking. >> Without the ability to disable it, RDMA throughput and >> latency promises and high performance links cannot be >> fully realized. >> >> On the other hand, dynamic page registration support is also >> included in the RDMA protocol. This second capability also >> cannot be fully realized without the ability to enable zero >> page scanning. >> >> So, we have two new capabilities which work together: >> >> 1. migrate_set_capability check_for_zero on|off (default on) >> 2. migrate_set_capability chunk_register_destination on|off (default off) >> >> Signed-off-by: Michael R. Hines > Michael, please listen to _all_ review comments. > > 1) I asked you to place check_for_zero in a separate patch. > > 2) Again, patch 3 cannot compile without this one. The code should > compile after each patch, with and without --enable-rdma. My apologies - I misunderstood the request. I am indeed addressing every comment. When you said separate, I thought you meant a different patch series altpgether. This is my first time, so the meaning of "separate" was not clear =) And yes, patch 3 does in fact compile both with and without --enable-rdma. >> --- >> include/migration/qemu-file.h | 15 +++++++++++++++ >> migration.c | 33 +++++++++++++++++++++++++++++++-- >> qapi-schema.json | 2 +- >> 3 files changed, 47 insertions(+), 3 deletions(-) >> >> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h >> index 623c434..b6f3256 100644 >> --- a/include/migration/qemu-file.h >> +++ b/include/migration/qemu-file.h >> @@ -57,12 +57,15 @@ typedef int (QEMUFileGetFD)(void *opaque); >> typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov, >> int iovcnt); >> >> +typedef struct QEMURamControlOps QEMURamControlOps; >> + >> typedef struct QEMUFileOps { >> QEMUFilePutBufferFunc *put_buffer; >> QEMUFileGetBufferFunc *get_buffer; >> QEMUFileCloseFunc *close; >> QEMUFileGetFD *get_fd; >> QEMUFileWritevBufferFunc *writev_buffer; >> + const QEMURamControlOps *ram_control; > Why a separate member? You can put them directly here. Acknowledged. >> } QEMUFileOps; >> >> QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops); >> @@ -80,6 +83,18 @@ void qemu_put_byte(QEMUFile *f, int v); >> * The buffer should be available till it is sent asynchronously. >> */ >> void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size); >> +void qemu_file_set_error(QEMUFile *f, int ret); >> + >> +void qemu_rdma_cleanup(void *opaque); >> +int qemu_rdma_close(void *opaque); >> +int qemu_rdma_get_fd(void *opaque); >> +int qemu_rdma_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size); >> +int qemu_rdma_put_buffer(void *opaque, const uint8_t *buf, >> + int64_t pos, int size); >> +bool qemu_file_mode_is_not_valid(const char * mode); >> + >> +extern const QEMUFileOps rdma_read_ops; >> +extern const QEMUFileOps rdma_write_ops; > Please put these in migration-rdma.c. You do not need them here. > Acknowledged. >> static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v) >> { >> diff --git a/migration.c b/migration.c >> index 3b4b467..875cee3 100644 >> --- a/migration.c >> +++ b/migration.c >> @@ -66,6 +66,7 @@ MigrationState *migrate_get_current(void) >> .state = MIG_STATE_SETUP, >> .bandwidth_limit = MAX_THROTTLE, >> .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE, >> + .enabled_capabilities[MIGRATION_CAPABILITY_CHECK_FOR_ZERO] = true, >> }; >> >> return ¤t_migration; >> @@ -77,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, "rdma:", &p)) >> + rdma_start_incoming_migration(p, errp); >> +#endif >> #if !defined(WIN32) >> else if (strstart(uri, "exec:", &p)) >> exec_start_incoming_migration(p, errp); >> @@ -120,8 +125,10 @@ void process_incoming_migration(QEMUFile *f) >> Coroutine *co = qemu_coroutine_create(process_incoming_migration_co); >> int fd = qemu_get_fd(f); >> >> - assert(fd != -1); >> - qemu_set_nonblock(fd); >> + if(fd != -2) { /* rdma returns -2 */ > Please do not invent a special return value, this will not be maintainable. > > Just sacrifice the assert for now, as we had agreed in the past. You flip-flopped on me =) You said conditionalize it, then make a separate patch, then delete it altogether =) But I'm fine with delete =)