Re: [PATCH] KVM: Fix an issue in non-preemptible kernel.

[Paul Burton <paul.burton@mips.com>]

Hi Gary,

On Mon, Sep 02, 2019 at 09:02:32AM +0000, Gary Fu wrote:
> Add a cond_resched() to give the scheduler a chance to run madvise
> task to avoid endless loop here in non-preemptible kernel.

Thanks for the patch!

> Otherwise, the kvm_mmu_notifier would have no chance to be descreased

s/descreased/decreased/
(and in the comment too)

> to 0 by madvise task -> syscall -> zap_page_range ->
> mmu_notifier_invalidate_range_end ->
> __mmu_notifier_invalidate_range_end -> invalidate_range_end ->
> kvm_mmu_notifier_invalidate_range_end, as the madvise task would be
> scheduled when running unmap_single_vma -> unmap_page_range ->
> zap_p4d_range -> zap_pud_range -> zap_pmd_range -> cond_resched which
> is called before mmu_notifier_invalidate_range_end in zap_page_range.

I'm not entirely sure I follow - could you clarify whether the task
invoking the madvise syscall is related to the task using KVM?

> Signed-off-by: Gary Fu <qfu@wavecomp.com>
> ---
>  arch/mips/kvm/mmu.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/mips/kvm/mmu.c b/arch/mips/kvm/mmu.c
> index 97e538a8c1be..e52e63d225f4 100644
> --- a/arch/mips/kvm/mmu.c
> +++ b/arch/mips/kvm/mmu.c
> @@ -746,6 +746,22 @@ static int kvm_mips_map_page(struct kvm_vcpu *vcpu, unsigned long gpa,
>  		 */
>  		spin_unlock(&kvm->mmu_lock);
>  		kvm_release_pfn_clean(pfn);
> +		/*
> +		 * Add a cond_resched() to give the scheduler a chance to run
> +		 * madvise task to avoid endless loop here in non-preemptible
> +		 * kernel.
> +		 * Otherwise, the kvm_mmu_notifier would have no chance to be
> +		 * descreased to 0 by madvise task -> syscall -> zap_page_range
> +		 * -> mmu_notifier_invalidate_range_end ->
> +		 * __mmu_notifier_invalidate_range_end -> invalidate_range_end
> +		 * -> kvm_mmu_notifier_invalidate_range_end, as the madvise task
> +		 * would be scheduled when running unmap_single_vma ->
> +		 * unmap_page_range -> zap_p4d_range -> zap_pud_range ->
> +		 * zap_pmd_range -> cond_resched which is called before
> +		 * mmu_notifier_invalidate_range_end in zap_page_range.
> +		 */
> +		if (need_resched())
> +			cond_resched();

Can we remove the need_resched() check here? cond_resched() already
checks should_resched(0) which tests the same thread-info flag as
need_resched(). So we should be fine to just call cond_resched()
unconditionally.

Thanks,
    Paul

>  		goto retry;
>  	}
>  
> -- 
> 2.17.1
>