From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EBB60C4167B for ; Wed, 29 Nov 2023 10:34:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229942AbjK2Ken (ORCPT ); Wed, 29 Nov 2023 05:34:43 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34172 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231540AbjK2JyA (ORCPT ); Wed, 29 Nov 2023 04:54:00 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 7F23D2123; Wed, 29 Nov 2023 01:53:43 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 54CAD2F4; Wed, 29 Nov 2023 01:54:30 -0800 (PST) Received: from [192.168.1.3] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 986C23F5A1; Wed, 29 Nov 2023 01:53:40 -0800 (PST) Message-ID: Date: Wed, 29 Nov 2023 09:53:39 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH RFC 2/3] perf/x86/intel/pt: Add support for pause_resume() Content-Language: en-US To: Adrian Hunter , Peter Zijlstra Cc: Ingo Molnar , Mark Rutland , Alexander Shishkin , Heiko Carstens , Thomas Richter , Hendrik Brueckner , Suzuki K Poulose , Mike Leach , coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, Yicong Yang , Jonathan Cameron , Will Deacon , Arnaldo Carvalho de Melo , Jiri Olsa , Namhyung Kim , Ian Rogers , linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org References: <20231123121851.10826-1-adrian.hunter@intel.com> <20231123121851.10826-3-adrian.hunter@intel.com> From: James Clark In-Reply-To: <20231123121851.10826-3-adrian.hunter@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23/11/2023 12:18, Adrian Hunter wrote: > Prevent tracing to start if aux_paused. > > Implement pause_resume() callback. When aux_paused, stop tracing. When > not aux_paused, only start tracing if it isn't currently meant to be > stopped. > > Signed-off-by: Adrian Hunter > --- > arch/x86/events/intel/pt.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c > index 42a55794004a..aa883b64814a 100644 > --- a/arch/x86/events/intel/pt.c > +++ b/arch/x86/events/intel/pt.c > @@ -418,6 +418,9 @@ static void pt_config_start(struct perf_event *event) > struct pt *pt = this_cpu_ptr(&pt_ctx); > u64 ctl = event->hw.config; > > + if (event->aux_paused) > + return; > + > ctl |= RTIT_CTL_TRACEEN; > if (READ_ONCE(pt->vmx_on)) > perf_aux_output_flag(&pt->handle, PERF_AUX_FLAG_PARTIAL); > @@ -1563,6 +1566,14 @@ EXPORT_SYMBOL_GPL(intel_pt_handle_vmx); > * PMU callbacks > */ > > +static void pt_event_pause_resume(struct perf_event *event) > +{ > + if (event->aux_paused) > + pt_config_stop(event); > + else if (!event->hw.state) > + pt_config_start(event); > +} It seems like having a single pause/resume callback rather than separate pause and resume ones pushes some of the event state management into the individual drivers and would be prone to code duplication and divergent behavior. Would it be possible to move the conditions from here into the core code and call separate functions instead? > + > static void pt_event_start(struct perf_event *event, int mode) > { > struct hw_perf_event *hwc = &event->hw; > @@ -1798,6 +1809,7 @@ static __init int pt_init(void) > pt_pmu.pmu.del = pt_event_del; > pt_pmu.pmu.start = pt_event_start; > pt_pmu.pmu.stop = pt_event_stop; > + pt_pmu.pmu.pause_resume = pt_event_pause_resume; The general idea seems ok to me. Is there a reason to not use the existing start() stop() callbacks, rather than adding a new one? I assume it's intended to be something like an optimisation where you can turn it on and off without having to do the full setup, teardown and emit an AUX record because you know the process being traced never gets switched out? Could you make it so that it works out of the box, with the option of later optimisation if you do something like this (not here but something like this in events/core.c): /* Use specialised pause/resume if it exists, otherwise use more * expensive start/stop. */ if (pmu->pause_resume) pmu->pause_resume(...) else pmu->stop(...) > pt_pmu.pmu.snapshot_aux = pt_event_snapshot_aux; > pt_pmu.pmu.read = pt_event_read; > pt_pmu.pmu.setup_aux = pt_buffer_setup_aux;