From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49486) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USUmW-0000WR-BB for qemu-devel@nongnu.org; Wed, 17 Apr 2013 12:00:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USUmP-0006g0-7g for qemu-devel@nongnu.org; Wed, 17 Apr 2013 12:00:24 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:34886) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USUmP-0006fk-1P for qemu-devel@nongnu.org; Wed, 17 Apr 2013 12:00:17 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 17 Apr 2013 12:00:16 -0400 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id D19336E8044 for ; Wed, 17 Apr 2013 12:00:09 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3HG0COo287106 for ; Wed, 17 Apr 2013 12:00:12 -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 r3HG0A3Q032451 for ; Wed, 17 Apr 2013 13:00:10 -0300 Message-ID: <516EC6F9.4070103@linux.vnet.ibm.com> Date: Wed, 17 Apr 2013 11:59:53 -0400 From: "Michael R. Hines" MIME-Version: 1.0 References: <1366080286-9288-1-git-send-email-mrhines@linux.vnet.ibm.com> <1366080286-9288-6-git-send-email-mrhines@linux.vnet.ibm.com> <516CD851.7050705@redhat.com> <516E1686.60601@linux.vnet.ibm.com> <516E641C.4040600@redhat.com> In-Reply-To: <516E641C.4040600@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic 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/17/2013 04:58 AM, Paolo Bonzini wrote: > > No, I really mean it. No dead code. We included the one useful option > as a capability, and that should be it. Acknowledged =) >> Failure already happens for unknown capabilities. > Maybe I misread the code, but all I saw is: > > + if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) { > + rdma->chunk_register_destination = true; > + } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) { > + /* handle new capability */ > + } > > and no failure. I may well be wrong of course. Sorry I miscommunicated. By "failure" I meant "disagreement". My design allows the two sides to "agree" with each other as I included in the documentation like this: 1. User sets capability on source (or not) 2. If 'true', source transmits our "intent" to activate capability to destination If (destination knows about the capability) then, enable on both sides else disable on both sides 3. If 'false', destination will never see the flag and will never know >>> + goto err_rdma_server_wait; >>> + } >>> + >>> + if (cap.version == RDMA_CONTROL_VERSION_1) { >>> + if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) { >>> + rdma->chunk_register_destination = true; >>> + } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) { >>> + /* handle new capability */ >>> + } >>> As mentioned above, please drop this "else if". But in general, this >>> "if" is useless. Please replace it with an >>> >>> /* We only support one version, and we rejected all >>> * others above. >>> */ >>> assert(cap.version == RDMA_CONTROL_VERSION_CURRENT); >> We don't want to kill QEMU with an assertion, do we? >> Shouldn't we throw the error back to the user? > You already filtered versions < min and > current with a decent error, > so the only reamining version at this point is 1. You know cap.version > == RDMA_CONTROL_VERSION_CURRENT ("we rejected all others above"). So, as I described above, when version 2 comes out, both QEMU instances are able to "agree" only on the minimum capabilities that both sides support and still allow the migration to begin. For that reason, I don't think we should be doing assert() here. We want this to be compatible across multiple QEMU versions. >> Yes, in my next patch, you will see a new version which uses >> that file descriptor by calling 's poll(). > Ok. Then we can improve it to support coroutines, too, so that the > monitor remains responsive on the incoming side. The idea is that if > poll says the file descriptor is not available you do > yield_until_fd_readable(fd) where yield_until_fd_readable is currently > static in qemu-file.c but could be made public and moved to > qemu-coroutine-io.c and include/qemu-common.h: > > typedef struct { > Coroutine *co; > int fd; > } FDYieldUntilData; > > static void fd_coroutine_enter(void *opaque) > { > FDYieldUntilData *data = opaque; > qemu_set_fd_handler(data->fd, NULL, NULL, NULL); > qemu_coroutine_enter(data->co, NULL); > } > > /** > * Yield until a file descriptor becomes readable > * > * Note that this function clobbers the handlers for the file > descriptor. > */ > static void coroutine_fn yield_until_fd_readable(int fd) > { > FDYieldUntilData data; > > assert(qemu_in_coroutine()); > data.co = qemu_coroutine_self(); > data.fd = fd; > qemu_set_fd_handler(fd, fd_coroutine_enter, NULL, &data); > qemu_coroutine_yield(); > } > > Please feel free to include such a patch (separate from this one) in > your next submission. Excellent. I include the patch. Thanks. - Michael