* Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check
@ 2020-02-26 3:20 linmiaohe
0 siblings, 0 replies; 4+ messages in thread
From: linmiaohe @ 2020-02-26 3:20 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
suravee.suthikulpanit, jmattson, joro, tglx, mingo, bp, hpa
Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>linmiaohe <linmiaohe@huawei.com> writes:
>
>> From: Miaohe Lin <linmiaohe@huawei.com>
>>
>> After test_and_set_bit() for kvm->arch.apicv_inhibit_reasons, we will
>> always get false when calling kvm_apicv_activated() because it's sure
>> apicv_inhibit_reasons do not equal to 0.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>> arch/x86/kvm/x86.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index
>> ddcc51b89e2c..fa62dcb0ed0c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8018,8 +8018,7 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>> !kvm_apicv_activated(kvm))
>> return;
>> } else {
>> - if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>> - kvm_apicv_activated(kvm))
>> + if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons))
>> return;
>> }
>
>This seems to be correct in a sense that we are not really protected against concurrent modifications of 'apicv_inhibit_reasons' (like what if 'apicv_inhibit_reasons' gets modified right after we've checked 'kvm_apicv_activated(kvm)').
Yes, there might be a race window. But this looks benign as we recalculate kvm_apicv_activated() when we proceed with KVM_REQ_APICV_UPDATE.
>
>The function, however, still gives a flase impression it is somewhat protected against concurent modifications. Like what are these
>test_and_{set,clear}_bit() for?
Yes, I think so too. And also test_and_{set,clear}_bit() checks wheather the requested bit is {set,clear} to the requested state.
>
>If I'm not mistaken, the logic this function was supposed to implement
>is: change the requested bit to the requested state and, if
>kvm_apicv_activated() changed (we set the first bit or cleared the last), proceed with KVM_REQ_APICV_UPDATE. What if we re-write it like
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 2103101eca78..b97b8ff4a789 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -8027,19 +8027,19 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
> */
> void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit) {
>+ bool apicv_was_activated = kvm_apicv_activated(kvm);
>+
> if (!kvm_x86_ops->check_apicv_inhibit_reasons ||
> !kvm_x86_ops->check_apicv_inhibit_reasons(bit))
> return;
>
>- if (activate) {
>- if (!test_and_clear_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>- !kvm_apicv_activated(kvm))
>- return;
>- } else {
>- if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>- kvm_apicv_activated(kvm))
>- return;
>- }
>+ if (activate)
>+ clear_bit(bit, &kvm->arch.apicv_inhibit_reasons);
>+ else
>+ set_bit(bit, &kvm->arch.apicv_inhibit_reasons);
>+
>+ if (kvm_apicv_activated(kvm) == apicv_was_activated)
>+ return;
>
> trace_kvm_apicv_update_request(activate, bit);
> if (kvm_x86_ops->pre_update_apicv_exec_ctrl)
>
>Is this equal?
>
Looks good. I think this version also improves the readability. Many thanks for your advice and review!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check
2020-02-25 12:43 ` Vitaly Kuznetsov
@ 2020-03-14 11:31 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2020-03-14 11:31 UTC (permalink / raw)
To: Vitaly Kuznetsov, linmiaohe
Cc: kvm, linux-kernel, x86, rkrcmar, sean.j.christopherson,
wanpengli, jmattson, joro, tglx, mingo, bp, hpa
On 25/02/20 13:43, Vitaly Kuznetsov wrote:
> If I'm not mistaken, the logic this function was supposed to implement
> is: change the requested bit to the requested state and, if
> kvm_apicv_activated() changed (we set the first bit or cleared the
> last), proceed with KVM_REQ_APICV_UPDATE. What if we re-write it like
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2103101eca78..b97b8ff4a789 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8027,19 +8027,19 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
> */
> void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> {
> + bool apicv_was_activated = kvm_apicv_activated(kvm);
> +
> if (!kvm_x86_ops->check_apicv_inhibit_reasons ||
> !kvm_x86_ops->check_apicv_inhibit_reasons(bit))
> return;
>
> - if (activate) {
> - if (!test_and_clear_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
> - !kvm_apicv_activated(kvm))
> - return;
> - } else {
> - if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
> - kvm_apicv_activated(kvm))
> - return;
> - }
> + if (activate)
> + clear_bit(bit, &kvm->arch.apicv_inhibit_reasons);
> + else
> + set_bit(bit, &kvm->arch.apicv_inhibit_reasons);
> +
> + if (kvm_apicv_activated(kvm) == apicv_was_activated)
> + return;
Yes, I got to the same conclusion before seeing you message. Another
possibility is to use cmpxchg, which I slightly prefer because if there
are multiple concurrent updates it has some possibilities of avoiding
the atomic operation and consequent cacheline bouncing. I've sent a patch.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check
2020-02-25 2:21 linmiaohe
@ 2020-02-25 12:43 ` Vitaly Kuznetsov
2020-03-14 11:31 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-25 12:43 UTC (permalink / raw)
To: linmiaohe
Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
wanpengli, jmattson, joro, tglx, mingo, bp, hpa
linmiaohe <linmiaohe@huawei.com> writes:
> From: Miaohe Lin <linmiaohe@huawei.com>
>
> After test_and_set_bit() for kvm->arch.apicv_inhibit_reasons, we will
> always get false when calling kvm_apicv_activated() because it's sure
> apicv_inhibit_reasons do not equal to 0.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> arch/x86/kvm/x86.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddcc51b89e2c..fa62dcb0ed0c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8018,8 +8018,7 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> !kvm_apicv_activated(kvm))
> return;
> } else {
> - if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
> - kvm_apicv_activated(kvm))
> + if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons))
> return;
> }
This seems to be correct in a sense that we are not really protected
against concurrent modifications of 'apicv_inhibit_reasons' (like what
if 'apicv_inhibit_reasons' gets modified right after we've checked
'kvm_apicv_activated(kvm)').
The function, however, still gives a flase impression it is somewhat
protected against concurent modifications. Like what are these
test_and_{set,clear}_bit() for?
If I'm not mistaken, the logic this function was supposed to implement
is: change the requested bit to the requested state and, if
kvm_apicv_activated() changed (we set the first bit or cleared the
last), proceed with KVM_REQ_APICV_UPDATE. What if we re-write it like
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2103101eca78..b97b8ff4a789 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8027,19 +8027,19 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
*/
void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
{
+ bool apicv_was_activated = kvm_apicv_activated(kvm);
+
if (!kvm_x86_ops->check_apicv_inhibit_reasons ||
!kvm_x86_ops->check_apicv_inhibit_reasons(bit))
return;
- if (activate) {
- if (!test_and_clear_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
- !kvm_apicv_activated(kvm))
- return;
- } else {
- if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
- kvm_apicv_activated(kvm))
- return;
- }
+ if (activate)
+ clear_bit(bit, &kvm->arch.apicv_inhibit_reasons);
+ else
+ set_bit(bit, &kvm->arch.apicv_inhibit_reasons);
+
+ if (kvm_apicv_activated(kvm) == apicv_was_activated)
+ return;
trace_kvm_apicv_update_request(activate, bit);
if (kvm_x86_ops->pre_update_apicv_exec_ctrl)
Is this equal?
--
Vitaly
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check
@ 2020-02-25 2:21 linmiaohe
2020-02-25 12:43 ` Vitaly Kuznetsov
0 siblings, 1 reply; 4+ messages in thread
From: linmiaohe @ 2020-02-25 2:21 UTC (permalink / raw)
To: pbonzini, rkrcmar, sean.j.christopherson, vkuznets, wanpengli,
jmattson, joro, tglx, mingo, bp, hpa
Cc: linmiaohe, kvm, linux-kernel, x86
From: Miaohe Lin <linmiaohe@huawei.com>
After test_and_set_bit() for kvm->arch.apicv_inhibit_reasons, we will
always get false when calling kvm_apicv_activated() because it's sure
apicv_inhibit_reasons do not equal to 0.
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
arch/x86/kvm/x86.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ddcc51b89e2c..fa62dcb0ed0c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8018,8 +8018,7 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
!kvm_apicv_activated(kvm))
return;
} else {
- if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
- kvm_apicv_activated(kvm))
+ if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons))
return;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-15 3:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 3:20 [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check linmiaohe
-- strict thread matches above, loose matches on Subject: below --
2020-02-25 2:21 linmiaohe
2020-02-25 12:43 ` Vitaly Kuznetsov
2020-03-14 11:31 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).