From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52923) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrAPl-0000g5-N7 for qemu-devel@nongnu.org; Mon, 24 Jun 2013 13:18:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UrAPj-0007ED-6O for qemu-devel@nongnu.org; Mon, 24 Jun 2013 13:18:53 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:55828) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrAPj-0007Du-3P for qemu-devel@nongnu.org; Mon, 24 Jun 2013 13:18:51 -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, 24 Jun 2013 13:18:50 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 9D8D6C9003E for ; Mon, 24 Jun 2013 13:18:47 -0400 (EDT) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r5OHImqh329386 for ; Mon, 24 Jun 2013 13:18:48 -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 r5OHIl4b004026 for ; Mon, 24 Jun 2013 13:18:48 -0400 Message-ID: <51C87F77.50805@linux.vnet.ibm.com> Date: Mon, 24 Jun 2013 13:18:47 -0400 From: "Michael R. Hines" MIME-Version: 1.0 References: <1372081590-24506-1-git-send-email-mrhines@linux.vnet.ibm.com> <1372081590-24506-15-git-send-email-mrhines@linux.vnet.ibm.com> <51C84EDD.4000508@redhat.com> <51C84FC3.3060302@linux.vnet.ibm.com> <51C85172.7020400@redhat.com> In-Reply-To: <51C85172.7020400@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v10 14/14, resend] rdma: add setup time accounting to QMP statistics List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini 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, chegu_vinod@hp.com, knoel@redhat.com On 06/24/2013 10:02 AM, Paolo Bonzini wrote: > Il 24/06/2013 15:55, Michael R. Hines ha scritto: >>>> Reviewed-by: Paolo Bonzini >>> Please stop inventing Reviewed-by tags, or I will stop reviewing your >>> patches. >>> >>> Paolo >> Inventing? I don't understand. >> >> I accumulated all of those tags from everybody - copy and pasted >> directly from everyone when they gave them to me.\ >> >> What exactly is wrong here? > I didn't review this patch in particular, or at least it was changed > very heavily since I last reviewed it. > > I certainly haven't reviewed this: OK, so I need finer granularity with the tags. I didn't understand that until now. I apologize for that. >>>> +static void migrate_set_state(MigrationState *s, int old_state, int new_state) >>>> { >>>> - if (__sync_val_compare_and_swap(&s->state, MIG_STATE_ACTIVE, >>>> + if (__sync_val_compare_and_swap(&s->state, old_state, >>>> new_state) == new_state) { > and I wouldn't have approved it in fact. In this shape, the patch > should be split in at least 3 or 4 pieces. > > In v9 I asked to not apply patch 14 of v9 (I don't remember if you had > already the R-b there); in v10 you rewrote patch 14 and I pointed out > that the Reviewed-by tag was coming out of thin air; now you resent the > series and left the wrong tag. That's why I started to be a bit grumpy. > > By now you must have learnt that I want changes outside migration-rdma.c > to be as a) minimal b) separate c) well-described as possible. > > What is _exactly_ the reason why you're changing the state machine? > What is the change exactly, in fact? Patch 14 is very important: Currently QEMU does not timestamp the setup phase. On the other hand: RDMA uses memory pinning, which happens in the setup phase. Unfortunately, since QEMU does not *explicitly* treat the setup phase like a fully-fledged phase, this phase is unknown to the user. As a result, pinning can take a very long time sometimes and it makes the migration command (not the VM, just the command) appear to "hang" for several seconds when in reality, the setup phase is just taking a long time for the pinning. As you recommended previously, since the pinning now happens in the setup phase, we need a patch which make the user aware of *why* the setup phase is taking such a long time. Does that make sense? This means that we cannot transition from MIG_STATE_SETUP => MIG_STATE_ACTIVE until the pinniing has already completed (i.e. *after* qemu_savevm_state_begin()). But the only way to do that is to call MIG_STATE_ACTIVE *inside* the migration thread. - Michael > Paolo >