This adds inline helper kvm_check_async_pf_completion_queue() to
check if there are pending completion in the queue. The empty stub
is also added on !CONFIG_KVM_ASYNC_PF so that the caller needn't
consider if CONFIG_KVM_ASYNC_PF is enabled.
All checks on the completion queue is done by the newly added inline
function since list_empty() and list_empty_careful() are interchangeable.
Signed-off-by: Gavin Shan <[email protected]>
---
arch/x86/kvm/x86.c | 2 +-
include/linux/kvm_host.h | 10 ++++++++++
virt/kvm/async_pf.c | 10 +++++-----
virt/kvm/kvm_main.c | 4 +---
4 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e5d5c5ed7dd4..7f35d9324b99 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11591,7 +11591,7 @@ static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
{
- if (!list_empty_careful(&vcpu->async_pf.done))
+ if (kvm_check_async_pf_completion_queue(vcpu))
return true;
if (kvm_apic_has_events(vcpu))
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 85b61a456f1c..a5f990f6dc35 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -339,12 +339,22 @@ struct kvm_async_pf {
bool notpresent_injected;
};
+static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
+{
+ return !list_empty_careful(&vcpu->async_pf.done);
+}
+
void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu);
void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu);
bool kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
unsigned long hva, struct kvm_arch_async_pf *arch);
int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu);
#else
+static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
+{
+ return false;
+}
+
static inline void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu) { }
#endif
diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index dd777688d14a..d145a61a046a 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -70,7 +70,7 @@ static void async_pf_execute(struct work_struct *work)
kvm_arch_async_page_present(vcpu, apf);
spin_lock(&vcpu->async_pf.lock);
- first = list_empty(&vcpu->async_pf.done);
+ first = !kvm_check_async_pf_completion_queue(vcpu);
list_add_tail(&apf->link, &vcpu->async_pf.done);
apf->vcpu = NULL;
spin_unlock(&vcpu->async_pf.lock);
@@ -122,7 +122,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
spin_lock(&vcpu->async_pf.lock);
}
- while (!list_empty(&vcpu->async_pf.done)) {
+ while (kvm_check_async_pf_completion_queue(vcpu)) {
struct kvm_async_pf *work =
list_first_entry(&vcpu->async_pf.done,
typeof(*work), link);
@@ -138,7 +138,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
{
struct kvm_async_pf *work;
- while (!list_empty_careful(&vcpu->async_pf.done) &&
+ while (kvm_check_async_pf_completion_queue(vcpu) &&
kvm_arch_can_dequeue_async_page_present(vcpu)) {
spin_lock(&vcpu->async_pf.lock);
work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
@@ -205,7 +205,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
struct kvm_async_pf *work;
bool first;
- if (!list_empty_careful(&vcpu->async_pf.done))
+ if (kvm_check_async_pf_completion_queue(vcpu))
return 0;
work = kmem_cache_zalloc(async_pf_cache, GFP_ATOMIC);
@@ -216,7 +216,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
INIT_LIST_HEAD(&work->queue); /* for list_del to work */
spin_lock(&vcpu->async_pf.lock);
- first = list_empty(&vcpu->async_pf.done);
+ first = !kvm_check_async_pf_completion_queue(vcpu);
list_add_tail(&work->link, &vcpu->async_pf.done);
spin_unlock(&vcpu->async_pf.lock);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b50dbe269f4b..8795503651b1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3282,10 +3282,8 @@ static bool vcpu_dy_runnable(struct kvm_vcpu *vcpu)
if (kvm_arch_dy_runnable(vcpu))
return true;
-#ifdef CONFIG_KVM_ASYNC_PF
- if (!list_empty_careful(&vcpu->async_pf.done))
+ if (kvm_check_async_pf_completion_queue(vcpu))
return true;
-#endif
return false;
}
--
2.23.0
Gavin Shan <[email protected]> writes:
> This adds inline helper kvm_check_async_pf_completion_queue() to
> check if there are pending completion in the queue. The empty stub
> is also added on !CONFIG_KVM_ASYNC_PF so that the caller needn't
> consider if CONFIG_KVM_ASYNC_PF is enabled.
>
> All checks on the completion queue is done by the newly added inline
> function since list_empty() and list_empty_careful() are interchangeable.
>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> arch/x86/kvm/x86.c | 2 +-
> include/linux/kvm_host.h | 10 ++++++++++
> virt/kvm/async_pf.c | 10 +++++-----
> virt/kvm/kvm_main.c | 4 +---
> 4 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e5d5c5ed7dd4..7f35d9324b99 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11591,7 +11591,7 @@ static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
>
> static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
> {
> - if (!list_empty_careful(&vcpu->async_pf.done))
> + if (kvm_check_async_pf_completion_queue(vcpu))
> return true;
>
> if (kvm_apic_has_events(vcpu))
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 85b61a456f1c..a5f990f6dc35 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -339,12 +339,22 @@ struct kvm_async_pf {
> bool notpresent_injected;
> };
>
> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
Nitpicking: When not reading the implementation, I'm not exactly sure
what this function returns as 'check' is too ambiguous ('true' when the
queue is full? when it's empty? when it's not empty? when it was
properly set up?). I'd suggest we go with a more specific:
kvm_async_pf_completion_queue_empty() or something like that instead
(we'll have to invert the logic everywhere then).
Side note: x86 seems to already use a shortened 'apf' instead of
'async_pf' in a number of places (e.g. 'apf_put_user_ready()'), we may
want to either fight this practice or support the rebelion by renaming
all functions from below instead :-)
> +{
> + return !list_empty_careful(&vcpu->async_pf.done);
> +}
> +
> void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu);
> void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu);
> bool kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> unsigned long hva, struct kvm_arch_async_pf *arch);
> int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu);
> #else
> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
> +
> static inline void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu) { }
> #endif
>
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index dd777688d14a..d145a61a046a 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -70,7 +70,7 @@ static void async_pf_execute(struct work_struct *work)
> kvm_arch_async_page_present(vcpu, apf);
>
> spin_lock(&vcpu->async_pf.lock);
> - first = list_empty(&vcpu->async_pf.done);
> + first = !kvm_check_async_pf_completion_queue(vcpu);
> list_add_tail(&apf->link, &vcpu->async_pf.done);
> apf->vcpu = NULL;
> spin_unlock(&vcpu->async_pf.lock);
> @@ -122,7 +122,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
> spin_lock(&vcpu->async_pf.lock);
> }
>
> - while (!list_empty(&vcpu->async_pf.done)) {
> + while (kvm_check_async_pf_completion_queue(vcpu)) {
> struct kvm_async_pf *work =
> list_first_entry(&vcpu->async_pf.done,
> typeof(*work), link);
> @@ -138,7 +138,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
> {
> struct kvm_async_pf *work;
>
> - while (!list_empty_careful(&vcpu->async_pf.done) &&
> + while (kvm_check_async_pf_completion_queue(vcpu) &&
> kvm_arch_can_dequeue_async_page_present(vcpu)) {
> spin_lock(&vcpu->async_pf.lock);
> work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
> @@ -205,7 +205,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
> struct kvm_async_pf *work;
> bool first;
>
> - if (!list_empty_careful(&vcpu->async_pf.done))
> + if (kvm_check_async_pf_completion_queue(vcpu))
> return 0;
>
> work = kmem_cache_zalloc(async_pf_cache, GFP_ATOMIC);
> @@ -216,7 +216,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
> INIT_LIST_HEAD(&work->queue); /* for list_del to work */
>
> spin_lock(&vcpu->async_pf.lock);
> - first = list_empty(&vcpu->async_pf.done);
> + first = !kvm_check_async_pf_completion_queue(vcpu);
> list_add_tail(&work->link, &vcpu->async_pf.done);
> spin_unlock(&vcpu->async_pf.lock);
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b50dbe269f4b..8795503651b1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3282,10 +3282,8 @@ static bool vcpu_dy_runnable(struct kvm_vcpu *vcpu)
> if (kvm_arch_dy_runnable(vcpu))
> return true;
>
> -#ifdef CONFIG_KVM_ASYNC_PF
> - if (!list_empty_careful(&vcpu->async_pf.done))
> + if (kvm_check_async_pf_completion_queue(vcpu))
> return true;
> -#endif
>
> return false;
> }
--
Vitaly
Hi Vitaly,
On 8/17/21 2:53 AM, Vitaly Kuznetsov wrote:
> Gavin Shan <[email protected]> writes:
>
>> This adds inline helper kvm_check_async_pf_completion_queue() to
>> check if there are pending completion in the queue. The empty stub
>> is also added on !CONFIG_KVM_ASYNC_PF so that the caller needn't
>> consider if CONFIG_KVM_ASYNC_PF is enabled.
>>
>> All checks on the completion queue is done by the newly added inline
>> function since list_empty() and list_empty_careful() are interchangeable.
>>
>> Signed-off-by: Gavin Shan <[email protected]>
>> ---
>> arch/x86/kvm/x86.c | 2 +-
>> include/linux/kvm_host.h | 10 ++++++++++
>> virt/kvm/async_pf.c | 10 +++++-----
>> virt/kvm/kvm_main.c | 4 +---
>> 4 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index e5d5c5ed7dd4..7f35d9324b99 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -11591,7 +11591,7 @@ static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
>>
>> static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
>> {
>> - if (!list_empty_careful(&vcpu->async_pf.done))
>> + if (kvm_check_async_pf_completion_queue(vcpu))
>> return true;
>>
>> if (kvm_apic_has_events(vcpu))
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 85b61a456f1c..a5f990f6dc35 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -339,12 +339,22 @@ struct kvm_async_pf {
>> bool notpresent_injected;
>> };
>>
>> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
>
> Nitpicking: When not reading the implementation, I'm not exactly sure
> what this function returns as 'check' is too ambiguous ('true' when the
> queue is full? when it's empty? when it's not empty? when it was
> properly set up?). I'd suggest we go with a more specific:
>
> kvm_async_pf_completion_queue_empty() or something like that instead
> (we'll have to invert the logic everywhere then).
>
> Side note: x86 seems to already use a shortened 'apf' instead of
> 'async_pf' in a number of places (e.g. 'apf_put_user_ready()'), we may
> want to either fight this practice or support the rebelion by renaming
> all functions from below instead :-)
>
Yeah, I was wandering if the name is ambiguous when I had it. The
reason why I had the name is to be consistent with the existing
one, which is kvm_check_async_pf_completion().
Yes, kvm_async_pf_completion_queue_empty() is much better and I
will include this in next revision.
It's correct that x86 functions include 'apf', but the generic
functions, shared by multiple architectures, use 'async_pf' if
my understanding is correct. So I wouldn't bother to change
the generic function names in this series :)
>> +{
>> + return !list_empty_careful(&vcpu->async_pf.done);
>> +}
>> +
>> void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu);
>> void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu);
>> bool kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>> unsigned long hva, struct kvm_arch_async_pf *arch);
>> int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu);
>> #else
>> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
>> +{
>> + return false;
>> +}
>> +
>> static inline void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu) { }
>> #endif
>>
>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
>> index dd777688d14a..d145a61a046a 100644
>> --- a/virt/kvm/async_pf.c
>> +++ b/virt/kvm/async_pf.c
>> @@ -70,7 +70,7 @@ static void async_pf_execute(struct work_struct *work)
>> kvm_arch_async_page_present(vcpu, apf);
>>
>> spin_lock(&vcpu->async_pf.lock);
>> - first = list_empty(&vcpu->async_pf.done);
>> + first = !kvm_check_async_pf_completion_queue(vcpu);
>> list_add_tail(&apf->link, &vcpu->async_pf.done);
>> apf->vcpu = NULL;
>> spin_unlock(&vcpu->async_pf.lock);
>> @@ -122,7 +122,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
>> spin_lock(&vcpu->async_pf.lock);
>> }
>>
>> - while (!list_empty(&vcpu->async_pf.done)) {
>> + while (kvm_check_async_pf_completion_queue(vcpu)) {
>> struct kvm_async_pf *work =
>> list_first_entry(&vcpu->async_pf.done,
>> typeof(*work), link);
>> @@ -138,7 +138,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
>> {
>> struct kvm_async_pf *work;
>>
>> - while (!list_empty_careful(&vcpu->async_pf.done) &&
>> + while (kvm_check_async_pf_completion_queue(vcpu) &&
>> kvm_arch_can_dequeue_async_page_present(vcpu)) {
>> spin_lock(&vcpu->async_pf.lock);
>> work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
>> @@ -205,7 +205,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
>> struct kvm_async_pf *work;
>> bool first;
>>
>> - if (!list_empty_careful(&vcpu->async_pf.done))
>> + if (kvm_check_async_pf_completion_queue(vcpu))
>> return 0;
>>
>> work = kmem_cache_zalloc(async_pf_cache, GFP_ATOMIC);
>> @@ -216,7 +216,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
>> INIT_LIST_HEAD(&work->queue); /* for list_del to work */
>>
>> spin_lock(&vcpu->async_pf.lock);
>> - first = list_empty(&vcpu->async_pf.done);
>> + first = !kvm_check_async_pf_completion_queue(vcpu);
>> list_add_tail(&work->link, &vcpu->async_pf.done);
>> spin_unlock(&vcpu->async_pf.lock);
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index b50dbe269f4b..8795503651b1 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3282,10 +3282,8 @@ static bool vcpu_dy_runnable(struct kvm_vcpu *vcpu)
>> if (kvm_arch_dy_runnable(vcpu))
>> return true;
>>
>> -#ifdef CONFIG_KVM_ASYNC_PF
>> - if (!list_empty_careful(&vcpu->async_pf.done))
>> + if (kvm_check_async_pf_completion_queue(vcpu))
>> return true;
>> -#endif
>>
>> return false;
>> }
>
Thanks,
Gavin
Hi Gavin,
On 8/15/21 2:59 AM, Gavin Shan wrote:
> This adds inline helper kvm_check_async_pf_completion_queue() to
> check if there are pending completion in the queue. The empty stub
> is also added on !CONFIG_KVM_ASYNC_PF so that the caller needn't
> consider if CONFIG_KVM_ASYNC_PF is enabled.
>
> All checks on the completion queue is done by the newly added inline
> function since list_empty() and list_empty_careful() are interchangeable.
why is it interchangeable?
>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> arch/x86/kvm/x86.c | 2 +-
> include/linux/kvm_host.h | 10 ++++++++++
> virt/kvm/async_pf.c | 10 +++++-----
> virt/kvm/kvm_main.c | 4 +---
> 4 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e5d5c5ed7dd4..7f35d9324b99 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11591,7 +11591,7 @@ static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
>
> static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
> {
> - if (!list_empty_careful(&vcpu->async_pf.done))
> + if (kvm_check_async_pf_completion_queue(vcpu))
> return true;
>
> if (kvm_apic_has_events(vcpu))
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 85b61a456f1c..a5f990f6dc35 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -339,12 +339,22 @@ struct kvm_async_pf {
> bool notpresent_injected;
> };
>
> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
> +{
> + return !list_empty_careful(&vcpu->async_pf.done);
> +}
> +
> void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu);
> void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu);
> bool kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> unsigned long hva, struct kvm_arch_async_pf *arch);
> int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu);
> #else
> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
> +
> static inline void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu) { }
> #endif
>
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index dd777688d14a..d145a61a046a 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -70,7 +70,7 @@ static void async_pf_execute(struct work_struct *work)
> kvm_arch_async_page_present(vcpu, apf);
>
> spin_lock(&vcpu->async_pf.lock);
> - first = list_empty(&vcpu->async_pf.done);
> + first = !kvm_check_async_pf_completion_queue(vcpu);
> list_add_tail(&apf->link, &vcpu->async_pf.done);
> apf->vcpu = NULL;
> spin_unlock(&vcpu->async_pf.lock);
> @@ -122,7 +122,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
> spin_lock(&vcpu->async_pf.lock);
> }
>
> - while (!list_empty(&vcpu->async_pf.done)) {
> + while (kvm_check_async_pf_completion_queue(vcpu)) {
this is replaced by a stronger check. Please can you explain why is it
equivalent?
> struct kvm_async_pf *work =
> list_first_entry(&vcpu->async_pf.done,
> typeof(*work), link);
> @@ -138,7 +138,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
> {
> struct kvm_async_pf *work;
>
> - while (!list_empty_careful(&vcpu->async_pf.done) &&
> + while (kvm_check_async_pf_completion_queue(vcpu) &&
> kvm_arch_can_dequeue_async_page_present(vcpu)) {
> spin_lock(&vcpu->async_pf.lock);
> work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
> @@ -205,7 +205,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
> struct kvm_async_pf *work;
> bool first;
>
> - if (!list_empty_careful(&vcpu->async_pf.done))
> + if (kvm_check_async_pf_completion_queue(vcpu))
> return 0;
>
> work = kmem_cache_zalloc(async_pf_cache, GFP_ATOMIC);
> @@ -216,7 +216,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
> INIT_LIST_HEAD(&work->queue); /* for list_del to work */
>
> spin_lock(&vcpu->async_pf.lock);
> - first = list_empty(&vcpu->async_pf.done);
> + first = !kvm_check_async_pf_completion_queue(vcpu);
> list_add_tail(&work->link, &vcpu->async_pf.done);
> spin_unlock(&vcpu->async_pf.lock);
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b50dbe269f4b..8795503651b1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3282,10 +3282,8 @@ static bool vcpu_dy_runnable(struct kvm_vcpu *vcpu)
> if (kvm_arch_dy_runnable(vcpu))
> return true;
>
> -#ifdef CONFIG_KVM_ASYNC_PF
> - if (!list_empty_careful(&vcpu->async_pf.done))
> + if (kvm_check_async_pf_completion_queue(vcpu))
> return true;
> -#endif
>
> return false;
> }
>
Thanks
Eric
Hi Eric,
On 11/10/21 11:37 PM, Eric Auger wrote:
> On 8/15/21 2:59 AM, Gavin Shan wrote:
>> This adds inline helper kvm_check_async_pf_completion_queue() to
>> check if there are pending completion in the queue. The empty stub
>> is also added on !CONFIG_KVM_ASYNC_PF so that the caller needn't
>> consider if CONFIG_KVM_ASYNC_PF is enabled.
>>
>> All checks on the completion queue is done by the newly added inline
>> function since list_empty() and list_empty_careful() are interchangeable.
> why is it interchangeable?
>
I think the commit log is misleading. list_empty_careful() is more strict
than list_empty(). In this patch, we replace list_empty() with list_empty_careful().
I will correct the commit log in next respin like below:
All checks on the completion queue is done by the newly added inline
function where list_empty_careful() instead of list_empty() is used.
>>
>> Signed-off-by: Gavin Shan <[email protected]>
>> ---
>> arch/x86/kvm/x86.c | 2 +-
>> include/linux/kvm_host.h | 10 ++++++++++
>> virt/kvm/async_pf.c | 10 +++++-----
>> virt/kvm/kvm_main.c | 4 +---
>> 4 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index e5d5c5ed7dd4..7f35d9324b99 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -11591,7 +11591,7 @@ static inline bool kvm_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
>>
>> static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
>> {
>> - if (!list_empty_careful(&vcpu->async_pf.done))
>> + if (kvm_check_async_pf_completion_queue(vcpu))
>> return true;
>>
>> if (kvm_apic_has_events(vcpu))
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 85b61a456f1c..a5f990f6dc35 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -339,12 +339,22 @@ struct kvm_async_pf {
>> bool notpresent_injected;
>> };
>>
>> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
>> +{
>> + return !list_empty_careful(&vcpu->async_pf.done);
>> +}
>> +
>> void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu);
>> void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu);
>> bool kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>> unsigned long hva, struct kvm_arch_async_pf *arch);
>> int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu);
>> #else
>> +static inline bool kvm_check_async_pf_completion_queue(struct kvm_vcpu *vcpu)
>> +{
>> + return false;
>> +}
>> +
>> static inline void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu) { }
>> #endif
>>
>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
>> index dd777688d14a..d145a61a046a 100644
>> --- a/virt/kvm/async_pf.c
>> +++ b/virt/kvm/async_pf.c
>> @@ -70,7 +70,7 @@ static void async_pf_execute(struct work_struct *work)
>> kvm_arch_async_page_present(vcpu, apf);
>>
>> spin_lock(&vcpu->async_pf.lock);
>> - first = list_empty(&vcpu->async_pf.done);
>> + first = !kvm_check_async_pf_completion_queue(vcpu);
>> list_add_tail(&apf->link, &vcpu->async_pf.done);
>> apf->vcpu = NULL;
>> spin_unlock(&vcpu->async_pf.lock);
>> @@ -122,7 +122,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
>> spin_lock(&vcpu->async_pf.lock);
>> }
>>
>> - while (!list_empty(&vcpu->async_pf.done)) {
>> + while (kvm_check_async_pf_completion_queue(vcpu)) {
> this is replaced by a stronger check. Please can you explain why is it
> equivalent?
Access to the completion queue is protected by spinlock. So the additional
check in list_empty_careful() to verify the head's prev/next are modified
on the fly shouldn't happen. It means they're same in our case.
>> struct kvm_async_pf *work =
>> list_first_entry(&vcpu->async_pf.done,
>> typeof(*work), link);
>> @@ -138,7 +138,7 @@ void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
>> {
>> struct kvm_async_pf *work;
>>
>> - while (!list_empty_careful(&vcpu->async_pf.done) &&
>> + while (kvm_check_async_pf_completion_queue(vcpu) &&
>> kvm_arch_can_dequeue_async_page_present(vcpu)) {
>> spin_lock(&vcpu->async_pf.lock);
>> work = list_first_entry(&vcpu->async_pf.done, typeof(*work),
>> @@ -205,7 +205,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
>> struct kvm_async_pf *work;
>> bool first;
>>
>> - if (!list_empty_careful(&vcpu->async_pf.done))
>> + if (kvm_check_async_pf_completion_queue(vcpu))
>> return 0;
>>
>> work = kmem_cache_zalloc(async_pf_cache, GFP_ATOMIC);
>> @@ -216,7 +216,7 @@ int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu)
>> INIT_LIST_HEAD(&work->queue); /* for list_del to work */
>>
>> spin_lock(&vcpu->async_pf.lock);
>> - first = list_empty(&vcpu->async_pf.done);
>> + first = !kvm_check_async_pf_completion_queue(vcpu);
>> list_add_tail(&work->link, &vcpu->async_pf.done);
>> spin_unlock(&vcpu->async_pf.lock);
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index b50dbe269f4b..8795503651b1 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3282,10 +3282,8 @@ static bool vcpu_dy_runnable(struct kvm_vcpu *vcpu)
>> if (kvm_arch_dy_runnable(vcpu))
>> return true;
>>
>> -#ifdef CONFIG_KVM_ASYNC_PF
>> - if (!list_empty_careful(&vcpu->async_pf.done))
>> + if (kvm_check_async_pf_completion_queue(vcpu))
>> return true;
>> -#endif
>>
>> return false;
>> }
>>
Thanks,
Gavin