Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340287 > unrolled thread

Re: [PATCH v3 05/11] KVM: page track: introduce kvm_page_track_{add,remove}_page

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 05/11] KVM: page track: introduce  kvm_page_track_{add,remove}_page Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-02-23 05:30 +0100
    Re: [PATCH v3 05/11] KVM: page track: introduce  kvm_page_track_{add,remove}_page Paolo Bonzini <pbonzini@redhat.com> - 2016-02-23 15:20 +0100

#1340287 — Re: [PATCH v3 05/11] KVM: page track: introduce kvm_page_track_{add,remove}_page

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.

[toc] | [next] | [standalone]

#1340664

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web