On Thu, Jul 8, 2021 at 4:42 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Tue, Jul 06, 2021 at 06:03:50PM +0000, Jing Zhang wrote:
> > Add simple stats halt_wait_ns to record the time a VCPU has spent on
> > waiting for all architectures (not just powerpc).
> > Add three log histogram stats to record the distribution of time spent
> > on successful polling, failed polling and VCPU wait.
> > halt_poll_success_hist: Distribution of time spent before a successful
> > polling.
> > halt_poll_fail_hist: Distribution of time spent before a failed polling.
> > halt_wait_hist: Distribution of time a VCPU has spent on waiting.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> > arch/powerpc/include/asm/kvm_host.h | 1 -
> > arch/powerpc/kvm/book3s.c | 1 -
> > arch/powerpc/kvm/book3s_hv.c | 20 +++++++++++++++++---
> > arch/powerpc/kvm/booke.c | 1 -
> > include/linux/kvm_host.h | 9 ++++++++-
> > include/linux/kvm_types.h | 4 ++++
> > virt/kvm/kvm_main.c | 19 +++++++++++++++++++
> > 7 files changed, 48 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> > index 9f52f282b1aa..4931d03e5799 100644
> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -103,7 +103,6 @@ struct kvm_vcpu_stat {
> > u64 emulated_inst_exits;
> > u64 dec_exits;
> > u64 ext_intr_exits;
> > - u64 halt_wait_ns;
>
> The halt_wait_ns refactor should be a separate patch.
>
How about putting it into a separate commit? Just want to keep all
halt related change in the
same patch series.
> > u64 halt_successful_wait;
> > u64 dbell_exits;
> > u64 gdbell_exits;
> > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> > index 5cc6e90095b0..b785f6772391 100644
> > --- a/arch/powerpc/kvm/book3s.c
> > +++ b/arch/powerpc/kvm/book3s.c
> > @@ -69,7 +69,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
> > STATS_DESC_COUNTER(VCPU, emulated_inst_exits),
> > STATS_DESC_COUNTER(VCPU, dec_exits),
> > STATS_DESC_COUNTER(VCPU, ext_intr_exits),
> > - STATS_DESC_TIME_NSEC(VCPU, halt_wait_ns),
> > STATS_DESC_COUNTER(VCPU, halt_successful_wait),
> > STATS_DESC_COUNTER(VCPU, dbell_exits),
> > STATS_DESC_COUNTER(VCPU, gdbell_exits),
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index cd544a46183e..103f998cee75 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -4144,19 +4144,33 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
> >
> > /* Attribute wait time */
> > if (do_sleep) {
> > - vc->runner->stat.halt_wait_ns +=
> > + vc->runner->stat.generic.halt_wait_ns +=
> > ktime_to_ns(cur) - ktime_to_ns(start_wait);
> > + kvm_stats_log_hist_update(
> > + vc->runner->stat.generic.halt_wait_hist,
> > + LOGHIST_SIZE_LARGE,
> > + ktime_to_ns(cur) - ktime_to_ns(start_wait));
> > /* Attribute failed poll time */
> > - if (vc->halt_poll_ns)
> > + if (vc->halt_poll_ns) {
> > vc->runner->stat.generic.halt_poll_fail_ns +=
> > ktime_to_ns(start_wait) -
> > ktime_to_ns(start_poll);
> > + kvm_stats_log_hist_update(
> > + vc->runner->stat.generic.halt_poll_fail_hist,
> > + LOGHIST_SIZE_LARGE, ktime_to_ns(start_wait) -
> > + ktime_to_ns(start_poll));
> > + }
> > } else {
> > /* Attribute successful poll time */
> > - if (vc->halt_poll_ns)
> > + if (vc->halt_poll_ns) {
> > vc->runner->stat.generic.halt_poll_success_ns +=
> > ktime_to_ns(cur) -
> > ktime_to_ns(start_poll);
> > + kvm_stats_log_hist_update(
> > + vc->runner->stat.generic.halt_poll_success_hist,
> > + LOGHIST_SIZE_LARGE,
> > + ktime_to_ns(cur) - ktime_to_ns(start_poll));
> > + }
> > }
> >
> > /* Adjust poll time */
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index 5ed6c235e059..977801c83aff 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -67,7 +67,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
> > STATS_DESC_COUNTER(VCPU, emulated_inst_exits),
> > STATS_DESC_COUNTER(VCPU, dec_exits),
> > STATS_DESC_COUNTER(VCPU, ext_intr_exits),
> > - STATS_DESC_TIME_NSEC(VCPU, halt_wait_ns),
> > STATS_DESC_COUNTER(VCPU, halt_successful_wait),
> > STATS_DESC_COUNTER(VCPU, dbell_exits),
> > STATS_DESC_COUNTER(VCPU, gdbell_exits),
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 356af173114d..268a0ccc9c5f 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1369,7 +1369,14 @@ struct _kvm_stats_desc {
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_poll_invalid), \
> > STATS_DESC_COUNTER(VCPU_GENERIC, halt_wakeup), \
> > STATS_DESC_TIME_NSEC(VCPU_GENERIC, halt_poll_success_ns), \
> > - STATS_DESC_TIME_NSEC(VCPU_GENERIC, halt_poll_fail_ns)
> > + STATS_DESC_TIME_NSEC(VCPU_GENERIC, halt_poll_fail_ns), \
> > + STATS_DESC_LOGHIST_TIME_NSEC(VCPU_GENERIC, halt_poll_success_hist, \
> > + LOGHIST_SIZE_LARGE), \
>
> This should probably be a new macro rather than using LOGHIST_SIZE_LARGE
> everywhere.
>
> #define HALT_POLL_HIST_SIZE LOGHIST_SIZE_LARGE
>
Will double check the new halt polling stats with this idea.
> > + STATS_DESC_LOGHIST_TIME_NSEC(VCPU_GENERIC, halt_poll_fail_hist, \
> > + LOGHIST_SIZE_LARGE), \
> > + STATS_DESC_TIME_NSEC(VCPU_GENERIC, halt_wait_ns), \
> > + STATS_DESC_LOGHIST_TIME_NSEC(VCPU_GENERIC, halt_wait_hist, \
> > + LOGHIST_SIZE_LARGE)
> >
> > extern struct dentry *kvm_debugfs_dir;
> >
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index cc88cd676775..7838a42932c8 100644
> > --- a/include/linux/kvm_types.h
> > +++ b/include/linux/kvm_types.h
> > @@ -103,6 +103,10 @@ struct kvm_vcpu_stat_generic {
> > u64 halt_wakeup;
> > u64 halt_poll_success_ns;
> > u64 halt_poll_fail_ns;
> > + u64 halt_poll_success_hist[LOGHIST_SIZE_LARGE];
> > + u64 halt_poll_fail_hist[LOGHIST_SIZE_LARGE];
> > + u64 halt_wait_ns;
> > + u64 halt_wait_hist[LOGHIST_SIZE_LARGE];
> > };
> >
> > #define KVM_STATS_NAME_SIZE 48
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 3dcc2abbfc60..840b5bece080 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3093,12 +3093,24 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> > ++vcpu->stat.generic.halt_successful_poll;
> > if (!vcpu_valid_wakeup(vcpu))
> > ++vcpu->stat.generic.halt_poll_invalid;
> > +
> > + kvm_stats_log_hist_update(
> > + vcpu->stat.generic.halt_poll_success_hist,
> > + LOGHIST_SIZE_LARGE,
> > + ktime_to_ns(ktime_get()) -
> > + ktime_to_ns(start));
> > goto out;
> > }
> > poll_end = cur = ktime_get();
> > } while (kvm_vcpu_can_poll(cur, stop));
> > +
> > + kvm_stats_log_hist_update(
> > + vcpu->stat.generic.halt_poll_fail_hist,
> > + LOGHIST_SIZE_LARGE,
> > + ktime_to_ns(ktime_get()) - ktime_to_ns(start));
>
> nit: Consider creating a wrapper for kvm_stats_log_hist_update() since
> there are so many call sites. You can save a line at every call site by
> avoiding passing in the histogram size.
>
> void halt_poll_hist_update(u64 *histogram, ktime_t time)
> {
> kvm_stats_log_hist_update(histogram, LOGHIST_SIZE_LARGE, time);
> }
>
>
Will double check the new halt polling stats with this idea.
> > }
> >
> > +
> > prepare_to_rcuwait(&vcpu->wait);
> > for (;;) {
> > set_current_state(TASK_INTERRUPTIBLE);
> > @@ -3111,6 +3123,13 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> > }
> > finish_rcuwait(&vcpu->wait);
> > cur = ktime_get();
> > + if (waited) {
> > + vcpu->stat.generic.halt_wait_ns +=
> > + ktime_to_ns(cur) - ktime_to_ns(poll_end);
> > + kvm_stats_log_hist_update(vcpu->stat.generic.halt_wait_hist,
> > + LOGHIST_SIZE_LARGE,
> > + ktime_to_ns(cur) - ktime_to_ns(poll_end));
> > + }
> > out:
> > kvm_arch_vcpu_unblocking(vcpu);
> > block_ns = ktime_to_ns(cur) - ktime_to_ns(start);
> > --
> > 2.32.0.93.g670b81a890-goog
> >
Thanks,
Jing
