linux-kernel - Re: [PATCH v6 2/3]: perf/core: use context tstamp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Aug 2017 11:54:15 +0300
From:   Alexey Budankov <alexey.budankov@...ux.intel.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Kan Liang <kan.liang@...el.com>,
        Dmitri Prokhorov <Dmitry.Prohorov@...el.com>,
        Valery Cherepennikov <valery.cherepennikov@...el.com>,
        Mark Rutland <mark.rutland@....com>,
        Stephane Eranian <eranian@...gle.com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 2/3]: perf/core: use context tstamp_data for skipped
 events on mux interrupt

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. 

>