On Mon, Jun 17, 2024 at 09:34:15AM -0400, Liang, Kan wrote:
>
>
> On 2024-06-17 3:51 a.m., Peter Zijlstra wrote:
> > 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,
>
> But the implementation seems stop the time.
>
> The ctx->is_active should be (EVENT_ALL | EVENT_TIME) for most of cases.
>
> When a vPMU is scheduling in (invoke ctx_sched_out()), the event_type
> should only be EVENT_GUEST.
>
> !((ctx->is_active & event_type) & EVENT_ALL) should be TRUE.
Hmm.. yeah, I think I might've gotten that wrong.
> > only the
> > 'normal' sched-out should stop time.
>
> If the guest is the only case which we want to keep the time for, I
> think we may use a straightforward check as below.
>
> stop = !(event_type & EVENT_GUEST) && ctx == &cpuctx->ctx;
So I think I was trying to get stop true when there weren't in fact
events on, that is when we got in without EVENT_ALL bits, but perhaps
that case isn't relevant.
