Re: [PATCH] KVM: x86: switch hugepage recovery thread to vhost_task

[Paolo Bonzini]

On Wed, Jan 15, 2025 at 5:51 PM Keith Busch <kbusch@xxxxxxxxxx> wrote:
>
> On Tue, Jan 14, 2025 at 07:06:20PM -0800, Sean Christopherson wrote:
> > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > > index 2401606db2604..422b6b06de4fe 100644
> > > > --- a/arch/x86/kvm/mmu/mmu.c
> > > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > > @@ -7415,6 +7415,8 @@ int kvm_mmu_post_init_vm(struct kvm *kvm)
> > > >   {
> > > >           if (nx_hugepage_mitigation_hard_disabled)
> > > >                   return 0;
> > > > + if (kvm->arch.nx_huge_page_recovery_thread)
> > > > +         return 0;
> >
> > ...
> >
> > > >           kvm->arch.nx_huge_page_last = get_jiffies_64();
> > > >           kvm->arch.nx_huge_page_recovery_thread = vhost_task_create(
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index c79a8cc57ba42..263363c46626b 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -11463,6 +11463,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> > > >           struct kvm_run *kvm_run = vcpu->run;
> > > >           int r;
> > > > + r = kvm_mmu_post_init_vm(vcpu->kvm);
> > > > + if (r)
> > > > +         return r;
> >
> > The only lock held at this point is vcpu->mutex, the obvious choices for guarding
> > the per-VM task creation are kvm->lock or kvm->mmu_lock, but we definitely don't
> > want to blindly take either lock in KVM_RUN.
>
> Thanks for the feedback. Would this otherwise be okay if I use a
> different mechanism to ensure the vhost task creation happens only once
> per kvm instance? Or are you suggesting the task creation needs to be
> somewhere other than KVM_RUN?

You can implement something like pthread_once():

#define ONCE_NOT_STARTED 0
#define ONCE_RUNNING     1
#define ONCE_COMPLETED   2

struct once {
        atomic_t state;
        struct mutex lock;
}

static inline void __once_init(struct once *once,
    const char *name, struct lock_class_key *key)
{
        atomic_set(&once->state, ONCE_NOT_STARTED);
        __mutex_init(&once->lock, name, key);
}

#define once_init(once)                                               \
do {                                                                  \
        static struct lock_class_key __key;                           \
                                                                      \
       __once_init((once), #once, &__key);                            \
} while (0)

static inline void call_once(struct once *once, void (*cb)(struct once *))
{
        /* Pairs with atomic_set_release() below.  */
        if (atomic_read_acquire(&once->state) == ONCE_COMPLETED)
                return;

        guard(mutex)(&once->lock);
        WARN_ON(atomic_read(&once->state) == ONCE_RUNNING);
        if (atomic_read(&once->state) != ONCE_NOT_STARTED)
                return;

        atomic_set(&once->state, ONCE_RUNNING);
        cb(once);
        atomic_set_release(&once->state, ONCE_COMPLETED);
}

Where to put it I don't know.  It doesn't belong in
include/linux/once.h.  I'm okay with arch/x86/kvm/call_once.h and just
pull it with #include "call_once.h".

Paolo