From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757818AbcH3Jr0 (ORCPT ); Tue, 30 Aug 2016 05:47:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53852 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756437AbcH3JrW (ORCPT ); Tue, 30 Aug 2016 05:47:22 -0400 Date: Tue, 30 Aug 2016 11:47:17 +0200 From: Jiri Olsa To: Peter Zijlstra Cc: Vegard Nossum , Thomas Gleixner , Stephane Eranian , Vince Weaver , Ingo Molnar , David Carrillo-Cisneros , "H. Peter Anvin" , Kan Liang , Arnaldo Carvalho de Melo , Paul Turner , Linus Torvalds , LKML , Alexander Shishkin , linux-tip-commits@vger.kernel.org Subject: Re: [tip:perf/core] perf/core: Check return value of the perf_event_read() IPI Message-ID: <20160830094717.GA21472@krava> References: <20160822071737.GF4349@krava> <20160822082932.GA13171@krava> <20160822103823.GA2271@krava> <20160829100309.GS10121@twins.programming.kicks-ass.net> <20160829130213.GF10168@twins.programming.kicks-ass.net> <20160829185841.GA27077@krava> <20160830064724.GX10153@twins.programming.kicks-ass.net> <20160830072603.GG10168@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160830072603.GG10168@twins.programming.kicks-ass.net> User-Agent: Mutt/1.7.0 (2016-08-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 30 Aug 2016 09:47:21 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 30, 2016 at 09:26:03AM +0200, Peter Zijlstra wrote: > On Tue, Aug 30, 2016 at 08:47:24AM +0200, Peter Zijlstra wrote: > > > If oncpu is not valid, the sched_out that made it invalid will have > > updated the event count and we're good. > > > > All I'll leave is an explicit comment that we've ignored the > > smp_call_function_single() return value on purpose. > > Something like so.. yep, it works in my tests also I thought there's no group time update in __perf_event_read, so I was hunting that but then I noticed we do that after during the read.. and meanwhile came to patch below ;-) jirka --- diff --git a/kernel/events/core.c b/kernel/events/core.c index 305dbd28ea86..c637496251fe 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -3440,6 +3440,26 @@ static int find_cpu_to_read(struct perf_event *event, int local_cpu) return event_cpu; } +static void read_update_times(struct perf_event *event, bool group) +{ + struct perf_event_context *ctx = event->ctx; + + /* + * may read while context is not active + * (e.g., thread is blocked), in that case + * we cannot update context time + */ + if (ctx->is_active) { + update_context_time(ctx); + update_cgrp_time_from_event(event); + } + + if (group) + update_group_times(event); + else + update_event_times(event); +} + /* * Cross CPU call to read the hardware event */ @@ -3462,12 +3482,9 @@ static void __perf_event_read(void *info) return; raw_spin_lock(&ctx->lock); - if (ctx->is_active) { - update_context_time(ctx); - update_cgrp_time_from_event(event); - } - update_event_times(event); + read_update_times(event, data->group); + if (event->state != PERF_EVENT_STATE_ACTIVE) goto unlock; @@ -3482,7 +3499,6 @@ static void __perf_event_read(void *info) pmu->read(event); list_for_each_entry(sub, &event->sibling_list, group_entry) { - update_event_times(sub); if (sub->state == PERF_EVENT_STATE_ACTIVE) { /* * Use sibling's PMU rather than @event's since @@ -3596,19 +3612,7 @@ static int perf_event_read(struct perf_event *event, bool group) unsigned long flags; raw_spin_lock_irqsave(&ctx->lock, flags); - /* - * may read while context is not active - * (e.g., thread is blocked), in that case - * we cannot update context time - */ - if (ctx->is_active) { - update_context_time(ctx); - update_cgrp_time_from_event(event); - } - if (group) - update_group_times(event); - else - update_event_times(event); + read_update_times(event, group); raw_spin_unlock_irqrestore(&ctx->lock, flags); }