From: Sean Christopherson <seanjc@google•com>
To: Oliver Upton <oliver.upton@linux•dev>
Cc: kvm@vger•kernel.org, linux-kernel@vger•kernel.org,
linux-riscv@lists•infradead.org,
Claudio Imbrenda <imbrenda@linux•ibm.com>,
Janosch Frank <frankja@linux•ibm.com>,
Marc Zyngier <maz@kernel•org>,
Huacai Chen <chenhuacai@kernel•org>,
Christian Borntraeger <borntraeger@linux•ibm.com>,
Albert Ou <aou@eecs•berkeley.edu>, Bibo Mao <maobibo@loongson•cn>,
loongarch@lists•linux.dev,
Paul Walmsley <paul.walmsley@sifive•com>,
kvmarm@lists•linux.dev, linux-arm-kernel@lists•infradead.org,
Anup Patel <anup@brainfault•org>,
linux-mips@vger•kernel.org, Palmer Dabbelt <palmer@dabbelt•com>,
kvm-riscv@lists•infradead.org,
Paolo Bonzini <pbonzini@redhat•com>,
Tianrui Zhao <zhaotianrui@loongson•cn>,
linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH 0/4] KVM: Fold kvm_arch_sched_in() into kvm_arch_vcpu_load()
Date: Wed, 1 May 2024 07:28:21 -0700 [thread overview]
Message-ID: <ZjJRhQhX_12eBvY-@google.com> (raw)
In-Reply-To: <ZjGMn5tlq8edKZYv@linux.dev>
On Wed, May 01, 2024, Oliver Upton wrote:
> On Tue, Apr 30, 2024 at 12:31:53PM -0700, Sean Christopherson wrote:
> > Drop kvm_arch_sched_in() and instead pass a @sched_in boolean to
> > kvm_arch_vcpu_load().
> >
> > While fiddling with an idea for optimizing state management on AMD CPUs,
> > I wanted to skip re-saving certain host state when a vCPU is scheduled back
> > in, as the state (theoretically) shouldn't change for the task while it's
> > scheduled out. Actually doing that was annoying and unnecessarily brittle
> > due to having a separate API for the kvm_sched_in() case (the state save
> > needed to be in kvm_arch_vcpu_load() for the common path).
> >
> > E.g. I could have set a "temporary"-ish flag somewhere in kvm_vcpu, but (a)
> > that's gross and (b) it would rely on the arbitrary ordering between
> > sched_in() and vcpu_load() staying the same.
>
> Another option would be to change the rules around kvm_arch_sched_in()
> where the callee is expected to load the vCPU context.
>
> The default implementation could just call kvm_arch_vcpu_load() directly
> and the x86 implementation can order things the way it wants before
> kvm_arch_vcpu_load().
>
> I say this because ...
>
> > The only real downside I see is that arm64 and riscv end up having to pass
> > "false" for their direct usage of kvm_arch_vcpu_load(), and passing boolean
> > literals isn't ideal. But that can be solved by adding an inner helper that
> > omits the @sched_in param (I almost added a patch to do that, but I couldn't
> > convince myself it was necessary).
>
> Needing to pass @sched_in for other usage of kvm_arch_vcpu_load() hurts
> readability, especially when no other architecture besides x86 cares
> about it.
Yeah, that bothers me too.
I tried your suggestion of having x86's kvm_arch_sched_in() do kvm_arch_vcpu_load(),
and even with an added kvm_arch_sched_out() to provide symmetry, the x86 code is
kludgy, and even the common code is a bit confusing as it's not super obvious
that kvm_sched_{in,out}() is really just kvm_arch_vcpu_{load,put}().
Staring a bit more at the vCPU flags we have, adding a "bool scheduled_out" isn't
terribly gross if it's done in common code and persists across load() and put(),
i.e. isn't so blatantly a temporary field. And because it's easy, it could be
set with WRITE_ONCE() so that if it can be read cross-task if there's ever a
reason to do so.
The x86 code ends up being less ugly, and adding future arch/vendor code for
sched_in() *or* sched_out() requires minimal churn, e.g. arch code doesn't need
to override kvm_arch_sched_in().
The only weird part is that vcpu->preempted and vcpu->ready have slightly
different behavior, as they are cleared before kvm_arch_vcpu_load(). But the
weirdness is really with those flags no having symmetry, not with scheduled_out
itself.
Thoughts?
static void kvm_sched_in(struct preempt_notifier *pn, int cpu)
{
struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn);
WRITE_ONCE(vcpu->preempted, false);
WRITE_ONCE(vcpu->ready, false);
__this_cpu_write(kvm_running_vcpu, vcpu);
kvm_arch_vcpu_load(vcpu, cpu);
WRITE_ONCE(vcpu->scheduled_out, false);
}
static void kvm_sched_out(struct preempt_notifier *pn,
struct task_struct *next)
{
struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn);
WRITE_ONCE(vcpu->scheduled_out, true);
if (current->on_rq) {
WRITE_ONCE(vcpu->preempted, true);
WRITE_ONCE(vcpu->ready, true);
}
kvm_arch_vcpu_put(vcpu);
__this_cpu_write(kvm_running_vcpu, NULL);
}
next prev parent reply other threads:[~2024-05-01 14:29 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 19:31 [PATCH 0/4] KVM: Fold kvm_arch_sched_in() into kvm_arch_vcpu_load() Sean Christopherson
2024-04-30 19:31 ` [PATCH 1/4] KVM: Plumb in a @sched_in flag to kvm_arch_vcpu_load() Sean Christopherson
2024-04-30 19:31 ` [PATCH 2/4] KVM: VMX: Move PLE grow/shrink helpers above vmx_vcpu_load() Sean Christopherson
2024-04-30 19:31 ` [PATCH 3/4] KVM: x86: Fold kvm_arch_sched_in() into kvm_arch_vcpu_load() Sean Christopherson
2024-04-30 19:31 ` [PATCH 4/4] KVM: Delete the now unused kvm_arch_sched_in() Sean Christopherson
2024-05-01 0:28 ` [PATCH 0/4] KVM: Fold kvm_arch_sched_in() into kvm_arch_vcpu_load() Oliver Upton
2024-05-01 14:28 ` Sean Christopherson [this message]
2024-05-01 18:01 ` Oliver Upton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjJRhQhX_12eBvY-@google.com \
--to=seanjc@google$(echo .)com \
--cc=anup@brainfault$(echo .)org \
--cc=aou@eecs$(echo .)berkeley.edu \
--cc=borntraeger@linux$(echo .)ibm.com \
--cc=chenhuacai@kernel$(echo .)org \
--cc=frankja@linux$(echo .)ibm.com \
--cc=imbrenda@linux$(echo .)ibm.com \
--cc=kvm-riscv@lists$(echo .)infradead.org \
--cc=kvm@vger$(echo .)kernel.org \
--cc=kvmarm@lists$(echo .)linux.dev \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-mips@vger$(echo .)kernel.org \
--cc=linux-riscv@lists$(echo .)infradead.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=loongarch@lists$(echo .)linux.dev \
--cc=maobibo@loongson$(echo .)cn \
--cc=maz@kernel$(echo .)org \
--cc=oliver.upton@linux$(echo .)dev \
--cc=palmer@dabbelt$(echo .)com \
--cc=paul.walmsley@sifive$(echo .)com \
--cc=pbonzini@redhat$(echo .)com \
--cc=zhaotianrui@loongson$(echo .)cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox