Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <94b5c78d-3878-1a6c-ab53-37daf3d6eb9c@redhat.com>
Date:   Wed, 2 Mar 2022 22:22:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3 22/28] KVM: x86/mmu: Zap defunct roots via asynchronous
 worker
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, David Matlack <dmatlack@google.com>,
        Ben Gardon <bgardon@google.com>,
        Mingwei Zhang <mizhang@google.com>
References: <20220226001546.360188-1-seanjc@google.com>
 <20220226001546.360188-23-seanjc@google.com>
 <b9270432-4ee8-be8e-8aa1-4b09992f82b8@redhat.com>
 <Yh+xA31FrfGoxXLB@google.com>
 <f4189f26-eff9-9fd0-40a1-69ac7759dedf@redhat.com>
 <Yh/GoUPxMRyFqFc5@google.com>
 <442859af-6454-b15e-b2ad-0fc7c4e22909@redhat.com>
 <Yh/X3m1rjYaY2s0z@google.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <Yh/X3m1rjYaY2s0z@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/2/22 21:47, Sean Christopherson wrote:
> On Wed, Mar 02, 2022, Paolo Bonzini wrote:
>> For now let's do it the simple but ugly way.  Keeping
>> next_invalidated_root() does not make things worse than the status quo, and
>> further work will be easier to review if it's kept separate from this
>> already-complex work.
> 
> Oof, that's not gonna work.  My approach here in v3 doesn't work either.  I finally
> remembered why I had the dedicated tdp_mmu_defunct_root flag and thus the smp_mb_*()
> dance.
> 
> kvm_tdp_mmu_zap_invalidated_roots() assumes that it was gifted a reference to
> _all_ invalid roots by kvm_tdp_mmu_invalidate_all_roots().  This works in the
> current code base only because kvm->slots_lock is held for the entire duration,
> i.e. roots can't become invalid between the end of kvm_tdp_mmu_invalidate_all_roots()
> and the end of kvm_tdp_mmu_zap_invalidated_roots().

Yeah, of course that doesn't work if kvm_tdp_mmu_zap_invalidated_roots() 
calls kvm_tdp_mmu_put_root() and the worker also does the same 
kvm_tdp_mmu_put_root().

But, it seems so me that we were so close to something that works and is 
elegant with the worker idea.  It does avoid the possibility of two 
"puts", because the work item is created on the valid->invalid 
transition.  What do you think of having a separate workqueue for each 
struct kvm, so that kvm_tdp_mmu_zap_invalidated_roots() can be replaced 
with a flush?  I can probably do it next Friday.

Paolo

> 
> Marking a root invalid in kvm_tdp_mmu_put_root() breaks that assumption, e.g. if a
> new root is created and then dropped, it will be marked invalid but the "fast zap"
> will not have a reference.  The "defunct" flag prevents this scenario by allowing
> the "fast zap" path to identify invalid roots for which it did not take a reference.
> By virtue of holding a reference, "fast zap" also guarantees that the roots it needs
> to invalidate and put can't become defunct.
> 
> My preference would be to either go back to a variant of v2, or to implement my
> "second list" idea.
> 
> I also need to figure out why I didn't encounter errors in v3, because I distinctly
> remember underflowing the refcount before adding the defunct flag...