Subject: Re: [PATCH v6 2/3]: perf/core: use context tstamp_data for skipped
 events on mux interrupt
To: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Andi Kleen <ak@linux.intel.com>, Kan Liang <kan.liang@intel.com>,
        Dmitri Prokhorov <Dmitry.Prohorov@intel.com>,
        Valery Cherepennikov <valery.cherepennikov@intel.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Stephane Eranian <eranian@google.com>,
        David Carrillo-Cisneros <davidcc@google.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
References: <96c7776f-1f17-a39e-23e9-658596216d6b@linux.intel.com>
 <bb431829-bf5d-8278-131d-a41f0001e594@linux.intel.com>
 <20170803150052.za2vofyqfgarukdr@hirez.programming.kicks-ass.net>
 <c052d929-db1d-8b31-4a62-07563d48063b@linux.intel.com>
 <20170822204743.GR32112@worktop.programming.kicks-ass.net>
From: Alexey Budankov <alexey.budankov@linux.intel.com>
Organization: Intel Corp.
Message-ID: <2a426aa2-42c8-e839-1cec-aa3971651f3e@linux.intel.com>
Date: Wed, 23 Aug 2017 11:54:15 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <20170822204743.GR32112@worktop.programming.kicks-ass.net>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1380
Lines: 37

On 22.08.2017 23:47, Peter Zijlstra wrote:
> On Thu, Aug 10, 2017 at 06:57:43PM +0300, Alexey Budankov wrote:
>> The key thing in the patch is explicit updating of tstamp fields for
>> INACTIVE events in update_event_times().
> 
>> @@ -1405,6 +1426,9 @@ static void update_event_times(struct perf_event *event)
>>  	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
>>  		return;
>>  
>> +	if (event->state == PERF_EVENT_STATE_INACTIVE)
>> +		perf_event_tstamp_update(event);
>> +
>>  	/*
>>  	 * in cgroup mode, time_enabled represents
>>  	 * the time the event was enabled AND active
> 
> But why!? I thought the whole point was to not need to do this.

update_event_times() is not called from timer interrupt handler 
thus it is not on the critical path which is optimized in this patch set.

But update_event_times() is called in the context of read() syscall so
this is the place where we may update event times for INACTIVE events 
instead of timer interrupt.

Also update_event_times() is called on thread context switch out so
we get event times also updated when the thread migrates to other CPU.

> 
> The thing I outlined earlier would only need to update timestamps when
> events change state and at no other point in time.

But we still may request times while event is in INACTIVE state 
thru read() syscall and event timings need to be up-to-date. 

>