Re: [PATCH v2] perf/core: Optimize event reschedule for a PMU

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2024-08-05 10:58 a.m., Peter Zijlstra wrote:
> On Mon, Aug 05, 2024 at 11:20:58AM +0200, Peter Zijlstra wrote:
>> On Fri, Aug 02, 2024 at 02:30:19PM -0400, Liang, Kan wrote:
>>>> @@ -2792,7 +2833,14 @@ static int  __perf_install_in_context(void *info)
>>>>  	if (reprogram) {
>>>>  		ctx_sched_out(ctx, EVENT_TIME);
>>>>  		add_event_to_ctx(event, ctx);
>>>> -		ctx_resched(cpuctx, task_ctx, get_event_type(event));
>>>> +		if (ctx->nr_events == 1) {
>>>> +			/* The first event needs to set ctx->is_active. */
>>>> +			ctx_resched(cpuctx, task_ctx, NULL, get_event_type(event));
>>>> +		} else {
>>>> +			ctx_resched(cpuctx, task_ctx, event->pmu_ctx->pmu,
>>>> +				    get_event_type(event));
>>>> +			ctx_sched_in(ctx, EVENT_TIME);
>>>
>>> The changelog doesn't mention the time difference much. As my
>>> understanding, the time is shared among PMUs in the same ctx.
>>> When perf does ctx_resched(), the time is deducted.
>>> There is no problem to stop and restart the global time when perf
>>> re-schedule all PMUs.
>>> But if only one PMU is re-scheduled while others are still running, it
>>> may be a problem to stop and restart the global time. Other PMUs will be
>>> impacted.
>>
>> So afaict, since we hold ctx->lock, nobody can observe EVENT_TIME was
>> cleared for a little while.
>>
>> So the point was to make all the various ctx_sched_out() calls have the
>> same timestamp. It does this by clearing EVENT_TIME first. Then the
>> first ctx_sched_in() will set it again, and later ctx_sched_in() won't
>> touch time.
>>
>> That leaves a little hole, because the time between
>> ctx_sched_out(EVENT_TIME) and the first ctx_sched_in() gets lost.
>>
>> This isn't typically a problem, but not very nice. Let me go find an
>> alternative solution for this. The simple update I did saturday is
>> broken as per the perf test.
> 
> OK, took a little longer than I would have liked, nor is it entirely
> pretty, but it seems to pass 'perf test'.
> 
> Please look at: queue.git perf/resched

Thanks. If I understand correctly, the freeze doesn't mean that the time
deduction. For the other PMUs, it can still see the time in during the
specific PMU reschedule, right? If so, the general idea looks good to me.

I also think of the vPMU time. For that case, perf has to deduct the
vPMU time. The freeze bit cannot be used in the vPMU case. But I
probably have to rebase the below patch on top of EVENT_FROZEN.
https://lore.kernel.org/kvm/20240801045907.4010984-10-mizhang@google.com/

Thanks,
Kan