From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757194AbcBWE02 (ORCPT ); Mon, 22 Feb 2016 23:26:28 -0500 Received: from mga11.intel.com ([192.55.52.93]:53576 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757056AbcBWE00 (ORCPT ); Mon, 22 Feb 2016 23:26:26 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,488,1449561600"; d="scan'208";a="751357301" Subject: Re: [PATCH v3 05/11] KVM: page track: introduce kvm_page_track_{add,remove}_page To: Paolo Bonzini 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> 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: Xiao Guangrong Message-ID: <56CBDD85.6050109@linux.intel.com> Date: Tue, 23 Feb 2016 12:18:13 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56C6FE6C.5050305@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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... > >> + /* the last tracker has already gone? */ >> + WARN_ON((count < 0) && (val < !count)); > > Also, here any underflow should warn. > > You can actually use the fact that val + count is an int like this: > > WARN_ON(val + count < 0 || val + count > USHRT_MAX) > It looks nice, i will change the type of val to int to simplify the code. > and also please return if the warning fires. > Okay. >> +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. > 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. > > 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. ;) > > 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.