Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753859AbdIDP6g (ORCPT ); Mon, 4 Sep 2017 11:58:36 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:34801 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753764AbdIDP6f (ORCPT ); Mon, 4 Sep 2017 11:58:35 -0400 Date: Mon, 4 Sep 2017 17:58:23 +0200 From: Peter Zijlstra To: Alexey Budankov 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 , Vince Weaver , Thomas Gleixner Subject: Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Message-ID: <20170904155823.unjqqjd7ffeudeo3@hirez.programming.kicks-ass.net> References: <20170803150052.za2vofyqfgarukdr@hirez.programming.kicks-ass.net> <20170822204743.GR32112@worktop.programming.kicks-ass.net> <2a426aa2-42c8-e839-1cec-aa3971651f3e@linux.intel.com> <20170831171837.njnc6r6elsvkl7lt@hirez.programming.kicks-ass.net> <46f72a3f-f18b-0227-3d78-fb23c8a6e18e@linux.intel.com> <20170904120843.oazlv73phoxoinlj@hirez.programming.kicks-ass.net> <385005b6-51ea-383e-df81-43365f3f5152@linux.intel.com> <20170904154145.xl4fyg7vhgbnmhwi@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170904154145.xl4fyg7vhgbnmhwi@hirez.programming.kicks-ass.net> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2125 Lines: 82 On Mon, Sep 04, 2017 at 05:41:45PM +0200, Peter Zijlstra wrote: > > >> U - allocation, A - ACTIVE, I - INACTIVE, O - OFF, > > >> E - ERROR, X - EXIT, D - DEAD, > > > > > > Not sure we care about the different <0 values, they're all effectively > > > OFF. > > > > We still need to care about proper initial state of timings when moving above >=0 state. > > Very true. I'm not sure I fully covered that, let me see if there's > something sensible to do for that. So given this: static __always_inline enum perf_event_state __perf_effective_state(struct perf_event *event) { struct perf_event *leader = event->group_leader; if (leader->state <= PERF_EVENT_STATE_OFF) return leader->state; return event->state; } static __always_inline void __perf_update_times(struct perf_event *event, u64 now, u64 *enabled, u64 *running) { enum perf_event_state state = __perf_effective_state(event); u64 delta = now - event->tstamp; *enabled = event->total_time_enabled; if (state >= PERF_EVENT_STATE_INACTIVE) *enabled += delta; *running = event->total_time_running; if (state >= PERF_EVENT_STATE_ACTIVE) *running += delta; } static void perf_event_update_time(struct perf_event *event) { u64 now = perf_event_time(event); __perf_update_times(event, now, &event->total_time_enabled, &event->total_time_running); event->tstamp = now; } static void perf_event_update_sibling_time(struct perf_event *leader) { struct perf_event *sibling; list_for_each_entry(sibling, &leader->sibling_list, group_entry) perf_event_update_time(sibling); } static void perf_event_set_state(struct perf_event *event, enum perf_event_state state) { if (event->state == state) return; perf_event_update_time(event); /* * If a group leader gets enabled/disabled all its siblings * are affected too. */ if ((event->state < 0) ^ (state < 0)) perf_event_update_sibling_time(event); WRITE_ONCE(event->state, state); } If event->state < 0, and we do perf_event_set_state(event, INACTIVE) then perf_event_update_time() will not add to enabled, not add to running, but set ->tstamp = now. So I think it DTRT.