From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Ehrhardt Subject: Re: [PATCH 3/3] kvm-s390: streamline memslot handling - rebased Date: Mon, 15 Jun 2009 15:47:09 +0200 Message-ID: <4A3650DD.8030905@linux.vnet.ibm.com> References: <1243952771-32428-1-git-send-email-ehrhardt@linux.vnet.ibm.com> <1243952771-32428-4-git-send-email-ehrhardt@linux.vnet.ibm.com> <20090605205312.GA13471@amt.cnet> <4A2CED2E.6030904@linux.vnet.ibm.com> <20090609005632.GA21096@amt.cnet> <4A34E758.6000000@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Marcelo Tosatti , kvm@vger.kernel.org, borntraeger@de.ibm.com, cotte@de.ibm.com, heiko.carstens@de.ibm.com, schwidefsky@de.ibm.com To: Avi Kivity Return-path: Received: from mtagate1.de.ibm.com ([195.212.17.161]:43415 "EHLO mtagate1.de.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752183AbZFONrS (ORCPT ); Mon, 15 Jun 2009 09:47:18 -0400 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate1.de.ibm.com (8.13.1/8.13.1) with ESMTP id n5FDlJ73030480 for ; Mon, 15 Jun 2009 13:47:19 GMT Received: from d12av03.megacenter.de.ibm.com (d12av03.megacenter.de.ibm.com [9.149.165.213]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n5FDlJH23235920 for ; Mon, 15 Jun 2009 15:47:19 +0200 Received: from d12av03.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av03.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n5FDlJgF009554 for ; Mon, 15 Jun 2009 15:47:19 +0200 In-Reply-To: <4A34E758.6000000@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Avi Kivity wrote: > Marcelo Tosatti wrote: >> =20 >>> (continued below) >>> =20 >>>> Anyway, yeah, the set request / wait mechanism you implement here = is >>>> quite similar to the idea mentioned earlier that could be used for= =20 >>>> x86. >>>> >>>> Just get rid of this explicit KVM_REQ_MMU_RELOAD knowledge in >>>> arch-independent code please (if you want to see this merged). >>>> =20 >>> I agree to lift the wait part to other archs later if needed, but=20 >>> as mentioned above I could move this to arch code to the cost of=20 >>> one arch hook more. But as also mentioned it doesn't really hurt. = I=20 >>> agree that it does not need to be KVM_REQ_MMU_RELOAD specific, we=20 >>> could just walk/clear/wake all bits on that vcpu->requests variabl= e. >>> Would that be generic enough in your opinion ? >>> =20 >> >> Don't know. >> >> Avi? >> =20 > > I think I lost the thread here, but I'll try. Isn't the wake part=20 > make_all_vcpus_request() in kvm_main.c? The wait part could be moved= =20 > to a similar generic function. > I'll try to summarize my current thoughts a bit: The rebased patch series brings several fixes and the wait/wakeup=20 mechanism which is in discussion here. As explained before this keeps the new wait implementation in s390 arch= =20 code which allows us to experiment with it. Later if we are happy with=20 it we might (or not) continue the merge and bring this mechanism to=20 make_all_vcpus_request (as on x86 you don't have the issues I try to fi= x=20 here we don't need to hurry bringing that into generic code). Well now to the wait/wakeup which is here in discussion in detail: The s390 arch code can kick a guest, but we don't know implicitly (as=20 x86 does) that the kick succeeded, it might happen somewhen sooner or l= ater. Therefore the code uses wait_on_bit to wait until the vcpu->request bit= =20 is consumed. To ensure cleanup of these waiting threads in some special cases the=20 clear&wake up is also needed at other places than the real bit=20 consumption. One of them is the vcpu release code where we should=20 clear&wakeup all waiters (Marcelo correctly pointed out that we should=20 not be bit specific there, so I just just wake up all in the updated co= de). That was the discussion here: "if it would be ok to clear & wake up=20 all". As wake_up_bit doesn't hurt if there is no waiter it looks like=20 the best solution to to do that in the generic part of vcpu_release. If= =20 ever someone else waits for this or another bit in vcpu->requests, the=20 code ensures all of them are awaken on vcpu release. I send an updated version of the rebased series in a few minutes,=20 containing updates related to what marcelo pointed out. P.S. in case you think we need much more discussions we might try to=20 catch up on irc to save this thread a few cycles :-) --=20 Gr=FCsse / regards, Christian Ehrhardt IBM Linux Technology Center, Open Virtualization=20