From: Jing Zhang <jingzhangos@google.com>
To: Cannon Matthews <cannonmatthews@google.com>
Cc: Sean Christopherson <seanjc@google.com>,
KVM <kvm@vger.kernel.org>, Paolo Bonzini <pbonzini@redhat.com>,
David Matlack <dmatlack@google.com>,
Peter Shier <pshier@google.com>, Oliver Upton <oupton@google.com>
Subject: Re: [PATCH] KVM: stats: add stats to detect if vcpu is currently halted
Date: Thu, 19 Aug 2021 11:09:10 -0700 [thread overview]
Message-ID: <CAAdAUtgcxZXWnqd6XNW7P=SwRQtGm11vrS9-T7rSOVc4xnySyQ@mail.gmail.com> (raw)
In-Reply-To: <CAJfu=UcwHWzqCvTjniAMkGj1mmjw9QCy5a-fGJ2mxTK8EFW7Dg@mail.gmail.com>
Hi Sean,
On Wed, Aug 18, 2021 at 10:01 AM Cannon Matthews
<cannonmatthews@google.com> wrote:
>
> +1 to the rephrasing of the commit message.
>
> On Wed, Aug 18, 2021 at 11:09 AM Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Hi Sean,
> >
> > On Tue, Aug 17, 2021 at 4:46 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Tue, Aug 17, 2021, Jing Zhang wrote:
> > > > Current guest/host/halt stats don't show when we are currently halting
> > >
> > > s/we are/KVM is
> > >
> > > And I would probably reword it to "when KVM is blocking a vCPU in response to
> > > the vCPU activity state, e.g. halt". More on that below.
> > >
> > > > well. If a guest halts for a long period of time they could appear
> > > > pathologically blocked but really it's the opposite there's nothing to
> > > > do.
> > > > Simply count the number of times we enter and leave the kvm_vcpu_block
> > >
> > > s/we/KVM
> > >
> > > In general, it's good practice to avoid pronouns in comments and changelogs as
> > > doing so all but forces using precise, unambiguous language. Things like 'it'
> > > and 'they' are ok when it's abundantly clear what they refer to, but 'we' and 'us'
> > > are best avoided entirely.
> > >
> > > > function per vcpu, if they are unequal, then a VCPU is currently
> > > > halting.
> > > > The existing stats like halt_exits and halt_wakeups don't quite capture
> > > > this. The time spend halted and halt polling is reported eventually, but
> > > > not until we wakeup and resume. If a guest were to indefinitely halt one
> > > > of it's CPUs we would never know, it may simply appear blocked.
> > > ^^^^ ^^
> > > its userspace?
> > >
> > >
> > > The "blocked" terminology is a bit confusing since KVM is explicitly blocking
> > > the vCPU, it just happens to mostly do so in response to a guest HLT. I think
> > > "block" is intended to mean "vCPU task not run", but it would be helpful to make
> > > that clear.
> > >
> > That's a good point. Will reword the comments as you suggested.
> > > > Original-by: Cannon Matthews <cannonmatthews@google.com>
> > > > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > > > ---
> > > > 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 point is to separate "blocked because not runable" and "guest
> explicitly asked to do nothing"
>
> IIRC I originally wrote this patch to help discriminate how we spent
> VCPU thread time,
> in particular into two categories of essentially "doing useful work
> on behalf of the guest" and
> "blocked from doing useful work."
>
> Since a guest has explictly asked for a vcpu to HLT, this is "useful
> work on behalf of the guest"
> even though the thread is "blocked" from running.
>
> This allows answering questions like, are we spending too much time
> waiting on mutexes, or
> long running kernel routines rather than running the vcpu in guest
> mode, or did the guest explictly
> tell us to not doing anything.
>
> So I would suggest keeping the "halt" part of the counters' name, and
> remove the "blocked" part
> rather than the other way around. We explicitly do not want to include
> non-halt blockages in this.
>
> That being said I suppose a boolean could work as well. I think we did
> this because it worked with
> and mirrored existing stats rather than anything particularly nuanced.
> Though there might be some
> eventual consistency sort of concerns with how these stats are updated
> and exported that could make
> monotonic increasing counters more useful.
Any more comments on the naming and the increasing counters?
>
> > 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_*".
> >
> > Thanks,
> > Jing
next prev parent reply other threads:[~2021-08-19 18:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-17 23:05 [PATCH] KVM: stats: add stats to detect if vcpu is currently halted Jing Zhang
2021-08-17 23:46 ` Sean Christopherson
2021-08-18 16:09 ` Jing Zhang
2021-08-18 16:57 ` Sean Christopherson
2021-08-18 17:01 ` Cannon Matthews
2021-08-19 18:09 ` Jing Zhang [this message]
2021-08-19 22:37 ` Sean Christopherson
2021-08-20 18:42 ` Jing Zhang
2021-09-22 16:39 ` Sean Christopherson
2021-09-22 16:41 ` Sean Christopherson
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='CAAdAUtgcxZXWnqd6XNW7P=SwRQtGm11vrS9-T7rSOVc4xnySyQ@mail.gmail.com' \
--to=jingzhangos@google.com \
--cc=cannonmatthews@google.com \
--cc=dmatlack@google.com \
--cc=kvm@vger.kernel.org \
--cc=oupton@google.com \
--cc=pbonzini@redhat.com \
--cc=pshier@google.com \
--cc=seanjc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).