From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755716AbcH1SKC (ORCPT ); Sun, 28 Aug 2016 14:10:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35912 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750857AbcH1SKB (ORCPT ); Sun, 28 Aug 2016 14:10:01 -0400 Date: Sun, 28 Aug 2016 20:09:55 +0200 From: Jiri Olsa To: Vegard Nossum , Peter Zijlstra Cc: 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: <20160828180955.GA9816@krava> References: <1471467307-61171-2-git-send-email-davidcc@google.com> <20160822071737.GF4349@krava> <20160822082932.GA13171@krava> <20160822103823.GA2271@krava> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160822103823.GA2271@krava> 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.27]); Sun, 28 Aug 2016 18:10:00 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 22, 2016 at 12:38:23PM +0200, Jiri Olsa wrote: > On Mon, Aug 22, 2016 at 10:29:32AM +0200, Jiri Olsa wrote: > > On Mon, Aug 22, 2016 at 09:17:37AM +0200, Jiri Olsa wrote: > > > On Sun, Aug 21, 2016 at 02:10:07PM +0200, Vegard Nossum wrote: > > > > > > SNIP > > > > > > > [] ? __fget+0x47/0x270 > > > > [] vfs_readv+0x8b/0xc0 > > > > [] do_readv+0xde/0x230 > > > > [] ? vfs_readv+0xc0/0xc0 > > > > [] ? exit_to_usermode_loop+0x190/0x190 > > > > [] ? check_preemption_disabled+0x37/0x1e0 > > > > [] SyS_readv+0x27/0x30 > > > > [] ? do_pwritev+0x1a0/0x1a0 > > > > [] do_syscall_64+0x1c4/0x4e0 > > > > [] entry_SYSCALL64_slow_path+0x25/0x25 > > > > > > > > I don't think WARN() is the right interface for signalling errors to > > > > userspace programs? > > > > > > any special way to trigger that? > > > > nope ;-) > > > > perf stat -a -I 10 > > reading the event could race with event schedule out > leaving us with active state and oncpu == -1 > > attached patch fixes the warn for me, but I might > be missing some other cases > > jirka ping, thanks jirka > > > --- > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 3f07e6cfc1b6..375274b6f3b4 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -1802,8 +1802,9 @@ event_sched_out(struct perf_event *event, > > event->tstamp_stopped = tstamp; > event->pmu->del(event, 0); > - event->oncpu = -1; > - event->state = PERF_EVENT_STATE_INACTIVE; > + WRITE_ONCE(event->state, PERF_EVENT_STATE_INACTIVE); > + smp_wmb(); > + WRITE_ONCE(event->oncpu, -1); > if (event->pending_disable) { > event->pending_disable = 0; > event->state = PERF_EVENT_STATE_OFF; > @@ -3424,9 +3425,8 @@ struct perf_read_data { > int ret; > }; > > -static int find_cpu_to_read(struct perf_event *event, int local_cpu) > +static int find_cpu_to_read(struct perf_event *event, int event_cpu, int local_cpu) > { > - int event_cpu = event->oncpu; > u16 local_pkg, event_pkg; > > if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) { > @@ -3561,13 +3561,17 @@ u64 perf_event_read_local(struct perf_event *event) > > static int perf_event_read(struct perf_event *event, bool group) > { > - int ret = 0, cpu_to_read, local_cpu; > + int ret = 0, cpu_to_read, local_cpu, state; > + > + state = READ_ONCE(event->state); > + smp_rmb(); > + cpu_to_read = event->oncpu; > > /* > * If event is enabled and currently active on a CPU, update the > * value in the event structure: > */ > - if (event->state == PERF_EVENT_STATE_ACTIVE) { > + if (state == PERF_EVENT_STATE_ACTIVE && cpu_to_read != -1) { > struct perf_read_data data = { > .event = event, > .group = group, > @@ -3575,7 +3579,7 @@ static int perf_event_read(struct perf_event *event, bool group) > }; > > local_cpu = get_cpu(); > - cpu_to_read = find_cpu_to_read(event, local_cpu); > + cpu_to_read = find_cpu_to_read(event, cpu_to_read, local_cpu); > put_cpu(); > > ret = smp_call_function_single(cpu_to_read, __perf_event_read, &data, 1);