| Message ID | 20220317005630.3666572-3-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add arm64 vcpu exit reasons and tracepoint | expand |
Hi Jing, On Thu, Mar 17, 2022 at 12:56:30AM +0000, Jing Zhang wrote: > This tracepoint only provides a hook for poking vcpu exits information, > not exported to tracefs. > A timestamp is added for the last vcpu exit, which would be useful for > analysis for vcpu exits. > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/arm.c | 2 ++ > arch/arm64/kvm/trace_arm.h | 8 ++++++++ > 3 files changed, 13 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index daa68b053bdc..576f2c18d008 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -415,6 +415,9 @@ struct kvm_vcpu_arch { > > /* Arch specific exit reason */ > enum arm_exit_reason exit_reason; > + > + /* Timestamp for the last vcpu exit */ > + u64 last_exit_time; > }; > > /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index f49ebdd9c990..98631f79c182 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > ret = 1; > run->exit_reason = KVM_EXIT_UNKNOWN; > while (ret > 0) { > + trace_kvm_vcpu_exits(vcpu); > /* > * Check conditions before entering the guest > */ > @@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > local_irq_enable(); > > trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu)); > + vcpu->arch.last_exit_time = ktime_to_ns(ktime_get()); > > /* Exit types that need handling before we can be preempted */ > handle_exit_early(vcpu, ret); > diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h > index 33e4e7dd2719..3e7dfd640e23 100644 > --- a/arch/arm64/kvm/trace_arm.h > +++ b/arch/arm64/kvm/trace_arm.h > @@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate, > __entry->timer_idx, __entry->should_fire) > ); > > +/* > + * Following tracepoints are not exported in tracefs and provide hooking > + * mechanisms only for testing and debugging purposes. > + */ > +DECLARE_TRACE(kvm_vcpu_exits, > + TP_PROTO(struct kvm_vcpu *vcpu), > + TP_ARGS(vcpu)); > + When we were discussing this earlier, I wasn't aware of the kvm_exit tracepoint which I think encapsulates what you're looking for. ESR_EL2.EC is the critical piece to determine what caused the exit. It is probably also important to call out that this trace point only will fire for a 'full' KVM exit (i.e. out of hyp and back to the kernel). There are several instances where the exit is handled in hyp and we immediately resume the guest. Now -- I am bordering on clueless with tracepoints, but it isn't immediately obvious how the attached program can determine the vCPU that triggered the TP. If we are going to propose modularizing certain KVM metrics with tracepoints then that would be a rather critical piece of information. Apologies for any confusion I added to the whole situation, but hopefully we can still engage in a broader conversation regarding how to package up optional KVM metrics. -- Thanks, Oliver
Hi Jing, On Thu, 17 Mar 2022 00:56:30 +0000, Jing Zhang <jingzhangos@google.com> wrote: > > This tracepoint only provides a hook for poking vcpu exits information, > not exported to tracefs. > A timestamp is added for the last vcpu exit, which would be useful for > analysis for vcpu exits. The trace itself gives you a timestamp. Why do you need an extra one? > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/arm.c | 2 ++ > arch/arm64/kvm/trace_arm.h | 8 ++++++++ > 3 files changed, 13 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index daa68b053bdc..576f2c18d008 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -415,6 +415,9 @@ struct kvm_vcpu_arch { > > /* Arch specific exit reason */ > enum arm_exit_reason exit_reason; > + > + /* Timestamp for the last vcpu exit */ > + u64 last_exit_time; > }; > > /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index f49ebdd9c990..98631f79c182 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > ret = 1; > run->exit_reason = KVM_EXIT_UNKNOWN; > while (ret > 0) { > + trace_kvm_vcpu_exits(vcpu); Exit? We haven't entered the guest yet! > /* > * Check conditions before entering the guest > */ > @@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > local_irq_enable(); > > trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu)); > + vcpu->arch.last_exit_time = ktime_to_ns(ktime_get()); Why isn't the above tracepoint sufficient? It gives you the EC, and comes with a timestamp for free. And why should *everyone* pay the price of this timestamp update if not tracing? > > /* Exit types that need handling before we can be preempted */ > handle_exit_early(vcpu, ret); > diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h > index 33e4e7dd2719..3e7dfd640e23 100644 > --- a/arch/arm64/kvm/trace_arm.h > +++ b/arch/arm64/kvm/trace_arm.h > @@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate, > __entry->timer_idx, __entry->should_fire) > ); > > +/* > + * Following tracepoints are not exported in tracefs and provide hooking > + * mechanisms only for testing and debugging purposes. > + */ > +DECLARE_TRACE(kvm_vcpu_exits, > + TP_PROTO(struct kvm_vcpu *vcpu), > + TP_ARGS(vcpu)); > + > #endif /* _TRACE_ARM_ARM64_KVM_H */ > > #undef TRACE_INCLUDE_PATH I guess this is the only bit I actually like about this series: a generic, out of the way mechanism to let people hook whatever they want and dump the state they need. Thanks, M.
On Wed, Mar 16, 2022 at 10:37 PM Oliver Upton <oupton@google.com> wrote: > > Hi Jing, > > On Thu, Mar 17, 2022 at 12:56:30AM +0000, Jing Zhang wrote: > > This tracepoint only provides a hook for poking vcpu exits information, > > not exported to tracefs. > > A timestamp is added for the last vcpu exit, which would be useful for > > analysis for vcpu exits. > > > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 3 +++ > > arch/arm64/kvm/arm.c | 2 ++ > > arch/arm64/kvm/trace_arm.h | 8 ++++++++ > > 3 files changed, 13 insertions(+) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index daa68b053bdc..576f2c18d008 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -415,6 +415,9 @@ struct kvm_vcpu_arch { > > > > /* Arch specific exit reason */ > > enum arm_exit_reason exit_reason; > > + > > + /* Timestamp for the last vcpu exit */ > > + u64 last_exit_time; > > }; > > > > /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index f49ebdd9c990..98631f79c182 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > > ret = 1; > > run->exit_reason = KVM_EXIT_UNKNOWN; > > while (ret > 0) { > > + trace_kvm_vcpu_exits(vcpu); > > /* > > * Check conditions before entering the guest > > */ > > @@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > > local_irq_enable(); > > > > trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu)); > > + vcpu->arch.last_exit_time = ktime_to_ns(ktime_get()); > > > > /* Exit types that need handling before we can be preempted */ > > handle_exit_early(vcpu, ret); > > diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h > > index 33e4e7dd2719..3e7dfd640e23 100644 > > --- a/arch/arm64/kvm/trace_arm.h > > +++ b/arch/arm64/kvm/trace_arm.h > > @@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate, > > __entry->timer_idx, __entry->should_fire) > > ); > > > > +/* > > + * Following tracepoints are not exported in tracefs and provide hooking > > + * mechanisms only for testing and debugging purposes. > > + */ > > +DECLARE_TRACE(kvm_vcpu_exits, > > + TP_PROTO(struct kvm_vcpu *vcpu), > > + TP_ARGS(vcpu)); > > + > > When we were discussing this earlier, I wasn't aware of the kvm_exit > tracepoint which I think encapsulates what you're looking for. > ESR_EL2.EC is the critical piece to determine what caused the exit. > > It is probably also important to call out that this trace point only > will fire for a 'full' KVM exit (i.e. out of hyp and back to the > kernel). There are several instances where the exit is handled in hyp > and we immediately resume the guest. > > Now -- I am bordering on clueless with tracepoints, but it isn't > immediately obvious how the attached program can determine the vCPU that > triggered the TP. If we are going to propose modularizing certain KVM > metrics with tracepoints then that would be a rather critical piece of > information. > > Apologies for any confusion I added to the whole situation, but > hopefully we can still engage in a broader conversation regarding > how to package up optional KVM metrics. These are all good questions. For context to non-Google folks on the mailing list, we are interested in exploring Marc's idea of using tracepoint hooking as a way for e.g. cloud providers to implement proprietary stats without having to modify KVM. Adding specific tracepoints (like this series does) is probably premature until we have figured out the broader design for how out-of-module stats will work end-to-end and get that infrastructure merged upstream. > > -- > Thanks, > Oliver
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index daa68b053bdc..576f2c18d008 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -415,6 +415,9 @@ struct kvm_vcpu_arch { /* Arch specific exit reason */ enum arm_exit_reason exit_reason; + + /* Timestamp for the last vcpu exit */ + u64 last_exit_time; }; /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index f49ebdd9c990..98631f79c182 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -783,6 +783,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) ret = 1; run->exit_reason = KVM_EXIT_UNKNOWN; while (ret > 0) { + trace_kvm_vcpu_exits(vcpu); /* * Check conditions before entering the guest */ @@ -898,6 +899,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) local_irq_enable(); trace_kvm_exit(ret, kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu)); + vcpu->arch.last_exit_time = ktime_to_ns(ktime_get()); /* Exit types that need handling before we can be preempted */ handle_exit_early(vcpu, ret); diff --git a/arch/arm64/kvm/trace_arm.h b/arch/arm64/kvm/trace_arm.h index 33e4e7dd2719..3e7dfd640e23 100644 --- a/arch/arm64/kvm/trace_arm.h +++ b/arch/arm64/kvm/trace_arm.h @@ -301,6 +301,14 @@ TRACE_EVENT(kvm_timer_emulate, __entry->timer_idx, __entry->should_fire) ); +/* + * Following tracepoints are not exported in tracefs and provide hooking + * mechanisms only for testing and debugging purposes. + */ +DECLARE_TRACE(kvm_vcpu_exits, + TP_PROTO(struct kvm_vcpu *vcpu), + TP_ARGS(vcpu)); + #endif /* _TRACE_ARM_ARM64_KVM_H */ #undef TRACE_INCLUDE_PATH
This tracepoint only provides a hook for poking vcpu exits information, not exported to tracefs. A timestamp is added for the last vcpu exit, which would be useful for analysis for vcpu exits. Signed-off-by: Jing Zhang <jingzhangos@google.com> --- arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/arm.c | 2 ++ arch/arm64/kvm/trace_arm.h | 8 ++++++++ 3 files changed, 13 insertions(+)