From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Ehrhardt Subject: Re: [PATCH 3/3] kvm-s390: streamline memslot handling Date: Mon, 25 May 2009 10:33:28 +0200 Message-ID: <4A1A57D8.4070203@linux.vnet.ibm.com> References: <1242826497-6797-1-git-send-email-ehrhardt@linux.vnet.ibm.com> <1242826497-6797-4-git-send-email-ehrhardt@linux.vnet.ibm.com> <4A195C0E.3000900@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, borntraeger@de.ibm.com, cotte@de.ibm.com, heiko.carstens@de.ibm.com, schwidefsky@de.ibm.com, Marcelo Tosatti To: Avi Kivity Return-path: Received: from mtagate5.de.ibm.com ([195.212.29.154]:55131 "EHLO mtagate5.de.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753204AbZEYIeq (ORCPT ); Mon, 25 May 2009 04:34:46 -0400 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate5.de.ibm.com (8.14.3/8.13.8) with ESMTP id n4P8XcNx397828 for ; Mon, 25 May 2009 08:33:38 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 n4P8Xc9K2232406 for ; Mon, 25 May 2009 10:33:38 +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 n4P8XcXe005354 for ; Mon, 25 May 2009 10:33:38 +0200 In-Reply-To: <4A195C0E.3000900@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Avi Kivity wrote: > ehrhardt@linux.vnet.ibm.com wrote: >> From: Christian Ehrhardt >> >> This patch relocates the variables kvm-s390 uses to track guest mem=20 >> addr/size. >> As discussed dropping the variables at struct kvm_arch level allows=20 >> to use the >> common vcpu->request based mechanism to reload guest memory if e.g.=20 >> changes >> via set_memory_region. >> The kick mechanism introduced in this series is used to ensure=20 >> running vcpus >> leave guest state to catch the update. >> >> >> =20 >> rerun_vcpu: >> + if (vcpu->requests) >> + if (test_and_clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests)= ) >> + kvm_s390_vcpu_set_mem(vcpu); >> + >> /* verify, that memory has been registered */ >> - if (!vcpu->kvm->arch.guest_memsize) { >> + if (!vcpu->arch.sie_block->gmslm) { >> vcpu_put(vcpu); >> + VCPU_EVENT(vcpu, 3, "%s", "no memory registered to run vcpu= "); >> return -EINVAL; >> } > > > x86 uses a double check: first we check vcpu->requests outside atomic= =20 > context, then we enter the critical section and check again for=20 > signals and vcpu->requests. > > This allows us (a) to do naughty things in vcpu->requests handlers,=20 > (b) keep the critical section short. > > Does this apply here? The patch already keeps the critical inner loop clear of extra code. The check for vcpu->requests I added is only reached by either a=20 heavyweight (userspace) exit/reentry or the explicit kickout of a vcpu=20 to this label. Therefore weit fulfills a+b as you mentioned them above.= =20 Additionally the s390 reload is very rare as well as fast, therefore it= =20 would not even be an issue. >> - /* update sie control blocks, and unlock all vcpus */ >> + /* request update of sie control block for all available vcpus = */ >> for (i =3D 0; i < KVM_MAX_VCPUS; ++i) { >> if (kvm->vcpus[i]) { >> - kvm->vcpus[i]->arch.sie_block->gmsor =3D >> - kvm->arch.guest_origin; >> - kvm->vcpus[i]->arch.sie_block->gmslm =3D >> - kvm->arch.guest_memsize + >> - kvm->arch.guest_origin + >> - VIRTIODESCSPACE - 1ul; >> - mutex_unlock(&kvm->vcpus[i]->mutex); >> + set_bit(KVM_REQ_MMU_RELOAD, &kvm->vcpus[i]->requests); >> + kvm_s390_inject_sigp_stop(kvm->vcpus[i], >> + ACTION_RELOADVCPU_ON_STOP); >> } >> } >> =20 > > There already exists a loop which does this, see=20 > make_all_cpus_request(). It uses an IPI (Marcelo, can't it use the=20 > reschedule interrupt?). It has a couple of optimizations -- if the=20 > request is already set, it skips the IPI, and it avoids the IPI for=20 > vcpus out of guest mode. Maybe it could fit s390 too. I assume that the IPI on x86 is a implicit consequence of the=20 smp_call_function_many function, but I think this doesn't work that way= =20 for us. The kick implied by that call would be recieved, but not reach=20 the code the checke vcpu->request. I could add that behaviour, but that= =20 could make our normal interrupt handling much slower. Therefore I don't= =20 want to call that function, but on the other hand I like the "skip if=20 the request is already set" functionality and think about adding that i= n=20 my loop. --=20 Gr=FCsse / regards, Christian Ehrhardt IBM Linux Technology Center, Open Virtualization=20