Re: [PATCH] KVM: stats: add stats to detect if vcpu is currently halted

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Wed, Aug 18, 2021, Jing Zhang wrote:
> > > Original-by: Cannon Matthews <cannonmatthews@xxxxxxxxxx>
> > > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > > ---
> > >  include/linux/kvm_host.h  | 4 +++-
> > >  include/linux/kvm_types.h | 2 ++
> > >  virt/kvm/kvm_main.c       | 2 ++
> > >  3 files changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index d447b21cdd73..23d2e19af3ce 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -1459,7 +1459,9 @@ struct _kvm_stats_desc {
> > >       STATS_DESC_LOGHIST_TIME_NSEC(VCPU_GENERIC, halt_poll_fail_hist,        \
> > >                       HALT_POLL_HIST_COUNT),                                 \
> > >       STATS_DESC_LOGHIST_TIME_NSEC(VCPU_GENERIC, halt_wait_hist,             \
> > > -                     HALT_POLL_HIST_COUNT)
> > > +                     HALT_POLL_HIST_COUNT),                                 \
> > > +     STATS_DESC_COUNTER(VCPU_GENERIC, halt_block_starts),                   \
> > > +     STATS_DESC_COUNTER(VCPU_GENERIC, halt_block_ends)
> >
> > Why two counters?  It's per-vCPU, can't this just be a "blocked" flag or so?  I
> > get that all the other stats use "halt", but that's technically wrong as KVM will
> > block vCPUs that are not runnable for other reason, e.g. because they're in WFS
> > on x86.
> The two counters are used to determine the reason why vCPU is not
> running. If the halt_block_ends is one less than halt_block_starts,
> then we know the vCPU is explicitly blocked by KVM. Otherwise, we know
> there might be something wrong with the vCPU. Does this make sense?
> Will rename from "halt_block_*" to "vcpu_block_*".

Yeah, but why not do the below?  "halt_block_starts - halt_block_ends" can only
ever be '0' or '1'; if it's anything else, KVM done messed up, e.g. failed to
bump halt_block_ends when unblocking.

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3e67c93ca403..aa98dec727d0 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3208,6 +3208,8 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)

        kvm_arch_vcpu_blocking(vcpu);

+       vcpu->stat.generic.blocked = 1;
+
        start = cur = poll_end = ktime_get();
        if (vcpu->halt_poll_ns && !kvm_arch_no_poll(vcpu)) {
                ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns);
@@ -3258,6 +3260,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
                                ktime_to_ns(cur) - ktime_to_ns(poll_end));
        }
 out:
+       vcpu->stat.generic.blocked = 0;
        kvm_arch_vcpu_unblocking(vcpu);
        block_ns = ktime_to_ns(cur) - ktime_to_ns(start);