On Thu, Jun 13, 2024 at 02:04:36PM -0400, Liang, Kan wrote:
> >> static enum event_type_t get_event_type(struct perf_event *event)
> >> @@ -3340,9 +3388,14 @@ ctx_sched_out(struct perf_event_context
> >> * would only update time for the pinned events.
> >> */
> >> if (is_active & EVENT_TIME) {
> >> + bool stop;
> >> +
> >> + stop = !((ctx->is_active & event_type) & EVENT_ALL) &&
> >> + ctx == &cpuctx->ctx;
> >> +
> >> /* update (and stop) ctx time */
> >> update_context_time(ctx);
> >> - update_cgrp_time_from_cpuctx(cpuctx, ctx == &cpuctx->ctx);
> >> + update_cgrp_time_from_cpuctx(cpuctx, stop);
>
> For the event_type == EVENT_GUEST, the "stop" should always be the same
> as "ctx == &cpuctx->ctx". Because the ctx->is_active never set the
> EVENT_GUEST bit.
> Why the stop is introduced?
Because the ctx_sched_out() for vPMU should not stop time, only the
'normal' sched-out should stop time.
> >> @@ -3949,6 +4015,8 @@ ctx_sched_in(struct perf_event_context *
> >> return;
> >>
> >> if (!(is_active & EVENT_TIME)) {
> >> + /* EVENT_TIME should be active while the guest runs */
> >> + WARN_ON_ONCE(event_type & EVENT_GUEST);
> >> /* start ctx time */
> >> __update_context_time(ctx, false);
> >> perf_cgroup_set_timestamp(cpuctx);
> >> @@ -3979,8 +4047,11 @@ ctx_sched_in(struct perf_event_context *
> >> * the exclude_guest events.
> >> */
> >> update_context_time(ctx);
> >> - } else
> >> + update_cgrp_time_from_cpuctx(cpuctx, false);
>
>
> In the above ctx_sched_out(), the cgrp_time is stopped and the cgrp has
> been set to inactive.
> I think we need a perf_cgroup_set_timestamp(cpuctx) here to restart the
> cgrp_time, Right?
So the idea was to not stop time when we schedule out for the vPMU, as
per the above.
> Also, I think the cgrp_time is different from the normal ctx->time. When
> a guest is running, there must be no cgroup. It's OK to disable the
> cgrp_time. If so, I don't think we need to track the guest_time for the
> cgrp.
Uh, the vCPU thread is/can-be part of a cgroup, and different guests
part of different cgroups. The CPU wide 'guest' time is all time spend
in guets, but the cgroup view of things might differ, depending on how
the guets are arranged in cgroups, no?
As such, we need per cgroup guest tracking.
