From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753771AbcBWOQD (ORCPT ); Tue, 23 Feb 2016 09:16:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37929 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753665AbcBWOPc (ORCPT ); Tue, 23 Feb 2016 09:15:32 -0500 Subject: Re: [PATCH v3 05/11] KVM: page track: introduce kvm_page_track_{add,remove}_page To: Xiao Guangrong References: <1455449503-20993-1-git-send-email-guangrong.xiao@linux.intel.com> <1455449503-20993-6-git-send-email-guangrong.xiao@linux.intel.com> <56C6FE6C.5050305@redhat.com> <56CBDD85.6050109@linux.intel.com> Cc: gleb@kernel.org, mtosatti@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, kai.huang@linux.intel.com, jike.song@intel.com From: Paolo Bonzini Message-ID: <56CC6980.4010904@redhat.com> Date: Tue, 23 Feb 2016 15:15:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <56CBDD85.6050109@linux.intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 23 Feb 2016 14:15:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23/02/2016 05:18, Xiao Guangrong wrote: > > > On 02/19/2016 07:37 PM, Paolo Bonzini wrote: >> >> >> On 14/02/2016 12:31, Xiao Guangrong wrote: >>> + /* does tracking count wrap? */ >>> + WARN_ON((count > 0) && (val + count < val)); >> >> This doesn't work, because "val + count" is an int. > > val is 'unsigned short val' and count is 'short', so > 'val + count' is not int... Actually, it is. "val" and "count" are both promoted to int, and the result of "val + count" is an int! >>> +void kvm_page_track_add_page(struct kvm *kvm, gfn_t gfn, >>> + enum kvm_page_track_mode mode) >>> +{ >>> + struct kvm_memslots *slots; >>> + struct kvm_memory_slot *slot; >>> + int i; >>> + >>> + for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { >>> + slots = __kvm_memslots(kvm, i); >>> + >>> + slot = __gfn_to_memslot(slots, gfn); >>> + if (!slot) >>> + continue; >>> + >>> + spin_lock(&kvm->mmu_lock); >>> + kvm_slot_page_track_add_page_nolock(kvm, slot, gfn, mode); >>> + spin_unlock(&kvm->mmu_lock); >>> + } >>> +} >> >> I don't think it is right to walk all address spaces. The good news is > > Then we can not track the page in SMM mode, but i think it is not a big > problem as SMM is invisible to OS (it is expected to not hurting OS) and > current shadow page in normal spaces can not reflect the changes happend > in SMM either. So it is okay to only take normal space into account. I think which address space to track depends on the scenario where you're using page tracking. For example, in the shadow case you only track either SMM or non-SMM depending on the CPU's mode. For KVM-GT you probably need to track only non-SMM. >> that you're not using kvm_page_track_{add,remove}_page at all as far as >> I can see, so you can just remove them. > > kvm_page_track_{add,remove}_page, which hides the mmu specifics (e.g. slot, > mmu-lock, etc.) and are exported as APIs for other users, are just the > small wrappers of kvm_slot_page_track_{add,remove}_page_nolock and the > nolock functions are used in the later patch. > > If you think it is not a good time to export these APIs, i am okay to > export _nolock functions only in the next version. Yes, please. >> Also, when you will need it, I think it's better to move the >> spin_lock/spin_unlock pair outside the for loop. With this change, >> perhaps it's better to leave it to the caller completely---but I cannot >> say until I see the caller. > > I will remove page tracking in SMM address space, so this is no loop in > the next version. ;) Instead please just remove the functions completely. Functions without a caller are unnecessary. >> In the meanwhile, please leave out _nolock from the other functions' >> name. > > I just wanted to warn the user that these functions are not safe as they > are not protected by mmu-lock. I will remove these hints if you dislike > them. I think there's several other functions that are not protected by mmu-lock. You can instead add kerneldoc comments and mention the locking requirements there. The common convention in the kernel is to have function take the lock and call __function. In this case however there is no "locked" function yet; if it comes later, we will rename the functions to add "__" in front. Paolo