Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753097AbdHDOXo (ORCPT ); Fri, 4 Aug 2017 10:23:44 -0400 Received: from mga09.intel.com ([134.134.136.24]:5184 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752632AbdHDOXk (ORCPT ); Fri, 4 Aug 2017 10:23:40 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,321,1498546800"; d="scan'208";a="1159197572" 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> <20170804123534.bcej4vblurz3stqb@hirez.programming.kicks-ass.net> From: Alexey Budankov Organization: Intel Corp. Message-ID: <1b32e61e-0f0c-e5d8-f03b-92efefc1a4cd@linux.intel.com> Date: Fri, 4 Aug 2017 17:23:35 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170804123534.bcej4vblurz3stqb@hirez.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: 2213 Lines: 81 On 04.08.2017 15:35, Peter Zijlstra wrote: > On Thu, Aug 03, 2017 at 09:47:56PM +0300, Alexey Budankov wrote: >> On 03.08.2017 18:00, Peter Zijlstra wrote: > >>> Are the magic spots, right? And I'm not convinced its right. >>> >>> Suppose I have two events in my context, and I created them 1 minute >>> apart. Then their respective tstamp_enabled are 1 minute apart as well. >>> But the above doesn't seem to preserve that difference. >>> >>> A similar argument can be made for running I think. That is a per event >>> value and cannot be passed along to the ctx and back. >> >> Aww, I see your point and it challenges my initial assumptions. >> Let me think thru the case more. There must be some solution. Thanks! > > So the sensible thing is probably to rewrite the entire time tracking to > make more sense. OTOH that's also the riskiest. > > Something like: > > __update_state_and_time(event, new_state) > { > u64 delta, now = perf_event_time(event); > int old_state = event->state; > > event->tstamp = now; > event->state = new_state; > > delta = now - event->tstamp; > switch (state) { > case STATE_ACTIVE: > WARN_ON_ONCE(old_state != STATE_INACTIVE); > event->total_time_enabled += delta; > break; > > case STATE_INACTIVE: > switch (old_state) { > case STATE_OFF: > /* ignore the OFF -> INACTIVE period */ > break; > > case STATE_ACTIVE: > event->total_time_enabled += delta; > event->total_time_running += delta; > break; > > default: > WARN_ONCE(); > } > break; > > case STATE_OFF: > WARN_ON_ONCE(old_state != STATE_INACTIVE) > event->total_time_enabled += delta; > break; > } > } > > __read_curent_times(event, u64 *enabled, u64 *running) > { > u64 delta, now = perf_event_time(event); > > delta = now - event->tstamp; > > *enabled = event->total_time_enabled; > if (event->state >= STATE_INACTIVE) > *enabled += delta; > *running = event->total_time_running > if (event->state == STATE_ACTIVE) > *running += delta; > } > > perhaps? That instantly solves the problem I think, because now we don't > need to update inactive events. But maybe I missed some, could you > verify? Thanks for the input. I will check it. > >