From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932978AbcDYQDS (ORCPT ); Mon, 25 Apr 2016 12:03:18 -0400 Received: from mga03.intel.com ([134.134.136.65]:26882 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932925AbcDYQDR (ORCPT ); Mon, 25 Apr 2016 12:03:17 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,533,1455004800"; d="scan'208";a="939785339" From: Alexander Shishkin To: Peter Zijlstra Cc: Ingo Molnar , linux-kernel@vger.kernel.org, vince@deater.net, eranian@google.com, Arnaldo Carvalho de Melo , Mathieu Poirier Subject: Re: [PATCH v1 4/5] perf: Introduce address range filtering In-Reply-To: <20160425131013.GD3448@twins.programming.kicks-ass.net> References: <1461251823-12416-1-git-send-email-alexander.shishkin@linux.intel.com> <1461251823-12416-5-git-send-email-alexander.shishkin@linux.intel.com> <20160422074555.GB3448@twins.programming.kicks-ass.net> <87zisl8vls.fsf@ashishki-desk.ger.corp.intel.com> <20160425131013.GD3448@twins.programming.kicks-ass.net> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Mon, 25 Apr 2016 19:02:32 +0300 Message-ID: <87twip8ynb.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Peter Zijlstra writes: > On Fri, Apr 22, 2016 at 07:19:11PM +0300, Alexander Shishkin wrote: >> +static int perf_event_restart(struct perf_event *event) >> +{ >> + struct stop_event_data sd = { >> + .event = event, >> + .restart = 1, >> + }; >> + int ret = 0; >> + >> + do { >> + if (READ_ONCE(event->state) != PERF_EVENT_STATE_ACTIVE) >> + return 0; >> + >> + /* matches smp_wmb() in event_sched_in() */ >> + smp_rmb(); >> + >> + ret = cpu_function_call(READ_ONCE(event->oncpu), >> + __perf_event_stop, &sd); >> + } while (ret == -EAGAIN); >> + >> + return ret; >> +} > > A few concerns with this; (awesome, and now I see where the comments are actually needed) > - if I understand things right; this will loose a bit of trace when > people tickle the ioctl(), right? This might want a note of sorts, I don't actually see how, if they're doing the ioctl() on an enabled event, it'll generate unfiltered data until it gets scheduled out or it receives this cross call, which will apply the new filters. The __perf_event_stop(), which restarts the event will run on the target cpu, so it doesn't race with the workload. > - you need to handle event->oncpu == -1, Ok, we're in practice ignoring this but it deserves a comment. The cpu_function_call() here should return -ENXIO in this case and we'll just fall through, which is fine, because that means that the event is inactive and the filters will apply when it goes active again. > - can we race with an actual stop, and accidentally re-enable the > thing? If not, this might be a good place for a comment explaining > how. We can (mmap in workload vs munmap(AUX) in trace consumer), but the event's start will fail in perf_aux_output_begin() because of rb::aux_mmap_count being zero. The actual stop is only used in aux unmapping, so like this we should be covered. Thanks, -- Alex