Hi Vitaly, On 8/17/21 2:53 AM, Vitaly Kuznetsov wrote:
Gavin Shan <gshan@xxxxxxxxxx> 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 <gshan@xxxxxxxxxx> --- 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) { } #endifdiff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.cindex 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.cindex 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; -#endifreturn false;}
Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
