From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51780) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UPunh-0007FL-LP for qemu-devel@nongnu.org; Wed, 10 Apr 2013 09:11:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UPund-00007V-64 for qemu-devel@nongnu.org; Wed, 10 Apr 2013 09:10:57 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:42053) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UPunc-00007G-Sn for qemu-devel@nongnu.org; Wed, 10 Apr 2013 09:10:53 -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 ; Wed, 10 Apr 2013 07:10:51 -0600 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id CFA4638C805C for ; Wed, 10 Apr 2013 09:10:47 -0400 (EDT) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3ADAkSK53215344 for ; Wed, 10 Apr 2013 09:10:47 -0400 Received: from d01av05.pok.ibm.com (loopback [127.0.0.1]) by d01av05.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3ADAkKo032230 for ; Wed, 10 Apr 2013 09:10:46 -0400 Message-ID: <516564D6.7040006@linux.vnet.ibm.com> Date: Wed, 10 Apr 2013 09:10:46 -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> <51655C62.2030508@linux.vnet.ibm.com> <51655DB4.3070600@redhat.com> In-Reply-To: <51655DB4.3070600@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 08:40 AM, Paolo Bonzini wrote: > Il 10/04/2013 14:34, Michael R. Hines ha scritto: >> 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. > How does this: > > +#ifdef CONFIG_RDMA > +const QEMURamControlOps qemu_rdma_write_control = { > + .before_ram_iterate = qemu_ram_registration_start, > + .after_ram_iterate = qemu_rdma_registration_stop, > + .register_ram_iterate = qemu_rdma_registration_handle, > + .save_page = qemu_rdma_save_page, > +}; > > compile (and link) successfully without a definition of > qemu_ram_registration_start, which is in patch 5? (Honest question). > > Similarly, patch 5 cannot link without qemu_ram_foreach_block. > > Anyway, patch 1 does not compile with --enable-rdma. :) Yes, all 7 patches do compile in both cases - that code is conditionalized with CONFIG_RDMA. What exactly is the problem there? I toggled the enable/disable flags several times and recompiled just to be sure. Also about qemu_ram_foreach_block: Why does this depend on patch 5, exactly? qemu_ram_foreach_block is not conditionalized by anything. >> You flip-flopped on me =) >> You said conditionalize it, then make a separate patch, then delete it >> altogether =) > Make qemu_set_nonblock conditional, dropping the assert. > > Do not touch the implementation of qemu_set_nonblock. (Yes, this was a > mess). > > Paolo Acknowledged.