Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753672AbdHWIyY (ORCPT ); Wed, 23 Aug 2017 04:54:24 -0400 Received: from mga07.intel.com ([134.134.136.100]:50991 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753603AbdHWIyU (ORCPT ); Wed, 23 Aug 2017 04:54:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,415,1498546800"; d="scan'208";a="121818981" Subject: Re: [PATCH v6 2/3]: perf/core: use context tstamp_data for skipped events on mux interrupt To: Peter Zijlstra Cc: Ingo Molnar , Arnaldo Carvalho de Melo , Alexander Shishkin , Andi Kleen , Kan Liang , Dmitri Prokhorov , Valery Cherepennikov , Mark Rutland , Stephane Eranian , David Carrillo-Cisneros , linux-kernel References: <96c7776f-1f17-a39e-23e9-658596216d6b@linux.intel.com> <20170803150052.za2vofyqfgarukdr@hirez.programming.kicks-ass.net> <20170822204743.GR32112@worktop.programming.kicks-ass.net> From: Alexey Budankov 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 List-ID: X-Mailing-List: linux-kernel@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. >